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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
