[PATCH 1/2] Fix (improve) deadlock condition on module removal netfilter socket option removal

From: Neil Horman
Date: Tue Sep 04 2007 - 16:29:15 EST

Patch 1/2 to fix netfilter socket option removal

This patch changes netfilter socket options to do reference counting on the
module refcounter (And saves us 4 bytes in the structure to boot :) ).

regards

Neil

Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>


include/linux/netfilter.h | 5 +--
net/bridge/netfilter/ebtables.c | 1
net/ipv4/ipvs/ip_vs_ctl.c | 1
net/ipv4/netfilter/arp_tables.c | 1
net/ipv4/netfilter/ip_tables.c | 1
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 1
net/ipv6/netfilter/ip6_tables.c | 1
net/netfilter/nf_sockopt.c | 36 +++++++------------------
8 files changed, 19 insertions(+), 28 deletions(-)


diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 0eed0b7..1dd075e 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -88,9 +88,8 @@ struct nf_sockopt_ops
int (*compat_get)(struct sock *sk, int optval,
void __user *user, int *len);

- /* Number of users inside set() or get(). */
- unsigned int use;
- struct task_struct *cleanup_task;
+ /* Use the module struct to lock set/get code in place */
+ struct module *owner;
};

/* Each queued (to userspace) skbuff has one of these. */
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 4169a2a..6018d0e 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1513,6 +1513,7 @@ static struct nf_sockopt_ops ebt_sockopts =
.get_optmin = EBT_BASE_CTL,
.get_optmax = EBT_SO_GET_MAX + 1,
.get = do_ebt_get_ctl,
+ .owner = THIS_MODULE,
};

static int __init ebtables_init(void)
diff --git a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
index e1052bc..0234122 100644
--- a/net/ipv4/ipvs/ip_vs_ctl.c
+++ b/net/ipv4/ipvs/ip_vs_ctl.c
@@ -2340,6 +2340,7 @@ static struct nf_sockopt_ops ip_vs_sockopts = {
.get_optmin = IP_VS_BASE_CTL,
.get_optmax = IP_VS_SO_GET_MAX+1,
.get = do_ip_vs_get_ctl,
+ .owner = THIS_MODULE,
};


diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index d1149ab..29114a9 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1161,6 +1161,7 @@ static struct nf_sockopt_ops arpt_sockopts = {
.get_optmin = ARPT_BASE_CTL,
.get_optmax = ARPT_SO_GET_MAX+1,
.get = do_arpt_get_ctl,
+ .owner = THIS_MODULE,
};

static int __init arp_tables_init(void)
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e1b402c..6486894 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -2296,6 +2296,7 @@ static struct nf_sockopt_ops ipt_sockopts = {
#ifdef CONFIG_COMPAT
.compat_get = compat_do_ipt_get_ctl,
#endif
+ .owner = THIS_MODULE,
};

static struct xt_match icmp_matchstruct __read_mostly = {
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 64552af..fd65337 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -403,6 +403,7 @@ static struct nf_sockopt_ops so_getorigdst = {
.get_optmin = SO_ORIGINAL_DST,
.get_optmax = SO_ORIGINAL_DST+1,
.get = &getorigdst,
+ .owner = THIS_MODULE,
};

struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv4 __read_mostly = {
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index aeda617..cd9df02 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1462,6 +1462,7 @@ static struct nf_sockopt_ops ip6t_sockopts = {
.get_optmin = IP6T_BASE_CTL,
.get_optmax = IP6T_SO_GET_MAX+1,
.get = do_ip6t_get_ctl,
+ .owner = THIS_MODULE,
};

static struct xt_match icmp6_matchstruct __read_mostly = {
diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
index 8b8ece7..e32761c 100644
--- a/net/netfilter/nf_sockopt.c
+++ b/net/netfilter/nf_sockopt.c
@@ -55,18 +55,7 @@ EXPORT_SYMBOL(nf_register_sockopt);

void nf_unregister_sockopt(struct nf_sockopt_ops *reg)
{
- /* No point being interruptible: we're probably in cleanup_module() */
- restart:
mutex_lock(&nf_sockopt_mutex);
- if (reg->use != 0) {
- /* To be woken by nf_sockopt call... */
- /* FIXME: Stuart Young's name appears gratuitously. */
- set_current_state(TASK_UNINTERRUPTIBLE);
- reg->cleanup_task = current;
- mutex_unlock(&nf_sockopt_mutex);
- schedule();
- goto restart;
- }
list_del(&reg->list);
mutex_unlock(&nf_sockopt_mutex);
}
@@ -86,10 +75,11 @@ static int nf_sockopt(struct sock *sk, int pf, int val,
list_for_each(i, &nf_sockopts) {
ops = (struct nf_sockopt_ops *)i;
if (ops->pf == pf) {
+ if (!try_module_get(ops->owner))
+ goto out_nosup;
if (get) {
if (val >= ops->get_optmin
&& val < ops->get_optmax) {
- ops->use++;
mutex_unlock(&nf_sockopt_mutex);
ret = ops->get(sk, val, opt, len);
goto out;
@@ -97,23 +87,20 @@ static int nf_sockopt(struct sock *sk, int pf, int val,
} else {
if (val >= ops->set_optmin
&& val < ops->set_optmax) {
- ops->use++;
mutex_unlock(&nf_sockopt_mutex);
ret = ops->set(sk, val, opt, *len);
goto out;
}
}
+ module_put(ops->owner);
}
}
+ out_nosup:
mutex_unlock(&nf_sockopt_mutex);
return -ENOPROTOOPT;

out:
- mutex_lock(&nf_sockopt_mutex);
- ops->use--;
- if (ops->cleanup_task)
- wake_up_process(ops->cleanup_task);
- mutex_unlock(&nf_sockopt_mutex);
+ module_put(ops->owner);
return ret;
}

@@ -144,10 +131,12 @@ static int compat_nf_sockopt(struct sock *sk, int pf, int val,
list_for_each(i, &nf_sockopts) {
ops = (struct nf_sockopt_ops *)i;
if (ops->pf == pf) {
+ if (!try_module_get(ops->owner))
+ goto out_nosup;
+
if (get) {
if (val >= ops->get_optmin
&& val < ops->get_optmax) {
- ops->use++;
mutex_unlock(&nf_sockopt_mutex);
if (ops->compat_get)
ret = ops->compat_get(sk,
@@ -160,7 +149,6 @@ static int compat_nf_sockopt(struct sock *sk, int pf, int val,
} else {
if (val >= ops->set_optmin
&& val < ops->set_optmax) {
- ops->use++;
mutex_unlock(&nf_sockopt_mutex);
if (ops->compat_set)
ret = ops->compat_set(sk,
@@ -171,17 +159,15 @@ static int compat_nf_sockopt(struct sock *sk, int pf, int val,
goto out;
}
}
+ module_put(ops->owner);
}
}
+ out_nosup:
mutex_unlock(&nf_sockopt_mutex);
return -ENOPROTOOPT;

out:
- mutex_lock(&nf_sockopt_mutex);
- ops->use--;
- if (ops->cleanup_task)
- wake_up_process(ops->cleanup_task);
- mutex_unlock(&nf_sockopt_mutex);
+ module_put(ops->owner);
return ret;
}

--
/***************************************************
*Neil Horman
*Software Engineer
*Red Hat, Inc.
*nhorman@xxxxxxxxxxxxx
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***************************************************/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/