Re: [PATCH 3/8] [PATCH] Helper modules load-on-demand support for ctnetlink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pablo Neira Ayuso wrote:
This patch adds module loading for helpers via ctnetlink.

* Creation path: We support explicit and implicit helper assignation. For
  the explicit case, we try to load the module. If the module is correctly
  loaded and the helper is present, we return EAGAIN to re-start the
  creation. Otherwise, we return EOPNOTSUPP.
* Update path: release the spin lock, load the module and check. If it is
  present, then return EAGAIN to re-start the update.

This patch includes a minor change in nfnetlink to support EAGAIN.
Based on a suggestion from Patrick McHardy.

This patch provides a refactorized function to lookup-and-set the
connection tracking helper. The function removes the exported symbol
__nf_ct_helper_find as it has not clients anymore.

This one doesn't apply:

patching file include/net/netfilter/nf_conntrack_helper.h
patching file net/netfilter/nf_conntrack_core.c
Hunk #1 FAILED at 581.
Hunk #2 succeeded at 753 (offset -5 lines).
Hunk #3 succeeded at 764 (offset -5 lines).
1 out of 3 hunks FAILED -- saving rejects to file net/netfilter/nf_conntrack_core.c.rej
patching file net/netfilter/nf_conntrack_helper.c
patching file net/netfilter/nf_conntrack_netlink.c
Hunk #2 succeeded at 1131 with fuzz 1.
patching file net/netfilter/nfnetlink.c

+static inline int nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = __nf_ct_set_helper(ct, flags);
+	rcu_read_unlock();

Its a bit confusing to use rcu_read_lock for a function called
_set_something, I would suggest to just do the rcu_read_lock
in that function itself.

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index f38c649..a649f7b 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -952,8 +952,22 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
 	}
helper = __nf_conntrack_helper_find_byname(helpname);
-	if (helper == NULL)
+	if (helper == NULL) {
+#ifdef CONFIG_KMOD
+		spin_unlock_bh(&nf_conntrack_lock);
+
+		if (request_module("nfct-helper-%s", helpname) < 0) {
+			spin_lock_bh(&nf_conntrack_lock);
+			return -EOPNOTSUPP;

EOPNOTSUPP doesn't seem wrong, but we usually use ENOENT for lookup
failures. It also needs to drop the nfnl_lock for request_module().

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index b75c9c4..323dcf8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -165,7 +165,8 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		} else
 			return -EINVAL;
- return nc->call(nfnl, skb, nlh, cda);
+		while ((err = nc->call(nfnl, skb, nlh, cda)) == -EAGAIN);

Ughh no hiding of ";" please :) With dropping nfnl_lock() in ctnetlink,
this needs to do the get_subsys() and find_client() again anyways since
both are protected by that lock.

I have this patch in my nftables tree:

@@ -132,6 +140,7 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                return 0;

        type = nlh->nlmsg_type;
+replay:
        ss = nfnetlink_get_subsys(type);
        if (!ss) {
 #ifdef CONFIG_KMOD
@@ -165,7 +174,10 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                } else
                        return -EINVAL;

-               return nc->call(nfnl, skb, nlh, cda);
+               err = nc->call(nfnl, skb, nlh, cda);
+               if (err == -EAGAIN)
+                       goto replay;
+               return err;
        }
 }

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

[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux