Re: nfnetlink: Busy-loop in nfnetlink_rcv_msg()

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

Hi Florian,

Sorry, I overlook your reply.

On Sat, Aug 22, 2020 at 08:46:21PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Starting firewalld with two active zones in an lxc container provokes a
> > situation in which nfnetlink_rcv_msg() loops indefinitely, because
> > nc->call_rcu() (nf_tables_getgen() in this case) returns -EAGAIN every
> > time.
> > 
> > I identified netlink_attachskb() as the originator for the above error
> > code. The conditional leading to it looks like this:
> > 
> > | if ((atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf ||
> > |      test_bit(NETLINK_S_CONGESTED, &nlk->state))) {
> > |         [...]
> > |         if (!*timeo) {
> > 
> > *timeo is zero, so this seems to be a non-blocking socket. Both
> > NETLINK_S_CONGESTED bit is set and sk->sk_rmem_alloc exceeds
> > sk->sk_rcvbuf.
> > 
> > From user space side, firewalld seems to simply call sendto() and the
> > call never returns.
> > 
> > How to solve that? I tried to find other code which does the same, but I
> > haven't found one that does any looping. Should nfnetlink_rcv_msg()
> > maybe just return -EAGAIN to the caller if it comes from call_rcu
> > backend?
> 
> Yes, I think thats the most straightforward solution.
> 
> We can of course also intercept -EAGAIN in nf_tables_api.c and translate
> it to -ENOBUFS like in nft_get_set_elem().
> 
> But I think a generic solution it better.  The call_rcu backends should
> not result in changes to nf_tables internal state so they do not load
> modules and therefore don't need a restart.

Handling this from the core would be better, so people don't have to
remember to use the nfnetlink_unicast() that I'm proposing.

Looking at the tree, call_rcu is not enough to assume this: there are
several nfnetlink subsystems calling netlink_unicast() that translate
EAGAIN to ENOBUFS, from .call and .call_rcu. The way to identify this
would be to decorate callbacks to know what are specifically GET
commands.

So either do this or just extend my patch to use nfnetlink_send()
everywhere to remove all existing translations from EAGAIN to ENOBUFS.