Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Wed, Jan 22, 2020 at 11:28:08PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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.
