On Wed, Jan 22, 2020 at 11:49:47PM +0100, Pablo Neira Ayuso 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.
>
> Now, on the second path, it will find that this already tried to load
s/second path/second pass
> the module, so it does not add it again, nft_request_module() returns 0.
> 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.
>
> So the code above is just checking for the second pass after one abort
> with autoload parameter set on, not to read it again.
>
> I can include this logic in the patch description in a v3.
>
> > Other than that I like this idea as it avoids the entire "drop
> > transaction mutex while inside a transaction" mess.
>
> request_module() and the transaction logic was not fitting well, after
> this there will be a well-defined location to do this.
>
> 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.
>
> Thanks.
