Re: [PATCH] [PATCH] remove module dependency between ctnetlink and nf_nat

[Patrick McHardy <kaber@trash.net>]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

Pablo Neira Ayuso wrote:
> This patch removes the module dependency between ctnetlink and
> nf_nat by means of an indirect call that is initialized when
> nf_nat is loaded. Now, nf_conntrack_netlink only requires
> nf_conntrack and nfnetlink.
> 
> This patch puts nfnetlink_parse_nat_setup_hook into the
> nf_conntrack_core to avoid dependencies between ctnetlink,
> nf_conntrack_ipv4 and nf_conntrack_ipv6.
> 
> This patch also introduces the function ctnetlink_change_nat
> that is only invoked from the creation path. Actually, the
> nat handling cannot be invoked from the update path since
> this is not allowed. By introducing this function, we remove
> the useless nat handling in the update path and we avoid
> deadlock-prone code.
> 
> This patch also adds the required EAGAIN logic for nfnetlink.

This looks great, I've applied it with one minor change,
thanks!

> +ctnetlink_parse_nat_setup(struct nf_conn *ct,
> +			  enum nf_nat_manip_type manip,
> +			  struct nlattr *attr)
> +{
> +	typeof(nfnetlink_parse_nat_setup_hook) parse_nat_setup;
> +
> +	parse_nat_setup = rcu_dereference(nfnetlink_parse_nat_setup_hook);
> +	if (!parse_nat_setup) {
> +#ifdef CONFIG_KMOD
> +		rcu_read_unlock();
> +		nfnl_unlock();
> +		if (request_module("nf-nat-ipv4") < 0) {
> +			nfnl_lock();
> +			rcu_read_lock();
> +			return -EOPNOTSUPP;
> +		}
> +		nfnl_lock();
> +		rcu_read_lock();
> +		parse_nat_setup =
> +			rcu_dereference(nfnetlink_parse_nat_setup_hook);
> +		if (parse_nat_setup)
> +			return -EAGAIN;

The rcu_dereference here isn't necessary because the pointer isn't
actually dereferenced. It doesn't matter much, but removing it also
looks slightly nicer :)


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 500 bytes --]

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index b132124..08e82d6 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -832,9 +832,7 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
 		}
 		nfnl_lock();
 		rcu_read_lock();
-		parse_nat_setup =
-			rcu_dereference(nfnetlink_parse_nat_setup_hook);
-		if (parse_nat_setup)
+		if (nfnetlink_parse_nat_setup_hook)
 			return -EAGAIN;
 #endif
 		return -EOPNOTSUPP;