Re: [nf-next PATCH v3 3/3] netfilter: nf_tables: Add locking for NFT_MSG_GETOBJ_RESET requests

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

Cc'ing Florian.

On Wed, Oct 25, 2023 at 11:00:14PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Oct 25, 2023 at 10:08:28PM +0200, Phil Sutter wrote:
> > Objects' dump callbacks are not concurrency-safe per-se with reset bit
> > set. If two CPUs perform a reset at the same time, at least counter and
> > quota objects suffer from value underrun.
> > 
> > Prevent this by introducing dedicated locking callbacks for nfnetlink
> > and the asynchronous dump handling to serialize access.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nf_tables_api.c | 72 ++++++++++++++++++++++++++++-------
> >  1 file changed, 59 insertions(+), 13 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 5f84bdd40c3f..245a2c5be082 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> [...]
> > @@ -7832,16 +7876,18 @@ static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info,
> >  		return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c);
> >  	}
> >  
> > -	if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)
> > -		reset = true;
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -EINVAL;
> 
> For netlink dump path, __netlink_dump_start() already grabs a
> reference module this via c->module.
> 
> Why is this module reference needed for getting one object? This does
> not follow netlink dump path, it creates the skb and it returns
> inmediately.

nfnetlink callbacks use nfnetlink_get_subsys() which use
rcu_dereference() to fetch the nfnetlink_subsystem callbacks. In
nfnetlink_rcv_batch() the ss pointer is fetched at the beginning of
the batch processing.

But then, if rcu_read_unlock() is released, then:

        const struct nfnetlink_subsystem *ss;

could become stale and refetch is needed because rcu read side lock
was released, so next iteration on the skb to process the next
nlmsghdr could be using stale pointers.

Could you please have a second look to confirm this?

Thanks!