[PATCH v3] netfilter: nf_tables: allow NFPROTO_INET in nft_(match/target)_validate()

[PATCH v3] netfilter: nf_tables: allow NFPROTO_INET in nft_(match/target)_validate()

[Ignat Korchagin]

Commit d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") added
some validation of NFPROTO_* families in the nft_compat module, but it broke
the ability to use legacy iptables modules in dual-stack nftables.

While with legacy iptables one had to independently manage IPv4 and IPv6
tables, with nftables it is possible to have dual-stack tables sharing the
rules. Moreover, it was possible to use rules based on legacy iptables
match/target modules in dual-stack nftables.

As an example, the program from [2] creates an INET dual-stack family table
using an xt_bpf based rule, which looks like the following (the actual output
was generated with a patched nft tool as the current nft tool does not parse
dual stack tables with legacy match rules, so consider it for illustrative
purposes only):

table inet testfw {
  chain input {
    type filter hook prerouting priority filter; policy accept;
    bytecode counter packets 0 bytes 0 accept
  }
}

After d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") we get
EOPNOTSUPP for the above program.

Fix this by allowing NFPROTO_INET for nft_(match/target)_validate(), but also
restrict the functions to classic iptables hooks.

Changes in v3:
  * clarify that upstream nft will not display such configuration properly and
    that the output was generated with a patched nft tool
  * remove example program from commit description and link to it instead
  * no code changes otherwise

Changes in v2:
  * restrict nft_(match/target)_validate() to classic iptables hooks
  * rewrite example program to use unmodified libnftnl

Fixes: d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family")
Link: https://lore.kernel.org/all/Zc1PfoWN38UuFJRI@calendula/T/#mc947262582c90fec044c7a3398cc92fac7afea72 [1]
Link: https://lore.kernel.org/all/20240220145509.53357-1-ignat@cloudflare.com/ [2]
Reported-by: Jordan Griege <jgriege@cloudflare.com>
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/netfilter/nft_compat.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index 1f9474fefe84..d3d11dede545 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -359,10 +359,20 @@ static int nft_target_validate(const struct nft_ctx *ctx,
 
 	if (ctx->family != NFPROTO_IPV4 &&
 	    ctx->family != NFPROTO_IPV6 &&
+	    ctx->family != NFPROTO_INET &&
 	    ctx->family != NFPROTO_BRIDGE &&
 	    ctx->family != NFPROTO_ARP)
 		return -EOPNOTSUPP;
 
+	ret = nft_chain_validate_hooks(ctx->chain,
+				       (1 << NF_INET_PRE_ROUTING) |
+				       (1 << NF_INET_LOCAL_IN) |
+				       (1 << NF_INET_FORWARD) |
+				       (1 << NF_INET_LOCAL_OUT) |
+				       (1 << NF_INET_POST_ROUTING));
+	if (ret)
+		return ret;
+
 	if (nft_is_base_chain(ctx->chain)) {
 		const struct nft_base_chain *basechain =
 						nft_base_chain(ctx->chain);
@@ -610,10 +620,20 @@ static int nft_match_validate(const struct nft_ctx *ctx,
 
 	if (ctx->family != NFPROTO_IPV4 &&
 	    ctx->family != NFPROTO_IPV6 &&
+	    ctx->family != NFPROTO_INET &&
 	    ctx->family != NFPROTO_BRIDGE &&
 	    ctx->family != NFPROTO_ARP)
 		return -EOPNOTSUPP;
 
+	ret = nft_chain_validate_hooks(ctx->chain,
+				       (1 << NF_INET_PRE_ROUTING) |
+				       (1 << NF_INET_LOCAL_IN) |
+				       (1 << NF_INET_FORWARD) |
+				       (1 << NF_INET_LOCAL_OUT) |
+				       (1 << NF_INET_POST_ROUTING));
+	if (ret)
+		return ret;
+
 	if (nft_is_base_chain(ctx->chain)) {
 		const struct nft_base_chain *basechain =
 						nft_base_chain(ctx->chain);
-- 
2.39.2

Re: [PATCH v3] netfilter: nf_tables: allow NFPROTO_INET in nft_(match/target)_validate()

[Pablo Neira Ayuso]

Hi Ignat,

On Thu, Feb 22, 2024 at 10:33:08AM +0000, Ignat Korchagin wrote:
> Commit d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") added
> some validation of NFPROTO_* families in the nft_compat module, but it broke
> the ability to use legacy iptables modules in dual-stack nftables.
> 
> While with legacy iptables one had to independently manage IPv4 and IPv6
> tables, with nftables it is possible to have dual-stack tables sharing the
> rules. Moreover, it was possible to use rules based on legacy iptables
> match/target modules in dual-stack nftables.
> 
> As an example, the program from [2] creates an INET dual-stack family table
> using an xt_bpf based rule, which looks like the following (the actual output
> was generated with a patched nft tool as the current nft tool does not parse
> dual stack tables with legacy match rules, so consider it for illustrative
> purposes only):
> 
> table inet testfw {
>   chain input {
>     type filter hook prerouting priority filter; policy accept;
>     bytecode counter packets 0 bytes 0 accept
>   }
> }

This nft command does not exist in tree, this does not restores fine
with nft -f. It provides a misleading hint to the reader.

I am fine with restoring this because you use it, but you have to find
a better interface than using nft_compat to achieve this IMO.

The upstream consensus this far is not to expose nft_compat features
through userspace nft. But as said, I understand and I am fine with
restoring kernel behaviour so you can keep going with your out-of-tree
patch.

Thanks !

> After d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") we get
> EOPNOTSUPP for the above program.
> 
> Fix this by allowing NFPROTO_INET for nft_(match/target)_validate(), but also
> restrict the functions to classic iptables hooks.
> 
> Changes in v3:
>   * clarify that upstream nft will not display such configuration properly and
>     that the output was generated with a patched nft tool
>   * remove example program from commit description and link to it instead
>   * no code changes otherwise
> 
> Changes in v2:
>   * restrict nft_(match/target)_validate() to classic iptables hooks
>   * rewrite example program to use unmodified libnftnl
> 
> Fixes: d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family")
> Link: https://lore.kernel.org/all/Zc1PfoWN38UuFJRI@calendula/T/#mc947262582c90fec044c7a3398cc92fac7afea72 [1]
> Link: https://lore.kernel.org/all/20240220145509.53357-1-ignat@cloudflare.com/ [2]
> Reported-by: Jordan Griege <jgriege@cloudflare.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> ---
>  net/netfilter/nft_compat.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
> index 1f9474fefe84..d3d11dede545 100644
> --- a/net/netfilter/nft_compat.c
> +++ b/net/netfilter/nft_compat.c
> @@ -359,10 +359,20 @@ static int nft_target_validate(const struct nft_ctx *ctx,
>  
>  	if (ctx->family != NFPROTO_IPV4 &&
>  	    ctx->family != NFPROTO_IPV6 &&
> +	    ctx->family != NFPROTO_INET &&
>  	    ctx->family != NFPROTO_BRIDGE &&
>  	    ctx->family != NFPROTO_ARP)
>  		return -EOPNOTSUPP;
>  
> +	ret = nft_chain_validate_hooks(ctx->chain,
> +				       (1 << NF_INET_PRE_ROUTING) |
> +				       (1 << NF_INET_LOCAL_IN) |
> +				       (1 << NF_INET_FORWARD) |
> +				       (1 << NF_INET_LOCAL_OUT) |
> +				       (1 << NF_INET_POST_ROUTING));
> +	if (ret)
> +		return ret;
> +
>  	if (nft_is_base_chain(ctx->chain)) {
>  		const struct nft_base_chain *basechain =
>  						nft_base_chain(ctx->chain);
> @@ -610,10 +620,20 @@ static int nft_match_validate(const struct nft_ctx *ctx,
>  
>  	if (ctx->family != NFPROTO_IPV4 &&
>  	    ctx->family != NFPROTO_IPV6 &&
> +	    ctx->family != NFPROTO_INET &&
>  	    ctx->family != NFPROTO_BRIDGE &&
>  	    ctx->family != NFPROTO_ARP)
>  		return -EOPNOTSUPP;
>  
> +	ret = nft_chain_validate_hooks(ctx->chain,
> +				       (1 << NF_INET_PRE_ROUTING) |
> +				       (1 << NF_INET_LOCAL_IN) |
> +				       (1 << NF_INET_FORWARD) |
> +				       (1 << NF_INET_LOCAL_OUT) |
> +				       (1 << NF_INET_POST_ROUTING));
> +	if (ret)
> +		return ret;
> +
>  	if (nft_is_base_chain(ctx->chain)) {
>  		const struct nft_base_chain *basechain =
>  						nft_base_chain(ctx->chain);
> -- 
> 2.39.2
> 

Re: [PATCH v3] netfilter: nf_tables: allow NFPROTO_INET in nft_(match/target)_validate()

[Ignat Korchagin]

Hi Pablo,

On Thu, Feb 22, 2024 at 10:52 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Ignat,
>
> On Thu, Feb 22, 2024 at 10:33:08AM +0000, Ignat Korchagin wrote:
> > Commit d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") added
> > some validation of NFPROTO_* families in the nft_compat module, but it broke
> > the ability to use legacy iptables modules in dual-stack nftables.
> >
> > While with legacy iptables one had to independently manage IPv4 and IPv6
> > tables, with nftables it is possible to have dual-stack tables sharing the
> > rules. Moreover, it was possible to use rules based on legacy iptables
> > match/target modules in dual-stack nftables.
> >
> > As an example, the program from [2] creates an INET dual-stack family table
> > using an xt_bpf based rule, which looks like the following (the actual output
> > was generated with a patched nft tool as the current nft tool does not parse
> > dual stack tables with legacy match rules, so consider it for illustrative
> > purposes only):
> >
> > table inet testfw {
> >   chain input {
> >     type filter hook prerouting priority filter; policy accept;
> >     bytecode counter packets 0 bytes 0 accept
> >   }
> > }
>
> This nft command does not exist in tree, this does not restores fine
> with nft -f. It provides a misleading hint to the reader.

I tried to clarify above that this is for illustrative purposes only -
just to give context about what we are trying to do, but do let me
know if you prefer a v4 with this completely removed.

> I am fine with restoring this because you use it, but you have to find
> a better interface than using nft_compat to achieve this IMO.

We're actually looking to restore the effort in [1] so some support
would be appreciated.

> The upstream consensus this far is not to expose nft_compat features
> through userspace nft. But as said, I understand and I am fine with
> restoring kernel behaviour so you can keep going with your out-of-tree
> patch.

Understood. There is no expectation from us that upstream userspace
nft should natively support this (as it didn't before d0009effa886),
but we can send the patch if consensus changes.

> Thanks !
>
> > After d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") we get
> > EOPNOTSUPP for the above program.
> >
> > Fix this by allowing NFPROTO_INET for nft_(match/target)_validate(), but also
> > restrict the functions to classic iptables hooks.
> >
> > Changes in v3:
> >   * clarify that upstream nft will not display such configuration properly and
> >     that the output was generated with a patched nft tool
> >   * remove example program from commit description and link to it instead
> >   * no code changes otherwise
> >
> > Changes in v2:
> >   * restrict nft_(match/target)_validate() to classic iptables hooks
> >   * rewrite example program to use unmodified libnftnl
> >
> > Fixes: d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family")
> > Link: https://lore.kernel.org/all/Zc1PfoWN38UuFJRI@calendula/T/#mc947262582c90fec044c7a3398cc92fac7afea72 [1]
> > Link: https://lore.kernel.org/all/20240220145509.53357-1-ignat@cloudflare.com/ [2]
> > Reported-by: Jordan Griege <jgriege@cloudflare.com>
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> > ---
> >  net/netfilter/nft_compat.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
> > index 1f9474fefe84..d3d11dede545 100644
> > --- a/net/netfilter/nft_compat.c
> > +++ b/net/netfilter/nft_compat.c
> > @@ -359,10 +359,20 @@ static int nft_target_validate(const struct nft_ctx *ctx,
> >
> >       if (ctx->family != NFPROTO_IPV4 &&
> >           ctx->family != NFPROTO_IPV6 &&
> > +         ctx->family != NFPROTO_INET &&
> >           ctx->family != NFPROTO_BRIDGE &&
> >           ctx->family != NFPROTO_ARP)
> >               return -EOPNOTSUPP;
> >
> > +     ret = nft_chain_validate_hooks(ctx->chain,
> > +                                    (1 << NF_INET_PRE_ROUTING) |
> > +                                    (1 << NF_INET_LOCAL_IN) |
> > +                                    (1 << NF_INET_FORWARD) |
> > +                                    (1 << NF_INET_LOCAL_OUT) |
> > +                                    (1 << NF_INET_POST_ROUTING));
> > +     if (ret)
> > +             return ret;
> > +
> >       if (nft_is_base_chain(ctx->chain)) {
> >               const struct nft_base_chain *basechain =
> >                                               nft_base_chain(ctx->chain);
> > @@ -610,10 +620,20 @@ static int nft_match_validate(const struct nft_ctx *ctx,
> >
> >       if (ctx->family != NFPROTO_IPV4 &&
> >           ctx->family != NFPROTO_IPV6 &&
> > +         ctx->family != NFPROTO_INET &&
> >           ctx->family != NFPROTO_BRIDGE &&
> >           ctx->family != NFPROTO_ARP)
> >               return -EOPNOTSUPP;
> >
> > +     ret = nft_chain_validate_hooks(ctx->chain,
> > +                                    (1 << NF_INET_PRE_ROUTING) |
> > +                                    (1 << NF_INET_LOCAL_IN) |
> > +                                    (1 << NF_INET_FORWARD) |
> > +                                    (1 << NF_INET_LOCAL_OUT) |
> > +                                    (1 << NF_INET_POST_ROUTING));
> > +     if (ret)
> > +             return ret;
> > +
> >       if (nft_is_base_chain(ctx->chain)) {
> >               const struct nft_base_chain *basechain =
> >                                               nft_base_chain(ctx->chain);
> > --
> > 2.39.2
> >

[1]: https://lore.kernel.org/netfilter-devel/20220831101617.22329-1-fw@strlen.de/

Ignat

Re: [PATCH v3] netfilter: nf_tables: allow NFPROTO_INET in nft_(match/target)_validate()

[Pablo Neira Ayuso]

On Thu, Feb 22, 2024 at 11:49:32AM +0000, Ignat Korchagin wrote:
> Hi Pablo,
> 
> On Thu, Feb 22, 2024 at 10:52 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi Ignat,
> >
> > On Thu, Feb 22, 2024 at 10:33:08AM +0000, Ignat Korchagin wrote:
> > > Commit d0009effa886 ("netfilter: nf_tables: validate NFPROTO_* family") added
> > > some validation of NFPROTO_* families in the nft_compat module, but it broke
> > > the ability to use legacy iptables modules in dual-stack nftables.
> > >
> > > While with legacy iptables one had to independently manage IPv4 and IPv6
> > > tables, with nftables it is possible to have dual-stack tables sharing the
> > > rules. Moreover, it was possible to use rules based on legacy iptables
> > > match/target modules in dual-stack nftables.
> > >
> > > As an example, the program from [2] creates an INET dual-stack family table
> > > using an xt_bpf based rule, which looks like the following (the actual output
> > > was generated with a patched nft tool as the current nft tool does not parse
> > > dual stack tables with legacy match rules, so consider it for illustrative
> > > purposes only):
> > >
> > > table inet testfw {
> > >   chain input {
> > >     type filter hook prerouting priority filter; policy accept;
> > >     bytecode counter packets 0 bytes 0 accept
> > >   }
> > > }
> >
> > This nft command does not exist in tree, this does not restores fine
> > with nft -f. It provides a misleading hint to the reader.
> 
> I tried to clarify above that this is for illustrative purposes only -
> just to give context about what we are trying to do, but do let me
> know if you prefer a v4 with this completely removed.

Thanks for clarifying.

> > I am fine with restoring this because you use it, but you have to find
> > a better interface than using nft_compat to achieve this IMO.
> 
> We're actually looking to restore the effort in [1] so some support
> would be appreciated.
>
> > The upstream consensus this far is not to expose nft_compat features
> > through userspace nft. But as said, I understand and I am fine with
> > restoring kernel behaviour so you can keep going with your out-of-tree
> > patch.
> 
> Understood. There is no expectation from us that upstream userspace
> nft should natively support this (as it didn't before d0009effa886),
> but we can send the patch if consensus changes.

Thanks for explaining.