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
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.
