Re: [PATCH v2 nf-next 0/7] netfilter: nf_tables: avoid PROVE_RCU_LIST splats

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

On Fri, Nov 01, 2024 at 12:37:42AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Nov 01, 2024 at 12:02:14AM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > # nft -f test.nft
> > > > test.nft:3:32-45: Error: Could not process rule: Operation not supported
> > > >                 udp dport 4789 vxlan ip saddr 1.2.3.4
> > > >                                ^^^^^^^^^^^^^^
> > > >g
> > > > Reverting "netfilter: nf_tables: must hold rcu read lock while iterating expression type list"
> > > > makes it work for me again.
> > > >g
> > > > Are you compiling nf_tables built-in there? I make as a module, the
> > > > type->owner is THIS_MODULE which refers to nf_tables.ko?
> > >g
> > > Indeed, this doesn't work.
> > >g
> > > But I cannot remove this test, this code looks broken to me in case
> > > inner type is its own module.
> > >g
> > > No idea yet how to fix this.
> >g
> > I'm missing why this check is required by now.
> >g
> > Only meta and payload provide inner_ops and they are always built-in.
> >g
> > I understand this is an issue if more expressions are supported in the
> > future.
>g
> Can you mangle the patch to remove the type->owner test and amend
> the comment to say that this restriction exists (inner_ops != NULL ->
> builtin?)
>g
> Else this might work:
>g
> +       if (!type->inner_ops || type->owner != THIS_MODULE) {
>g
> ... but that would also need a comment, I think :-/

IUC, your concern is future extensibility.

To support for extensions other than meta and payload, this code will
need to be updated anyway, because __nft_expr_type_get() is used and
that cannot autoload modules:

        type = __nft_expr_type_get(ctx->family, tb[NFTA_EXPR_NAME]);
        if (!type)g
                return -ENOENT;

        if (!type->inner_ops)
                return -EOPNOTSUPP;

I think removing it by now is just fine.

If you prefer the defensive approach, the an explicit check for meta
and payload ops is perfectly fine, but I don't think that is needed.

There is another issue that is spotted by smack which needs to be
addressed in this series anyway, this needs a v3.

Thanks Florian.