| Message ID | 1448615614-16510-1-git-send-email-kaber@trash.net |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
Patrick McHardy <kaber@trash.net> wrote:
> The VID is located after Priority and CFI.
With this patch matching on vlan id does not work for me anymore
on x86-64.
With trace-patch nft but without this patch:
table bridge filter {
chain input {
type filter hook input priority -200; policy accept;
vlan id 4094 counter packets 827 bytes 63839
With this patch, the counters remain at zero:
unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375
vlan id 4094 counter packets 0 bytes 0
(The 'unknown unknown' line is the 'old' vlan rule added by unpatched
nft binary, the 'vlan id' is the one added with the patched one).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.11, Florian Westphal wrote: > Patrick McHardy <kaber@trash.net> wrote: > > The VID is located after Priority and CFI. > > With this patch matching on vlan id does not work for me anymore > on x86-64. > > With trace-patch nft but without this patch: > > table bridge filter { > chain input { > type filter hook input priority -200; policy accept; > vlan id 4094 counter packets 827 bytes 63839 > > With this patch, the counters remain at zero: > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375 > vlan id 4094 counter packets 0 bytes 0 > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched > nft binary, the 'vlan id' is the one added with the patched one). Odd, since it decodes correctly. Could you send the output of nft --debug=netlink with and without the patch? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patrick McHardy <kaber@trash.net> wrote: > On 27.11, Florian Westphal wrote: > > Patrick McHardy <kaber@trash.net> wrote: > > > The VID is located after Priority and CFI. > > > > With this patch matching on vlan id does not work for me anymore > > on x86-64. > > > > With trace-patch nft but without this patch: > > > > table bridge filter { > > chain input { > > type filter hook input priority -200; policy accept; > > vlan id 4094 counter packets 827 bytes 63839 > > > > With this patch, the counters remain at zero: > > > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375 > > vlan id 4094 counter packets 0 bytes 0 > > > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched > > nft binary, the 'vlan id' is the one added with the patched one). > > Odd, since it decodes correctly. Could you send the output of > nft --debug=netlink with and without the patch? master: nft --debug=netlink add rule bridge filter input vlan id 4094 counter bridge filter input [ payload load 2b @ link header + 12 => reg 1 ] [ cmp eq reg 1 0x00000081 ] [ payload load 2b @ link header + 14 => reg 1 ] [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ] [ cmp eq reg 1 0x0000fe0f ] [ counter pkts 0 bytes 0 ] patched: # src/nft --debug=netlink add rule bridge filter input vlan id 4094 counter bridge filter input [ payload load 2b @ link header + 12 => reg 1 ] [ cmp eq reg 1 0x00000081 ] [ payload load 2b @ link header + 14 => reg 1 ] [ bitwise reg 1 = (reg=1 & 0x0000f0ff ) ^ 0x00000000 ] [ cmp eq reg 1 0x0000e0ff ] [ counter pkts 0 bytes 0 ] -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Florian Westphal <fw@strlen.de> wrote: > Patrick McHardy <kaber@trash.net> wrote: > > On 27.11, Florian Westphal wrote: > > > Patrick McHardy <kaber@trash.net> wrote: > > > > The VID is located after Priority and CFI. > > > > > > With this patch matching on vlan id does not work for me anymore > > > on x86-64. > > > > > > With trace-patch nft but without this patch: > > > > > > table bridge filter { > > > chain input { > > > type filter hook input priority -200; policy accept; > > > vlan id 4094 counter packets 827 bytes 63839 > > > > > > With this patch, the counters remain at zero: > > > > > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375 > > > vlan id 4094 counter packets 0 bytes 0 > > > > > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched > > > nft binary, the 'vlan id' is the one added with the patched one). > > > > Odd, since it decodes correctly. Could you send the output of > > nft --debug=netlink with and without the patch? > > master: > nft --debug=netlink add rule bridge filter input vlan id 4094 counter > bridge filter input > [ payload load 2b @ link header + 12 => reg 1 ] > [ cmp eq reg 1 0x00000081 ] > [ payload load 2b @ link header + 14 => reg 1 ] > [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ] > [ cmp eq reg 1 0x0000fe0f ] > [ counter pkts 0 bytes 0 ] I checked nft_payload again and I believe rebuild of the vlan header is correct (a bug there would also explain this problem). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.11, Florian Westphal wrote: > Florian Westphal <fw@strlen.de> wrote: > > Patrick McHardy <kaber@trash.net> wrote: > > > On 27.11, Florian Westphal wrote: > > > > Patrick McHardy <kaber@trash.net> wrote: > > > > > The VID is located after Priority and CFI. > > > > > > > > With this patch matching on vlan id does not work for me anymore > > > > on x86-64. > > > > > > > > With trace-patch nft but without this patch: > > > > > > > > table bridge filter { > > > > chain input { > > > > type filter hook input priority -200; policy accept; > > > > vlan id 4094 counter packets 827 bytes 63839 > > > > > > > > With this patch, the counters remain at zero: > > > > > > > > unknown unknown & 0xfff [invalid type] == 0xffe [invalid type] counter packets 850 bytes 65375 > > > > vlan id 4094 counter packets 0 bytes 0 > > > > > > > > (The 'unknown unknown' line is the 'old' vlan rule added by unpatched > > > > nft binary, the 'vlan id' is the one added with the patched one). > > > > > > Odd, since it decodes correctly. Could you send the output of > > > nft --debug=netlink with and without the patch? > > > > master: > > nft --debug=netlink add rule bridge filter input vlan id 4094 counter > > bridge filter input > > [ payload load 2b @ link header + 12 => reg 1 ] > > [ cmp eq reg 1 0x00000081 ] > > [ payload load 2b @ link header + 14 => reg 1 ] > > [ bitwise reg 1 = (reg=1 & 0x0000ff0f ) ^ 0x00000000 ] > > [ cmp eq reg 1 0x0000fe0f ] > > [ counter pkts 0 bytes 0 ] > > I checked nft_payload again and I believe rebuild of the vlan header is > correct (a bug there would also explain this problem). Yes, I also did that and it looks correct. I think we probably have a discrepancy with bit numbering: Looking at an older patch of you: - [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4), - [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 4, 4), + [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 4, 4), + [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 0, 4), So you seem to assume a numbering which corresponds to how you would express it in C. My patch assumes numbering as used in the RFCs/IEEE standards, which is basically the opposite direction. I have to check that code in more detail. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patrick McHardy <kaber@trash.net> wrote: > Yes, I also did that and it looks correct. I think we probably have a > discrepancy with bit numbering: > > Looking at an older patch of you: > > - [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4), > - [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 4, 4), > + [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 4, 4), > + [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 0, 4), > > So you seem to assume a numbering which corresponds to how you would express > it in C. My patch assumes numbering as used in the RFCs/IEEE standards, which > is basically the opposite direction. Right, there is a general problem with all sub-byte fields. I just noticed that decoding of ip version/hdrlen doesn't work either. (ip hdrlength 4 ip version 5). I am sure that I tested matching on ip version/hdrlen on both x86-64 and a MSB machine (don't recall architecture, ppc i think). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.11, Florian Westphal wrote: > Patrick McHardy <kaber@trash.net> wrote: > > Yes, I also did that and it looks correct. I think we probably have a > > discrepancy with bit numbering: > > > > Looking at an older patch of you: > > > > - [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4), > > - [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 4, 4), > > + [IPHDR_VERSION] = HDR_BITFIELD("version", &integer_type, 4, 4), > > + [IPHDR_HDRLENGTH] = HDR_BITFIELD("hdrlength", &integer_type, 0, 4), > > > > So you seem to assume a numbering which corresponds to how you would express > > it in C. My patch assumes numbering as used in the RFCs/IEEE standards, which > > is basically the opposite direction. > > Right, there is a general problem with all sub-byte fields. > > I just noticed that decoding of ip version/hdrlen doesn't work either. > (ip hdrlength 4 ip version 5). > > I am sure that I tested matching on ip version/hdrlen on both > x86-64 and a MSB machine (don't recall architecture, ppc i think). Yeah, the actual variant chosen doesn't matter, I guess we just need to make sure it is handled consistently. I chose the other way because you can simply use the offsets defined in the standards. That's how I (think I) expressed all the protocol definitions, so if we're using the other way around, we'll possibly have some more bitfields which are not correct with the current code. I'll do a audit of the code and the other definitions and see which way would be easier to fix. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/proto.h b/include/proto.h index 974116f..d90bccd 100644 --- a/include/proto.h +++ b/include/proto.h @@ -159,9 +159,9 @@ enum eth_hdr_fields { enum vlan_hdr_fields { VLANHDR_INVALID, - VLANHDR_VID, - VLANHDR_CFI, VLANHDR_PCP, + VLANHDR_CFI, + VLANHDR_VID, VLANHDR_TYPE, }; diff --git a/src/proto.c b/src/proto.c index 0fe0b88..f2cf297 100644 --- a/src/proto.c +++ b/src/proto.c @@ -730,9 +730,9 @@ const struct proto_desc proto_vlan = { }, .templates = { - [VLANHDR_VID] = VLANHDR_BITFIELD("id", 0, 12), - [VLANHDR_CFI] = VLANHDR_BITFIELD("cfi", 12, 1), - [VLANHDR_PCP] = VLANHDR_BITFIELD("pcp", 13, 3), + [VLANHDR_PCP] = VLANHDR_BITFIELD("pcp", 0, 3), + [VLANHDR_CFI] = VLANHDR_BITFIELD("cfi", 3, 1), + [VLANHDR_VID] = VLANHDR_BITFIELD("id", 4, 12), [VLANHDR_TYPE] = VLANHDR_TYPE("type", ðertype_type, vlan_type), }, };
The VID is located after Priority and CFI. Signed-off-by: Patrick McHardy <kaber@trash.net> --- include/proto.h | 4 ++-- src/proto.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)