diff mbox

[nf-next] netfilter: nf_ct_ext: invoke destroy even when ext is not attached

Message ID 1493474389-9217-1-git-send-email-zlpnobody@163.com
State Accepted
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang April 29, 2017, 1:59 p.m. UTC 
From: Liping Zhang <zlpnobody@gmail.com>

For NF_NAT_MANIP_SRC, we will insert the ct to the nat_bysource_table,
then remove it from the nat_bysource_table via nat_extend->destroy.

But now, the nat extension is attached on demand, so if the nat extension
is not attached, we will not be notified when the ct is destroyed, i.e.
we may fail to remove ct from the nat_bysource_table.

So just keep it simple, even if the extension is not attached, we will
still invoke the related ext->destroy. And this will also preserve the
flexibility for the future extension.

Fixes: 9a08ecfe74d7 ("netfilter: don't attach a nat extension by default")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 include/net/netfilter/nf_conntrack_extend.h | 7 +------
 net/netfilter/nf_conntrack_extend.c         | 8 ++------
 2 files changed, 3 insertions(+), 12 deletions(-)

Comments

Florian Westphal April 29, 2017, 8:03 p.m. UTC | #1 
Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> For NF_NAT_MANIP_SRC, we will insert the ct to the nat_bysource_table,
> then remove it from the nat_bysource_table via nat_extend->destroy.

Right, I forgot about that.

> But now, the nat extension is attached on demand, so if the nat extension
> is not attached, we will not be notified when the ct is destroyed, i.e.
> we may fail to remove ct from the nat_bysource_table.
>
> So just keep it simple, even if the extension is not attached, we will
> still invoke the related ext->destroy. And this will also preserve the
> flexibility for the future extension.

So afaics only helper and nat have destructors and both are safe to be
called if the extension isn't present.

IOW, this looks correct to me, thanks for finding and fixing this.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso May 1, 2017, 9:49 a.m. UTC | #2 
On Sat, Apr 29, 2017 at 09:59:49PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> For NF_NAT_MANIP_SRC, we will insert the ct to the nat_bysource_table,
> then remove it from the nat_bysource_table via nat_extend->destroy.
> 
> But now, the nat extension is attached on demand, so if the nat extension
> is not attached, we will not be notified when the ct is destroyed, i.e.
> we may fail to remove ct from the nat_bysource_table.
> 
> So just keep it simple, even if the extension is not attached, we will
> still invoke the related ext->destroy. And this will also preserve the
> flexibility for the future extension.

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index b01f73f..4944bc9 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -69,12 +69,7 @@  static inline void *__nf_ct_ext_find(const struct nf_conn *ct, u8 id)
 	((id##_TYPE *)__nf_ct_ext_find((ext), (id)))
 
 /* Destroy all relationships */
-void __nf_ct_ext_destroy(struct nf_conn *ct);
-static inline void nf_ct_ext_destroy(struct nf_conn *ct)
-{
-	if (ct->ext)
-		__nf_ct_ext_destroy(ct);
-}
+void nf_ct_ext_destroy(struct nf_conn *ct);
 
 /* Free operation. If you want to free a object referred from private area,
  * please implement __nf_ct_ext_free() and call it.
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index 68ae1be..6c605e8 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -20,16 +20,12 @@  static struct nf_ct_ext_type __rcu *nf_ct_ext_types[NF_CT_EXT_NUM];
 static DEFINE_MUTEX(nf_ct_ext_type_mutex);
 #define NF_CT_EXT_PREALLOC	128u /* conntrack events are on by default */
 
-void __nf_ct_ext_destroy(struct nf_conn *ct)
+void nf_ct_ext_destroy(struct nf_conn *ct)
 {
 	unsigned int i;
 	struct nf_ct_ext_type *t;
-	struct nf_ct_ext *ext = ct->ext;
 
 	for (i = 0; i < NF_CT_EXT_NUM; i++) {
-		if (!__nf_ct_ext_exist(ext, i))
-			continue;
-
 		rcu_read_lock();
 		t = rcu_dereference(nf_ct_ext_types[i]);
 
@@ -42,7 +38,7 @@  void __nf_ct_ext_destroy(struct nf_conn *ct)
 		rcu_read_unlock();
 	}
 }
-EXPORT_SYMBOL(__nf_ct_ext_destroy);
+EXPORT_SYMBOL(nf_ct_ext_destroy);
 
 void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {