diff mbox

[nft] src: allow update of net base w. meta l4proto icmpv6

Message ID 20170322192252.GC8584@breakpoint.cc
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Florian Westphal March 22, 2017, 7:22 p.m. UTC
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:



--
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

Comments

Pablo Neira Ayuso March 24, 2017, 11:50 a.m. UTC | #1
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.
--
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 March 24, 2017, 12:21 p.m. UTC | #2
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.
--
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
Pablo Neira Ayuso March 29, 2017, 10:13 a.m. UTC | #3
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.
--
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 mbox

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),