Re: [PATCH nf,v2] netfilter: nf_tables: autoload modules from the abort path

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jan 22, 2020 at 11:28:08PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +	list_for_each_entry(req, &net->nft.module_list, list) {
> > > +		if (!strcmp(req->module, module_name) && req->done)
> > > +			return 0;
> > > +	}
> > 
> > If the module is already on this list, why does it need to be
> > added a second time?
> 
> The first time this finds no module on the list, then the module is
> added to the list and nft_request_module() returns -EAGAIN. This
> triggers abort path with autoload parameter set to true from
> nfnetlink, this sets the module done field to true.

I guess I was confused by the need for the "&& req->done" part.

AFAIU req->done is always true here.

> Now, on the second path, it will find that this already tried to load
> the module, so it does not add it again, nft_request_module() returns 0.

But the "I already tried this" is already implied by the presence of the
module name?  Or did I misunderstand?

> Then, there is a look up to find the object that was missing. If
> module was successfully load, the object will be in place, otherwise
> -ENOENT is reported to userspace.

Good, that will prevent infite retries in case userspace requests
non-existent module.

> I can include this logic in the patch description in a v3.

That would be good, thanks!

> I run the syzbot reproducer for 1 hour and no problems, not sure how
> much I have to run it more. I guess the more time the better.

It triggers instantly for me provided:
1. CONFIG_MODULES=y (with MODULES=n the faulty code part isn't built...)
2. set "sysctl kernel.modprobe=/root/sleep1.sh"
   I found that with normal modprobe the race window is rather small and
   the thread doing the request_module has a decent chance of re-locking
   the mutex before another syzkaller thread has a chance to alter the
   current generation.