Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

epb: add three new epb options, packetid, queue and verdict #91

Merged
merged 4 commits into from Jun 3, 2020
Merged

epb: add three new epb options, packetid, queue and verdict #91

merged 4 commits into from Jun 3, 2020

Conversation

chaudron
Copy link
Contributor

@chaudron chaudron commented Apr 9, 2020

Signed-off-by: Eelco Chaudron echaudro@redhat.com

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
@chaudron
Copy link
Contributor Author

chaudron commented Apr 9, 2020

Not sure if I had to add my name as an editor if I added text. If not I can remove it, no problem.

Also for some reason, I'm not getting emails from the pcap-ng-format mailing list. I've asked the pcap-ng-format-owner to verify I'm properly registered but got no update. So I will monitor the list manually, but if you do send related emails there I would appreciate it if you can put me on CC directly.


<t>Example: '0'.</t>

<t hangText="epb_verdict:"><vspace blankLines="0"/>The epb_verdict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "verdict" mean? I assume it means firewall/ACL activity like "drop", "reject", "accept"? Not all readers might be familiar with the term, so it would be good to explain it just a bit :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some additional explanation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the Linux_eBPF_TC and Linux_eBPF_XDP verdict types appear to be integral values; if so, are they in the byte order of the host writing the file (so that a little-endian machine will write them as little-endian numbers and a big-endian machine will write them as big-endian numbers)?

And is the hardware verdict type an opaque blob of bytes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The byte order question also applies to the packet ID and queue options, with presumably the same answer.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Linux_eBPF_TC and Linux_eBPF_XDP are stored a uint64 and hence the same as all other metadata in the block, so in host order as explained in the "Endianness" section.

You are right about the hardware verdict type, this is an opaque blob of bytes.

Packet and queue ID are the same as any other option, for example, epb_dropcount, which are stored as explained in the "Endianness" section.

@mcr
Copy link
Collaborator

mcr commented Apr 9, 2020 via email

@netoptimizer
Copy link

General question what does "epb" stand for?

@packetfoo
Copy link
Contributor

General question what does "epb" stand for?

It stands for "Enhanced Packet Block", which is the most commonly used block type to store each packet.

@netoptimizer
Copy link

General question what does "epb" stand for?

It stands for "Enhanced Packet Block", which is the most commonly used block type to store each packet.

Thanks for explaining that! :-)

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
@chaudron
Copy link
Contributor Author

Generally, in an IETF document, we would acknowledge all contributors in the Acknolwedgements section, but only list the lead authors/editors in the heading. There is a guideline for five authors only on an RFC, and we'll justify or prune names if/when we get closer to that stage.

Ok, will leave it in for now, or let me know and I'll remove/move my name.

@chaudron
Copy link
Contributor Author

Anything pending from my side, or something I can do to get this merged?

type, while the following octets contain the actual verdict data,
whose size depends on the verdict type, and hence from the value
in the first octet. The verdict type can be: Hardware (type
octet = 0, size = XXX), Linux_eBPF_TC (type octet = 1, size = 8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"XXX" presumably meaning "variable".

Copy link
Contributor Author

@chaudron chaudron Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it means variable, copied this layout from the epb_hash option above. I will change XXX to variable, as it makes more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the epb_hash option is vague about those hashes in a fashion that doesn't make it clear whether 1) they haven't yet decided how big the hash should be or 2) deliberately left it unspecified to allow different-width hashes to be used.

type, while the following octets contain the actual verdict data,
whose size depends on the verdict type, and hence from the value
in the first octet. The verdict type can be: Hardware (type
octet = 0, size = XXX), Linux_eBPF_TC (type octet = 1, size = 8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably point to a source indicating what the values for Linux eBPF TC and Linux eBPF XDP values signify.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

The verdict type can be: Hardware (type
octet = 0, size = variable), Linux_eBPF_TC (type octet = 1, size = 8
(64-bit unsigned integer), value = TC_ACT_* as defined in the Linux
pck_cls.h
include), Linux_eBPF_XDP (type octet = 2, size = 8 (64-bit unsigned
integer), value = xdp_action as defined in the Linux
pbf.h
include).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So where can these "pck_cls.h" and "pbf.h" files be found? They can't be found in any obvious place in the Linux 5.6.7 kernel source, at least not if the source tarball is downloaded and unpacked and a "find" command is run in the top-level directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice... The Github editor removed the ref links I put in...

The verdict type can be: Hardware (type
            octet = 0, size = variable), Linux_eBPF_TC (type octet = 1, size = 8
            (64-bit unsigned integer), value = TC_ACT_* as defined in the Linux
            <eref target="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/pkt_cls.h">pck_cls.h</eref>
            include), Linux_eBPF_XDP (type octet = 2, size = 8 (64-bit unsigned
            integer), value = xdp_action as defined in the Linux
            <eref target="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/bpf.h">pbf.h</eref>
            include).</t>

example could be where an XDP program on ingress captures a packet,
which gets modified and then exits the XDP program. In this case,
two packets are in the capture file, which are not identical but
the epb_packetid can be used correlate them.</t>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the capture has two packets that are different from each other but that are the same?

This probably needs to be rephrased in a way to make it clearer that "the packet" doesn't refer to the exact sequence of octets. Perhaps it should include a less-exotic example than an XDP one, such as a packet captured on a multiple-interface router that arrives on one interface, gets its IP TTL reduced, and is transmitted on another interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the idea, what about changing the example to:

An example could be a router that captures packets on all its interfaces in both directions. When a packet hits interface A on ingress, an ECB entry gets created, TTL gets decremented, and right before it egresses on interface B another ECB entry gets created in the trace file. In this case, two packets are in the capture file, which are not identical but the epb_packetid can be used to correlate them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an "ECB"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, should be EPB :(

@chaudron
Copy link
Contributor Author

Commited the changes suggested by @guyharris as I got no further feedback. Please let me know if I need to change more...

epb_packetid option is a 64-bit unsigned integer that uniquely
identifies the packet. If the same packet is seen by multiple
interfaces and there is a way for the capture application to
correlated them, the same epb_packetid value must be used. An
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be "to correlate them".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks will fix in next update...

<t hangText="epb_verdict:"><vspace blankLines="0"/>The epb_verdict
option stores a verdict of the packet. The verdict indicates what
would be done with the packet after processing it. For example, a
firewall implemented with eBPF could drop the packet. This verdict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just say "a firewall could drop the packet"; how the firewall rules are internally implemented is a detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it!

chaudron added a commit to chaudron/xdp-tools that referenced this pull request May 1, 2020
See the following pull request for details:
  IETF-OPSAWG-WG/draft-ietf-opsawg-pcap#91

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
chaudron added a commit to chaudron/xdp-tools that referenced this pull request May 1, 2020
See the following pull request for details:
  IETF-OPSAWG-WG/draft-ietf-opsawg-pcap#91

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
@chaudron
Copy link
Contributor Author

chaudron commented May 4, 2020

@guyharris pushed the suggested changes and fixed one other issue with a duplicate ecb_option number. Anything else?

@chaudron
Copy link
Contributor Author

@guyharris anything else I can do to speed up the acceptance of this change? I've done a rough implementation in WireShark to display these new fields.

@chaudron
Copy link
Contributor Author

chaudron commented Jun 2, 2020

@guyharris @mcr anything I can do to speed up this merge request? Asking as I have a project that would really benefit from this.

@guyharris
Copy link
Collaborator

@guyharris @mcr anything I can do to speed up this merge request? Asking as I have a project that would really benefit from this.

I'm OK with it; @mcr, are you OK with @chaudron's appearance in the authors list?

@mcr mcr merged commit 587eb9f into IETF-OPSAWG-WG:master Jun 3, 2020
@chaudron chaudron deleted the ecb_options branch June 3, 2020 15:05
@chaudron
Copy link
Contributor Author

chaudron commented Jun 3, 2020

Thanks a lot for merging this!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants