Hi Pablo,
On Thu, Feb 22, 2024 at 10:52 AM Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ [2]
> > Reported-by: Jordan Griege <jgriege@xxxxxxxxxxxxxx>
> > Signed-off-by: Ignat Korchagin <ignat@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
Ignat
