* [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 @ 2017-03-21 18:54 Florian Westphal 2017-03-22 13:09 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2017-03-21 18:54 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6 add icmpv6 to nexthdr list so base gets updated correctly. Reported-by: Thomas Woerner <twoerner@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de> --- NB: This is STILL not correct. nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as icmpv6 type nd-router-advert because post processing removes the l3 dependency. However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6 which isn't the same as meta l4proto icmpv6. I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)? diff --git a/src/proto.c b/src/proto.c index 79e9dbf2b33e..fcdfbe73c735 100644 --- a/src/proto.c +++ b/src/proto.c @@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = { PROTO_LINK(IPPROTO_TCP, &proto_tcp), PROTO_LINK(IPPROTO_DCCP, &proto_dccp), PROTO_LINK(IPPROTO_SCTP, &proto_sctp), + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), }, .templates = { [0] = PROTO_META_TEMPLATE("l4proto", &inet_protocol_type, NFT_META_L4PROTO, 8), -- 2.10.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-21 18:54 [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 Florian Westphal @ 2017-03-22 13:09 ` Pablo Neira Ayuso 2017-03-22 13:44 ` Florian Westphal 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-22 13:09 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote: > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6 > > add icmpv6 to nexthdr list so base gets updated correctly. > > Reported-by: Thomas Woerner <twoerner@redhat.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > NB: This is STILL not correct. > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > gets listed as > icmpv6 type nd-router-advert > > because post processing removes the l3 dependency. > > However, "icmpv6 type nd-router-advert" uses dependency > ip6 nexthdr icmpv6 > which isn't the same as meta l4proto icmpv6. > > I suspect nft should always generate implicit l4 dependencies > via meta in the ipv6 case, what do others think > (and not autoremove 'nexthdr' check)? I think we should use meta l4proto, ip6 nexthdr may point to some of the extension headers in the packet actually. > diff --git a/src/proto.c b/src/proto.c > index 79e9dbf2b33e..fcdfbe73c735 100644 > --- a/src/proto.c > +++ b/src/proto.c > @@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = { > PROTO_LINK(IPPROTO_TCP, &proto_tcp), > PROTO_LINK(IPPROTO_DCCP, &proto_dccp), > PROTO_LINK(IPPROTO_SCTP, &proto_sctp), > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), This also allows icmp6 from IPv4, right? I remember I mentioned this in a patch that I attached to bugzilla at some point so I didn't apply this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 13:09 ` Pablo Neira Ayuso @ 2017-03-22 13:44 ` Florian Westphal 2017-03-22 15:29 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2017-03-22 13:44 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote: > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6 > > > > add icmpv6 to nexthdr list so base gets updated correctly. > > > > Reported-by: Thomas Woerner <twoerner@redhat.com> > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > NB: This is STILL not correct. > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > gets listed as > > icmpv6 type nd-router-advert > > > > because post processing removes the l3 dependency. > > > > However, "icmpv6 type nd-router-advert" uses dependency > > ip6 nexthdr icmpv6 > > which isn't the same as meta l4proto icmpv6. > > > > I suspect nft should always generate implicit l4 dependencies > > via meta in the ipv6 case, what do others think > > (and not autoremove 'nexthdr' check)? > > I think we should use meta l4proto, ip6 nexthdr may point to some of > the extension headers in the packet actually. Yes. Alright, I'll work on this change towards l4 meta. > > diff --git a/src/proto.c b/src/proto.c > > index 79e9dbf2b33e..fcdfbe73c735 100644 > > --- a/src/proto.c > > +++ b/src/proto.c > > @@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = { > > PROTO_LINK(IPPROTO_TCP, &proto_tcp), > > PROTO_LINK(IPPROTO_DCCP, &proto_dccp), > > PROTO_LINK(IPPROTO_SCTP, &proto_sctp), > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > This also allows icmp6 from IPv4, right? I remember I mentioned this > in a patch that I attached to bugzilla at some point so I didn't apply > this. Yes, whats the concern with that? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 13:44 ` Florian Westphal @ 2017-03-22 15:29 ` Pablo Neira Ayuso 2017-03-22 15:32 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-22 15:29 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Mar 22, 2017 at 02:44:12PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote: > > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > > <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6 > > > > > > add icmpv6 to nexthdr list so base gets updated correctly. > > > > > > Reported-by: Thomas Woerner <twoerner@redhat.com> > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > > --- > > > NB: This is STILL not correct. > > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > > gets listed as > > > icmpv6 type nd-router-advert > > > > > > because post processing removes the l3 dependency. > > > > > > However, "icmpv6 type nd-router-advert" uses dependency > > > ip6 nexthdr icmpv6 > > > which isn't the same as meta l4proto icmpv6. > > > > > > I suspect nft should always generate implicit l4 dependencies > > > via meta in the ipv6 case, what do others think > > > (and not autoremove 'nexthdr' check)? > > > > I think we should use meta l4proto, ip6 nexthdr may point to some of > > the extension headers in the packet actually. > > Yes. > Alright, I'll work on this change towards l4 meta. > > > > diff --git a/src/proto.c b/src/proto.c > > > index 79e9dbf2b33e..fcdfbe73c735 100644 > > > --- a/src/proto.c > > > +++ b/src/proto.c > > > @@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = { > > > PROTO_LINK(IPPROTO_TCP, &proto_tcp), > > > PROTO_LINK(IPPROTO_DCCP, &proto_dccp), > > > PROTO_LINK(IPPROTO_SCTP, &proto_sctp), > > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > > > This also allows icmp6 from IPv4, right? I remember I mentioned this > > in a patch that I attached to bugzilla at some point so I didn't apply > > this. > > Yes, whats the concern with that? Not a problem these days from inet/netdev chains since: commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c Author: Arturo Borrero Gonzalez <arturo@debian.org> Date: Wed Jan 25 12:51:08 2017 +0100 payload: explicit network ctx assignment for icmp/icmp6 in special families Now we generate the right bytecode to restrict this to IPv6: # nft --debug=netlink add rule inet f i icmpv6 type nd-router-advert inet f i [ meta load nfproto => reg 1 ] [ cmp eq reg 1 0x0000000a ] [ payload load 1b @ network header + 6 => reg 1 ] [ cmp eq reg 1 0x0000003a ] [ payload load 1b @ transport header + 0 => reg 1 ] [ cmp eq reg 1 0x00000086 ] So forget my concern, just remove this mental "postit" note, it's stale ;) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 15:29 ` Pablo Neira Ayuso @ 2017-03-22 15:32 ` Pablo Neira Ayuso 2017-03-22 15:44 ` Florian Westphal 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-22 15:32 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Mar 22, 2017 at 04:29:09PM +0100, Pablo Neira Ayuso wrote: > On Wed, Mar 22, 2017 at 02:44:12PM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote: > > > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > > > <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6 > > > > > > > > add icmpv6 to nexthdr list so base gets updated correctly. > > > > > > > > Reported-by: Thomas Woerner <twoerner@redhat.com> > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > > > --- > > > > NB: This is STILL not correct. > > > > nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > > > > gets listed as > > > > icmpv6 type nd-router-advert > > > > > > > > because post processing removes the l3 dependency. > > > > > > > > However, "icmpv6 type nd-router-advert" uses dependency > > > > ip6 nexthdr icmpv6 > > > > which isn't the same as meta l4proto icmpv6. > > > > > > > > I suspect nft should always generate implicit l4 dependencies > > > > via meta in the ipv6 case, what do others think > > > > (and not autoremove 'nexthdr' check)? > > > > > > I think we should use meta l4proto, ip6 nexthdr may point to some of > > > the extension headers in the packet actually. > > > > Yes. > > Alright, I'll work on this change towards l4 meta. > > > > > > diff --git a/src/proto.c b/src/proto.c > > > > index 79e9dbf2b33e..fcdfbe73c735 100644 > > > > --- a/src/proto.c > > > > +++ b/src/proto.c > > > > @@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = { > > > > PROTO_LINK(IPPROTO_TCP, &proto_tcp), > > > > PROTO_LINK(IPPROTO_DCCP, &proto_dccp), > > > > PROTO_LINK(IPPROTO_SCTP, &proto_sctp), > > > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > > > > > This also allows icmp6 from IPv4, right? I remember I mentioned this > > > in a patch that I attached to bugzilla at some point so I didn't apply > > > this. > > > > Yes, whats the concern with that? > > Not a problem these days from inet/netdev chains since: > > commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c > Author: Arturo Borrero Gonzalez <arturo@debian.org> > Date: Wed Jan 25 12:51:08 2017 +0100 > > payload: explicit network ctx assignment for icmp/icmp6 in special families > > Now we generate the right bytecode to restrict this to IPv6: > > # nft --debug=netlink add rule inet f i icmpv6 type nd-router-advert > inet f i > [ meta load nfproto => reg 1 ] > [ cmp eq reg 1 0x0000000a ] > [ payload load 1b @ network header + 6 => reg 1 ] > [ cmp eq reg 1 0x0000003a ] > [ payload load 1b @ transport header + 0 => reg 1 ] > [ cmp eq reg 1 0x00000086 ] > > So forget my concern, just remove this mental "postit" note, it's stale ;) Hm, I wonder why you need this new line in proto_inet_service: + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 15:32 ` Pablo Neira Ayuso @ 2017-03-22 15:44 ` Florian Westphal 2017-03-22 16:07 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2017-03-22 15:44 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Hm, I wonder why you need this new line in proto_inet_service: > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), meta_expr_pctx_update calls proto_find_upper(), without this that returns NULL and proto base is set to 'unknown'. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 15:44 ` Florian Westphal @ 2017-03-22 16:07 ` Pablo Neira Ayuso 2017-03-22 19:22 ` Florian Westphal 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-22 16:07 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hm, I wonder why you need this new line in proto_inet_service: > > > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > meta_expr_pctx_update calls proto_find_upper(), without this > that returns NULL and proto base is set to 'unknown'. Oh right. Will this still happen if you tell nft to generate the dependency using meta l4proto instead of ip6 nexthdr? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 16:07 ` Pablo Neira Ayuso @ 2017-03-22 19:22 ` Florian Westphal 2017-03-24 11:50 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2017-03-22 19:22 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > Hm, I wonder why you need this new line in proto_inet_service: > > > > > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > > > meta_expr_pctx_update calls proto_find_upper(), without this > > that returns NULL and proto base is set to 'unknown'. > > Oh right. > > Will this still happen if you tell nft to generate the dependency > using meta l4proto instead of ip6 nexthdr? Yes, tried with src/nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:41-51: Error: conflicting protocols specified: unknown vs. icmpv6 and this patch: diff --git a/src/proto.c b/src/proto.c --- a/src/proto.c +++ b/src/proto.c @@ -707,7 +707,7 @@ const struct proto_desc proto_icmp6 = { const struct proto_desc proto_ip6 = { .name = "ip6", .base = PROTO_BASE_NETWORK_HDR, - .protocol_key = IP6HDR_NEXTHDR, + .protocol_key = IP6HDR_INVALID, .protocols = { PROTO_LINK(IPPROTO_ESP, &proto_esp), PROTO_LINK(IPPROTO_AH, &proto_ah), @@ -720,6 +720,7 @@ const struct proto_desc proto_ip6 = { PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), }, .templates = { + [IP6HDR_INVALID] = PROTO_META_TEMPLATE("nfproto", &inet_protocol_type, NFT_META_L4PROTO, 8), [IP6HDR_VERSION] = HDR_BITFIELD("version", &integer_type, 0, 4), [IP6HDR_DSCP] = HDR_BITFIELD("dscp", &dscp_type, 4, 6), [IP6HDR_ECN] = HDR_BITFIELD("ecn", &ecn_type, 10, 2), ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-22 19:22 ` Florian Westphal @ 2017-03-24 11:50 ` Pablo Neira Ayuso 2017-03-24 12:21 ` Florian Westphal 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-24 11:50 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Mar 22, 2017 at 08:22:52PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote: > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > Hm, I wonder why you need this new line in proto_inet_service: > > > > > > > > + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > > > > > > meta_expr_pctx_update calls proto_find_upper(), without this > > > that returns NULL and proto base is set to 'unknown'. > > > > Oh right. > > > > Will this still happen if you tell nft to generate the dependency > > using meta l4proto instead of ip6 nexthdr? > > Yes, tried with > > src/nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert > <cmdline>:1:41-51: Error: conflicting protocols specified: unknown vs. icmpv6 > > and this patch: > > diff --git a/src/proto.c b/src/proto.c > --- a/src/proto.c > +++ b/src/proto.c > @@ -707,7 +707,7 @@ const struct proto_desc proto_icmp6 = { > const struct proto_desc proto_ip6 = { > .name = "ip6", > .base = PROTO_BASE_NETWORK_HDR, > - .protocol_key = IP6HDR_NEXTHDR, > + .protocol_key = IP6HDR_INVALID, In order spots, we just remove this line given IP6HDR_INVALID is zero. I think this may be confusing to newcomers reading the code. > .protocols = { > PROTO_LINK(IPPROTO_ESP, &proto_esp), > PROTO_LINK(IPPROTO_AH, &proto_ah), > @@ -720,6 +720,7 @@ const struct proto_desc proto_ip6 = { > PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), > }, > .templates = { > + [IP6HDR_INVALID] = PROTO_META_TEMPLATE("nfproto", &inet_protocol_type, NFT_META_L4PROTO, 8), We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 too, right? And use: [0] = PROTO_META_TEMPLATE(...) for consistency. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-24 11:50 ` Pablo Neira Ayuso @ 2017-03-24 12:21 ` Florian Westphal 2017-03-29 10:13 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2017-03-24 12:21 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 > too, right? Right, we can indeed do that and change ip as well. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 2017-03-24 12:21 ` Florian Westphal @ 2017-03-29 10:13 ` Pablo Neira Ayuso 0 siblings, 0 replies; 11+ messages in thread From: Pablo Neira Ayuso @ 2017-03-29 10:13 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Fri, Mar 24, 2017 at 01:21:12PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 > > too, right? > > Right, we can indeed do that and change ip as well. BTW, I think this problem may be the root cause for this report: https://bugzilla.netfilter.org/show_bug.cgi?id=1138 Probably these people are just getting that ICMPv6 with some extension header. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-29 10:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-21 18:54 [PATCH nft] src: allow update of net base w. meta l4proto icmpv6 Florian Westphal 2017-03-22 13:09 ` Pablo Neira Ayuso 2017-03-22 13:44 ` Florian Westphal 2017-03-22 15:29 ` Pablo Neira Ayuso 2017-03-22 15:32 ` Pablo Neira Ayuso 2017-03-22 15:44 ` Florian Westphal 2017-03-22 16:07 ` Pablo Neira Ayuso 2017-03-22 19:22 ` Florian Westphal 2017-03-24 11:50 ` Pablo Neira Ayuso 2017-03-24 12:21 ` Florian Westphal 2017-03-29 10:13 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).