expression rejects all packets message when adding vlan at the end of BPF filter

[seladb]

when compiling a BPF filter of this kind:

(src port 80) and (ip and src net 10.0.0.1) and (vlan 1)

there is an error thrown:

expression rejects all packets

However when moving the vlan 1 part to the beginning compilation succeeds:

(vlan 1) and (src port 80) and (ip and src net 10.0.0.1)

This was tried on libpcap 1.8.1.

This looks a little similar to #600 but on a different type of filter (vlan instead of ipv6).

I wonder why the order of the vlan clause matters?

The same issue is also reported in PcapPlusPlus: seladb/PcapPlusPlus#409

[fenner]

vlan is an unusual filter argument in that it modifies the filter from that point on. You can see what's happening if you look at the un-optimized code (it's the optimizer that discovers the filter accepts no packets):

tcpdump -O -d '(src port 80) and (ip and src net 10.0.0.1) and (vlan 1)
...
(000) ldh      [12]
(001) jeq      #0x86dd          jt 2    jf 14
...
(014) ldh      [12]
(015) jeq      #0x800           jt 16   jf 61
...
(036) jeq      #0x50            jt 37   jf 61
(037) ldh      [12]
(038) jeq      #0x800           jt 39   jf 61
(039) ldh      [12]
(040) jeq      #0x800           jt 41   jf 43
(041) ld       [26]
(042) jeq      #0xa000001       jt 51   jf 43
...
(051) ldh      [12]
(052) jeq      #0x8100          jt 57   jf 53
(053) ldh      [12]
(054) jeq      #0x88a8          jt 57   jf 55
(055) ldh      [12]
(056) jeq      #0x9100          jt 57   jf 61
...
(061) ret      #0

Pretend we're parsing an IP packet with source 10.0.0.1 - ethertype is not 0x86dd so we go to 14, ethertype is 0x800 so we go through the port and net tests to 51, where we test that it's a vlan packet - but the vlan requires that the packet be ethertype 0x8100, 0x88a8, or 0x9100. So, since the packet can't be ether proto 0x800 and also ether proto 0x8100, the filter rejects all packets.

If you look at the same filter compiled with "vlan" first, you'll see that it checks the normal ethertype for 0x8100 etc, then the subsequent ethertype for v4/v6.

The "vlan" keyword is not commutative. It would be nice to fix this, but it would take a complete rewrite of the compiler.

[seladb]

Thanks for the thorough explanation! I understand it won't be an easy fix, but if can consider fixing it at some point that'd be nice. Otherwise it's also ok

[guyharris]

The "vlan" keyword is not commutative. It would be nice to fix this, but it would take a complete rewrite of the compiler.

And, for better or worse, would amount to an API/ABI change - if any programs, scripts, or users have filters that depend on vlan's unusual behavior, fixing it to be less of a hack might break that.

[Oppen]

It'd probably end up in a maintenance mess, but maybe the BPF compiler could be versioned. Unless specified, you use version 1 (so old programs don't break), but you can call a new function to set the version, so version 2 would contain that fix. Does that sound as a decent idea?

[mcr]

Mario Rugiero <notifications@github.com> wrote: It'd probably end up in a maintenance mess, but maybe the BPF compiler could be versioned. Unless specified, you use version 1 (so old programs don't break), but you can call a new function to set the version, so version 2 would contain that fix. Does that sound as a decent idea? It's not a crazy thing. I'd do this with a single compile that had something like "version2" keyword that enabled the new behaviour.

[Oppen]

It's not a crazy thing. I'd do this with a single compile that had something like "version2" keyword that enabled the new behaviour.

I meant something accessible at runtime. Imagine you have a proprietary program that links to libpcap and attempts to load a filter (assume that miraculously it doesn't break for other incompatibilities), it happily installs the filter, but since you updated your system it does unexpected things. It may be argued that it should just be used with older versions of libpcap built for the new system, though.

[infrastation]

Thank you everyone for your input, this issue has been filed to FAQ Q13 for any future development, now closing.