Skip to content

WIP/RFC: add support for manual link layer offset changes #1477

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stefan-baranoff
Copy link

This is mostly a check to see if there's interest in this kind of feature, if the approach is sane, and what all might need added if this is a feature with interest. If there's a better process for this, please let me know! The contribution guidelines are a little sparse in this area.


A few pcap-filter expressions implicitly change the link layer offset. There are other less common headers without their own keywords that might require skipping over some bytes in a link layer adjacent header to get to higher layer network headers. This commit adds an "offset" term that takes a + or - value to adjust the link layer offets in the same way VLAN does. This allows for something like ethertype 0x8926 and offset +6 and vlan 14 and ip host 192.0.2.14 to skip over a vntag header, process a VLAN header, and look for an IPv4 host.

A few pcap-filter expressions implicitly change the link layer offset.
There are other less common headers without their own keywords that
might require skipping over some bytes in a link layer adjacent header
to get to higher layer network headers. This commit adds an "offset"
term that takes a + or - value to adjust the link layer offets in the
same way VLAN does. This allows for something like `ethertype 0x8926
and offset +6 and vlan 14 and ip host 192.0.2.14` to skip over a vntag
header, process a VLAN header, and look for an IPv4 host.
gencode.c Outdated
@@ -10712,3 +10712,11 @@ gen_atmmulti_abbrev(compiler_state_t *cstate, int type)
}
return b1;
}

struct block *gen_offset_adjustment(compiler_state_t *cstate, int n) {
struct block *b = new_block(cstate, BPF_JMP|BPF_JA);
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't think of a better way of generating a "noop" block. I tried BPF_ALU|BPF_ADD|BPF_K with a value of 0, but the optimizer turned it into an unimplemented operator with an operand of 0xffff.

This is an odd ball in pcap-filter syntax, since it doesn't "check" for anything; it's an "always true" term that controls parsing rather than checking anything. But without returning a block something like offset+6 and host 192.0.2.68 would never work, since and assumes a block on both sides (as best I can tell). Returning NULL or nothing doesn't really work.

@infrastation
Copy link
Member

From the implementation perspective, it seems a better fit to use a C function that modifies the offsets and returns gen_true(cstate). From the syntax perspective, OFFSET et al. are unnecessary because the argument of the new keyword could be what is known as narth in the existing grammar terms, then the following would be valid syntax:

offset 4
offset +4
offset -4
offset 2 + 2
offset (len - 4)
offset link[4 * 5]

...and so on.

@stefan-baranoff
Copy link
Author

From the implementation perspective, it seems a better fit to use a C function that modifies the offsets and returns gen_true(cstate). From the syntax perspective, OFFSET et al. are unnecessary because the argument of the new keyword could be what is known as narth in the existing grammar terms, then the following would be valid syntax:

offset 4
offset +4
offset -4
offset 2 + 2
offset (len - 4)
offset link[4 * 5]

...and so on.

For some reason I cannot reply directly to your previous comment, so adding this here:

I think I mostly follow what you're suggesting here, although offset +4 and offset 4 at the moment in my latest attempt do the same thing, which is probably okay. My original intent with +X and -X was to make it clear these were relative adjustments. I believe what I've pushed most recently works conceptually. Per the commit comments, I would be interested in feedback on:

  1. Is this even worth continuing to pursue? Is there general support for it?
  2. Is this the right process for pursuing this, or is there a better venue for having these discussions and getting advice on code approach/design?
  3. Am I on track with what you were thinking around narth? It took a bit to figure out how that all worked, but I think I'm starting to understand it better.
  4. How do you think would be best to handle allowing adjustments of multiple offsets? E.g. VLAN adjusts off_linkhdr and off_linkpl but MPLS adjusts off_nl and off_nl_nosnap. Then there are other offsets that may or may not make sense to adjust, but I'm not familiar with their purposes or the protocols they support. Is something like I proposed in the commit comment of the off-linkhdr/off-linkpl style keywords for at least those 4 fields?
  5. As I'm working through this, I'm realizing pcap-filter hasn't really had "behavior control" included before. While I started this project to see if I could help a team with VNTag without hard coding it to just VNTag (who knows what they'll need next) I realized it could make the vlan X or vlan Y case behave more as expected, ish. That would be ugly and fragile as vlan X and offset -4 or vlan Y and .... The issue is, reading the code now, VLAN isn't always a jump by 4 due to all of the places it can be stashed. So, that got me wondering about that use case and if it's worth looking at a "control" word concept to do something like vlan-jump-off and vlan X or VLAN y or VLAN z) vlan-jump-on and VLAN and .... Obviously a different PR entirely if that's worth doing, but it sure doesn't look like it would be too hard and might solve that long-standing VLAN jumping issue in a backwards compatible way!

@stefan-baranoff stefan-baranoff force-pushed the manual-offset branch 2 times, most recently from 1a4f736 to a1a2abd Compare February 28, 2025 23:05
@infrastation
Copy link
Member

It is difficult to provide a more specific answer at this time, but it certainly looks like a worthwhile direction to research, at least to be able to state exactly how far the solution space would extend. This will likely take some time, so if you have a use case and a good solution for VN tag (see also #770), it may be better to consider that first.

Some keywords, like VLAN and MPLS, implicitly adjust offsets. New protocols not
yet handled by pcap-filter may also require offset jumping, but are not part of
the existing known protocol code. Allowing arbitrary offset adjustments by users
supports overcoming long standing "vlan X or vlan Y" issues, albeit with awkward
syntax, and for jumping over new protocols not yet handled explicitly, like
VNTag.

This second attempt switches from adjusting compile time static offsets to using
the variable arguments already baked into the offset structs to allow use with
nvarg to generate either fixed or dynamically computed jumps. There is no
support for "set to X"; all actions are increment or decrement. There is also no
support at this time for selecting which offsets to adjust. That can be added
once the basic concept is flushed out and there's some agreement on if this is a
desired feature, syntax, and implementation approach.

Initially my thoughts for a next commit to support selecting offset are
something like `off-linkhdr` or `off-nl` or `off-linkpl` for each type of offset
that might be reasonable for an end user to want to adjust themseleves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants