Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Sorry, it's wrong. Please, take this.
>>> @@ -1672,9 +1660,24 @@ ctnetlink_create_expect(struct nlattr *c
>>>      help = nfct_help(ct);
>>>  
>>>      if (!help || !help->helper) {
>>> -        /* such conntrack hasn't got any helper, abort */
>>> +#ifdef CONFIG_KMOD
>>> +        char *name;
>>> +
>>>          err = -EINVAL;
>>> +        if (!cda[CTA_EXPECT_HELP_NAME])
>>> +            goto out;
>>> +
>>> +        err = -ENOTSUPP;
>>> +        name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
>>> +        if (request_module("nfct-helper-%s", name) < 0)
>>> +            goto out;
>>> +
>>> +        if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
>>> +            goto out;
>> This strikes me as quite inconsistent. First, we only perform
>> autoloading for expectation creation, but not for conntracks.
> 
> The module autoloading for conntracks is tricky, it's easy to add for
> the creation case, but I don't see a sane way to do this in the update
> case because of the spin lock that we hold most of the time.
> 
> The only idea that comes to my mind is to do the module load-on-demand
> in a very early stage - just after the tuple parsing in the
> new_conntrack function - but then we'll have to do another look up for
> the helper inside the change_helper function - that would make two
> invocations of find_byname() to assign the helper.
> 
> Moreover, someone may remove the module in the middle just after the
> module loading but, well, we have lost the race in the case.

I'd do something similar to qdiscs etc:

- lookup helper
- if not found: request_module, take lock again, repeat lookup, return 
EAGAIN if found now
- in the nfnetlink command handler: if ret == EAGAIN replay message

grep for "replay" in net/ for a few examples of this. This also
handles the race BTW.

>> Second, this implicit helper assignment is also a bit unusual,
>> why don't we simply insist that the conntrack has a helper
>> assigned through the ctnetlink conntrack interface?
> 
> If I understood well, then we simply assign the helper to the conntrack
> and the expectation part of ctnetlink should rely on the existing
> assigned helper, right?

Yes, I think thats cleaner.

> 
> Please, have a look at the patch attached.

Looks fine.