[PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function
From: Liping Zhang
And convert module_put invocation to nf_conntrack_helper_put, this is
prepared for the later patch, which will add a refcnt for cthelper.
Signed-off-by: Liping Zhang
---
include/net/netfilter/nf_conntrack_helper.h | 2 ++
net/netfilter/nf_conntrack_helper.c | 6 ++
net/netfilter/xt_CT.c | 6 +++---
net/openvswitch/conntrack.c | 4 ++--
4 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_helper.h
b/include/net/netfilter/nf_conntrack_helper.h
index 1eaac1f..65e1dcf 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -58,6 +58,8 @@ struct nf_conntrack_helper *__nf_conntrack_helper_find(const
char *name,
struct nf_conntrack_helper *nf_conntrack_helper_try_module_get(const char
*name,
u16 l3num,
u8 protonum);
+void nf_conntrack_helper_put(struct nf_conntrack_helper *helper);
+
void nf_ct_helper_init(struct nf_conntrack_helper *helper,
u16 l3num, u16 protonum, const char *name,
u16 default_port, u16 spec_port, u32 id,
diff --git a/net/netfilter/nf_conntrack_helper.c
b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..5275e9a 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -172,6 +172,12 @@ nf_conntrack_helper_try_module_get(const char *name, u16
l3num, u8 protonum)
}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
+void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
+{
+ module_put(helper->me);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
+
struct nf_conn_help *
nf_ct_helper_ext_add(struct nf_conn *ct,
struct nf_conntrack_helper *helper, gfp_t gfp)
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 841cfba..2db42ca 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -95,7 +95,7 @@ xt_ct_set_helper(struct nf_conn *ct, const char *helper_name,
help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL);
if (help == NULL) {
- module_put(helper->me);
+ nf_conntrack_helper_put(helper);
return -ENOMEM;
}
@@ -260,7 +260,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par,
err4:
help = nfct_help(ct);
if (help)
- module_put(help->helper->me);
+ nf_conntrack_helper_put(help->helper);
err3:
nf_ct_tmpl_free(ct);
err2:
@@ -343,7 +343,7 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param
*par,
if (ct && !nf_ct_is_untracked(ct)) {
help = nfct_help(ct);
if (help)
- module_put(help->helper->me);
+ nf_conntrack_helper_put(help->helper);
nf_ct_netns_put(par->net, par->family);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 7b2c2fc..fd861d9 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1084,7 +1084,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info
*info, const char *name,
help = nf_ct_helper_ext_add(info->ct, helper, GFP_KERNEL);
if (!help) {
- module_put(helper->me);
+ nf_conntrack_helper_put(helper);
return -ENOMEM;
}
@@ -1534,7 +1534,7 @@ void ovs_ct_free_action(const struct nlattr *a)
static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
{
if (ct_info->helper)
- module_put(ct_info->helper->me);
+ nf_conntrack_helper_put(ct_info->helper);
if (ct_info->ct)
nf_ct_tmpl_free(ct_info->ct);
}
--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH nf 0/2] netfilter: reject cthelper del request if it is in use
From: Liping Zhang User can still delete the cthelper even if it is in use: # nfct helper add ssdp inet udp # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp # nfct helper delete ssdp //--> succeed! This will cause a use-after-free error. So we shoule add a refcnt to fix this issue. Before accomplishing this, it's better to introduce a nf_conntrack_helper_put helper function. Note, this patch set is based on http://patchwork.ozlabs.org/patch/748533/. But I think it may still conflict with other patches. If so, I can rebase it. Liping Zhang (2): netfilter: introduce nf_conntrack_helper_put helper function netfilter: nfnl_cthelper: reject del request if helper obj is in use include/net/netfilter/nf_conntrack_helper.h | 4 net/netfilter/nf_conntrack_helper.c | 12 net/netfilter/nfnetlink_cthelper.c | 17 +++-- net/netfilter/xt_CT.c | 6 +++--- net/openvswitch/conntrack.c | 4 ++-- 5 files changed, 32 insertions(+), 11 deletions(-) -- 2.5.5 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use
From: Liping Zhang
We can still delete the ct helper even if it is in use, this will cause
a use-after-free error. In more detail, I mean:
# nfct helper add ssdp inet udp
# iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp
# nfct helper delete ssdp //--> succeed!
So add reference count to fix this issue, if ct helper is used by
others, reject the delete request.
Apply this patch:
# nfct helper delete ssdp
nfct v1.4.3: netlink error: Device or resource busy
Signed-off-by: Liping Zhang
---
Also note, nft ct helper obj only exists in nf-next tree, so after this
patch appeared in nf-next, I will send another patch to fix it.
include/net/netfilter/nf_conntrack_helper.h | 2 ++
net/netfilter/nf_conntrack_helper.c | 6 ++
net/netfilter/nfnetlink_cthelper.c | 17 +++--
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_helper.h
b/include/net/netfilter/nf_conntrack_helper.h
index 65e1dcf..c7a9ad7 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -9,6 +9,7 @@
#ifndef _NF_CONNTRACK_HELPER_H
#define _NF_CONNTRACK_HELPER_H
+#include
#include
#include
#include
@@ -26,6 +27,7 @@ struct nf_conntrack_helper {
struct hlist_node hnode;/* Internal use. */
char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
+ refcount_t refcnt;
struct module *me; /* pointer to self */
const struct nf_conntrack_expect_policy *expect_policy;
diff --git a/net/netfilter/nf_conntrack_helper.c
b/net/netfilter/nf_conntrack_helper.c
index 5275e9a..8fdd03b 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -167,6 +167,10 @@ nf_conntrack_helper_try_module_get(const char *name, u16
l3num, u8 protonum)
#endif
if (h != NULL && !try_module_get(h->me))
h = NULL;
+ if (h != NULL && !refcount_inc_not_zero(&h->refcnt)) {
+ module_put(h->me);
+ h = NULL;
+ }
return h;
}
@@ -174,6 +178,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
{
+ refcount_dec(&helper->refcnt);
module_put(helper->me);
}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
@@ -398,6 +403,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper
*me)
goto out;
}
}
+ refcount_set(&me->refcnt, 1);
hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
nf_ct_helper_count++;
out:
diff --git a/net/netfilter/nfnetlink_cthelper.c
b/net/netfilter/nfnetlink_cthelper.c
index d455581..6e9e3c4 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -672,6 +672,7 @@ static int nfnl_cthelper_del(struct net *net, struct sock
*nfnl,
tuple_set = true;
}
+ ret = -ENOENT;
list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
cur = &nlcth->helper;
j++;
@@ -685,16 +686,20 @@ static int nfnl_cthelper_del(struct net *net, struct sock
*nfnl,
tuple.dst.protonum != cur->tuple.dst.protonum))
continue;
- found = true;
- nf_conntrack_helper_unregister(cur);
- kfree(cur->expect_policy);
+ if (refcount_dec_if_one(&cur->refcnt)) {
+ found = true;
+ nf_conntrack_helper_unregister(cur);
+ kfree(cur->expect_policy);
- list_del(&nlcth->list);
- kfree(nlcth);
+ list_del(&nlcth->list);
+ kfree(nlcth);
+ } else {
+ ret = -EBUSY;
+ }
}
/* Make sure we return success if we flush and there is no helpers */
- return (found || j == 0) ? 0 : -ENOENT;
+ return (found || j == 0) ? 0 : ret;
}
static const struct nla_policy nfnl_cthelper_policy[NFCTH_MAX+1] = {
--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: netfilter: Replace explicit NULL comparisons
On Sunday 2017-04-09 05:42, Arushi Singhal wrote: >On Sun, Apr 9, 2017 at 1:44 AM, Pablo Neira Ayuso wrote: > On Sat, Apr 08, 2017 at 08:21:56PM +0200, Jan Engelhardt wrote: > > On Saturday 2017-04-08 19:21, Arushi Singhal wrote: > > > > >Replace explicit NULL comparison with ! operator to simplify code. > > > > I still wouldn't do this, for the same reason as before. Comparing to > > NULL explicitly more or less gave an extra guarantee that the other > > operand was also a pointer. > > Arushi, where does it say in the coding style that this is prefered? > >This is reported by checkpatch.pl script. checkpatch has been controversial at times, like when people took the 80 character limit way too literally. Changing pointer comparisons looks like another thing that is better left ignored. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: netfilter: Replace explicit NULL comparisons
2017-04-09 16:26 GMT+08:00 Jan Engelhardt : > > On Sunday 2017-04-09 05:42, Arushi Singhal wrote: >>On Sun, Apr 9, 2017 at 1:44 AM, Pablo Neira Ayuso wrote: >> On Sat, Apr 08, 2017 at 08:21:56PM +0200, Jan Engelhardt wrote: >> > On Saturday 2017-04-08 19:21, Arushi Singhal wrote: >> > >> > >Replace explicit NULL comparison with ! operator to simplify code. >> > >> > I still wouldn't do this, for the same reason as before. Comparing to >> > NULL explicitly more or less gave an extra guarantee that the other >> > operand was also a pointer. >> >> Arushi, where does it say in the coding style that this is prefered? >> >>This is reported by checkpatch.pl script. > > checkpatch has been controversial at times, like when people took the 80 > character limit way too literally. Changing pointer comparisons looks like > another thing that is better left ignored. Yes, I agree too. Converting the "if (p != NULL)" to "if (p)" like this seems unnecessary. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
