[PATCH 0/2] rework of userspace expectation support

[PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

Hi Patrick,

The following patches rework the userspace expectation support
to fix one problematic scenario: if the master conntrack vanishes
while there are still userspace expectations, we hit an oops
in the destroy event path for expectations.

The idea to fix this is to extend the iptables CT target to
explicit allocate the helper extension for conntracks that
are suppose to behave as master for user-space expectations.

In the case of the userspace FTP helper, people would need
to add the following rule:

iptables -A PREROUTING -t raw \
	-p tcp --dport 21 -j CT --userspace-helper

Thus, we can store the list of expectations that belong to
one master, and delete them in case that the master vanishes.

---

Pablo Neira Ayuso (2):
      netfilter: CT: allow to set userspace helper status flag
      netfilter: nf_ct_expect: rework userspace expectation support


 include/linux/netfilter/nf_conntrack_common.h |    4 ++
 include/linux/netfilter/xt_CT.h               |    3 +
 include/net/netfilter/nf_conntrack_expect.h   |    1 
 net/netfilter/nf_conntrack_expect.c           |   63 ++++++++-----------------
 net/netfilter/nf_conntrack_helper.c           |   12 +++++
 net/netfilter/nf_conntrack_netlink.c          |    5 ++
 net/netfilter/xt_CT.c                         |    8 ++-
 7 files changed, 48 insertions(+), 48 deletions(-)

[PATCH 1/2] netfilter: CT: allow to set userspace helper status flag

[Pablo Neira Ayuso]

This patch is new feature and bug fix at the same time.

With this patch, the CT target allows to set the conntrack's
userspace helper status flags. This flag is used to tell the
conntrack system to explicitly allocate the helper extension.

This helper extension is useful to link the userspace expectations
with the master conntrack that is being tracked from one userspace
helper.

This feature fixes a problem in the current approach of the
userspace helper support. Basically, if the master conntrack that
has got a userspace expectation vanishes, the expectations point to
one invalid memory address. Thus, triggering an oops in the
expectation deletion event path.

I decided not to add a new revision of the CT target because
I only needed to add a new flag for it. I'll document in this
issue in the iptables manpages. I have also changed the return
value from EINVAL to EOPNOTSUPP if one flag not supported is
specified. Thus, in the future adding new features that only
require a new flag can be added without a new revision.

Reported-by: Sam Roberts <vieuxtech@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/nf_conntrack_common.h |    4 ++++
 include/linux/netfilter/xt_CT.h               |    3 ++-
 net/netfilter/nf_conntrack_helper.c           |   12 ++++++++++++
 net/netfilter/xt_CT.c                         |    8 +++++---
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/netfilter/nf_conntrack_common.h b/include/linux/netfilter/nf_conntrack_common.h
index 50cdc25..6097edd 100644
--- a/include/linux/netfilter/nf_conntrack_common.h
+++ b/include/linux/netfilter/nf_conntrack_common.h
@@ -80,6 +80,10 @@ enum ip_conntrack_status {
 	/* Conntrack is a fake untracked entry */
 	IPS_UNTRACKED_BIT = 12,
 	IPS_UNTRACKED = (1 << IPS_UNTRACKED_BIT),
+
+	/* Conntrack has a userspace helper. */
+	IPS_USERSPACE_HELPER_BIT = 13,
+	IPS_USERSPACE_HELPER = (1 << IPS_USERSPACE_HELPER_BIT),
 };
 
 /* Connection tracking event types */
diff --git a/include/linux/netfilter/xt_CT.h b/include/linux/netfilter/xt_CT.h
index b56e768..6390f09 100644
--- a/include/linux/netfilter/xt_CT.h
+++ b/include/linux/netfilter/xt_CT.h
@@ -3,7 +3,8 @@
 
 #include <linux/types.h>
 
-#define XT_CT_NOTRACK	0x1
+#define XT_CT_NOTRACK		0x1
+#define XT_CT_USERSPACE_HELPER	0x2
 
 struct xt_ct_target_info {
 	__u16 flags;
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 1bdfea3..4d90102 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -121,6 +121,18 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	int ret = 0;
 
 	if (tmpl != NULL) {
+		/* we've got a userspace helper. */
+		if (tmpl->status & IPS_USERSPACE_HELPER) {
+			help = nf_ct_helper_ext_add(ct, flags);
+			if (help == NULL) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			rcu_assign_pointer(help->helper, NULL);
+			__set_bit(IPS_USERSPACE_HELPER_BIT, &ct->status);
+			ret = 0;
+			goto out;
+		}
 		help = nfct_help(tmpl);
 		if (help != NULL)
 			helper = help->helper;
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 782e519..5f6279b 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -62,8 +62,8 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par)
 	int ret = 0;
 	u8 proto;
 
-	if (info->flags & ~XT_CT_NOTRACK)
-		return -EINVAL;
+	if (info->flags & ~(XT_CT_NOTRACK | XT_CT_USERSPACE_HELPER))
+		return -EOPNOTSUPP;
 
 	if (info->flags & XT_CT_NOTRACK) {
 		ct = nf_ct_untracked_get();
@@ -92,7 +92,9 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par)
 				  GFP_KERNEL))
 		goto err3;
 
-	if (info->helper[0]) {
+	if (info->flags & XT_CT_USERSPACE_HELPER) {
+		__set_bit(IPS_USERSPACE_HELPER_BIT, &ct->status);
+	} else if (info->helper[0]) {
 		ret = -ENOENT;
 		proto = xt_ct_find_proto(par);
 		if (!proto)

[PATCH 2/2] netfilter: nf_ct_expect: rework userspace expectation support

[Pablo Neira Ayuso]

This partially reworks bc01befdcf3e40979eb518085a075cbf0aacede0
which added userspace expectation support.

This patch removes the nf_ct_userspace_expect_list since now we
force to use the new iptables CT target feature to add the helper
extension for conntracks that have attached expectations from
userspace.

A new version of the proof-of-concept code to implement userspace
helpers from userspace is available at:

http://people.netfilter.org/pablo/userspace-conntrack-helpers/nf-ftp-helper-POC.tar.bz2

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_expect.h |    1 
 net/netfilter/nf_conntrack_expect.c         |   63 +++++++++------------------
 net/netfilter/nf_conntrack_netlink.c        |    5 ++
 3 files changed, 25 insertions(+), 44 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 0f8a8c5..4619caa 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -91,7 +91,6 @@ static inline void nf_ct_unlink_expect(struct nf_conntrack_expect *exp)
 
 void nf_ct_remove_expectations(struct nf_conn *ct);
 void nf_ct_unexpect_related(struct nf_conntrack_expect *exp);
-void nf_ct_remove_userspace_expectations(void);
 
 /* Allocate space for an expectation: this is mandatory before calling
    nf_ct_expect_related.  You will have to call put afterwards. */
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index cd1e8e0..73670e9 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -36,8 +36,6 @@ unsigned int nf_ct_expect_max __read_mostly;
 
 static struct kmem_cache *nf_ct_expect_cachep __read_mostly;
 
-static HLIST_HEAD(nf_ct_userspace_expect_list);
-
 /* nf_conntrack_expect helper functions */
 void nf_ct_unlink_expect_report(struct nf_conntrack_expect *exp,
 				u32 pid, int report)
@@ -45,14 +43,14 @@ void nf_ct_unlink_expect_report(struct nf_conntrack_expect *exp,
 	struct nf_conn_help *master_help = nfct_help(exp->master);
 	struct net *net = nf_ct_exp_net(exp);
 
+	NF_CT_ASSERT(master_help);
 	NF_CT_ASSERT(!timer_pending(&exp->timeout));
 
 	hlist_del_rcu(&exp->hnode);
 	net->ct.expect_count--;
 
 	hlist_del(&exp->lnode);
-	if (!(exp->flags & NF_CT_EXPECT_USERSPACE))
-		master_help->expecting[exp->class]--;
+	master_help->expecting[exp->class]--;
 
 	nf_ct_expect_event_report(IPEXP_DESTROY, exp, pid, report);
 	nf_ct_expect_put(exp);
@@ -312,37 +310,34 @@ void nf_ct_expect_put(struct nf_conntrack_expect *exp)
 }
 EXPORT_SYMBOL_GPL(nf_ct_expect_put);
 
-static void nf_ct_expect_insert(struct nf_conntrack_expect *exp)
+static int nf_ct_expect_insert(struct nf_conntrack_expect *exp)
 {
 	struct nf_conn_help *master_help = nfct_help(exp->master);
+	struct nf_conntrack_helper *helper;
 	struct net *net = nf_ct_exp_net(exp);
-	const struct nf_conntrack_expect_policy *p;
 	unsigned int h = nf_ct_expect_dst_hash(&exp->tuple);
 
 	/* two references : one for hash insert, one for the timer */
 	atomic_add(2, &exp->use);
 
-	if (master_help) {
-		hlist_add_head(&exp->lnode, &master_help->expectations);
-		master_help->expecting[exp->class]++;
-	} else if (exp->flags & NF_CT_EXPECT_USERSPACE)
-		hlist_add_head(&exp->lnode, &nf_ct_userspace_expect_list);
+	hlist_add_head(&exp->lnode, &master_help->expectations);
+	master_help->expecting[exp->class]++;
 
 	hlist_add_head_rcu(&exp->hnode, &net->ct.expect_hash[h]);
 	net->ct.expect_count++;
 
 	setup_timer(&exp->timeout, nf_ct_expectation_timed_out,
 		    (unsigned long)exp);
-	if (master_help) {
-		p = &rcu_dereference_protected(
-				master_help->helper,
-				lockdep_is_held(&nf_conntrack_lock)
-				)->expect_policy[exp->class];
-		exp->timeout.expires = jiffies + p->timeout * HZ;
+	helper = rcu_dereference_protected(master_help->helper,
+					   lockdep_is_held(&nf_conntrack_lock));
+	if (helper) {
+		exp->timeout.expires = jiffies +
+			helper->expect_policy[exp->class].timeout * HZ;
 	}
 	add_timer(&exp->timeout);
 
 	NF_CT_STAT_INC(net, expect_create);
+	return 0;
 }
 
 /* Race with expectations being used means we could have none to find; OK. */
@@ -387,14 +382,13 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 	struct nf_conntrack_expect *i;
 	struct nf_conn *master = expect->master;
 	struct nf_conn_help *master_help = nfct_help(master);
+	struct nf_conntrack_helper *helper;
 	struct net *net = nf_ct_exp_net(expect);
 	struct hlist_node *n;
 	unsigned int h;
 	int ret = 1;
 
-	/* Don't allow expectations created from kernel-space with no helper */
-	if (!(expect->flags & NF_CT_EXPECT_USERSPACE) &&
-	    (!master_help || (master_help && !master_help->helper))) {
+	if (!master_help) {
 		ret = -ESHUTDOWN;
 		goto out;
 	}
@@ -412,11 +406,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 		}
 	}
 	/* Will be over limit? */
-	if (master_help) {
-		p = &rcu_dereference_protected(
-			master_help->helper,
-			lockdep_is_held(&nf_conntrack_lock)
-			)->expect_policy[expect->class];
+	helper = rcu_dereference_protected(master_help->helper,
+					   lockdep_is_held(&nf_conntrack_lock));
+	if (helper) {
+		p = &helper->expect_policy[expect->class];
 		if (p->max_expected &&
 		    master_help->expecting[expect->class] >= p->max_expected) {
 			evict_oldest_expect(master, expect);
@@ -448,8 +441,9 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
 	if (ret <= 0)
 		goto out;
 
-	ret = 0;
-	nf_ct_expect_insert(expect);
+	ret = nf_ct_expect_insert(expect);
+	if (ret < 0)
+		goto out;
 	spin_unlock_bh(&nf_conntrack_lock);
 	nf_ct_expect_event_report(IPEXP_NEW, expect, pid, report);
 	return ret;
@@ -459,21 +453,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(nf_ct_expect_related_report);
 
-void nf_ct_remove_userspace_expectations(void)
-{
-	struct nf_conntrack_expect *exp;
-	struct hlist_node *n, *next;
-
-	hlist_for_each_entry_safe(exp, n, next,
-				  &nf_ct_userspace_expect_list, lnode) {
-		if (del_timer(&exp->timeout)) {
-			nf_ct_unlink_expect(exp);
-			nf_ct_expect_put(exp);
-		}
-	}
-}
-EXPORT_SYMBOL_GPL(nf_ct_remove_userspace_expectations);
-
 #ifdef CONFIG_PROC_FS
 struct ct_expect_iter_state {
 	struct seq_net_private p;
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 30bf8a1..7ab2227 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2019,6 +2019,10 @@ ctnetlink_create_expect(struct net *net, u16 zone,
 	}
 	help = nfct_help(ct);
 	if (!help) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+	if (test_bit(IPS_USERSPACE_HELPER_BIT, &ct->status)) {
 		if (!cda[CTA_EXPECT_TIMEOUT]) {
 			err = -EINVAL;
 			goto out;
@@ -2208,7 +2212,6 @@ static void __exit ctnetlink_exit(void)
 {
 	pr_info("ctnetlink: unregistering from nfnetlink.\n");
 
-	nf_ct_remove_userspace_expectations();
 #ifdef CONFIG_NF_CONNTRACK_EVENTS
 	nf_ct_expect_unregister_notifier(&ctnl_notifier_exp);
 	nf_conntrack_unregister_notifier(&ctnl_notifier);

Re: [PATCH 0/2] rework of userspace expectation support

[Patrick McHardy]

Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
> Hi Patrick,
> 
> The following patches rework the userspace expectation support
> to fix one problematic scenario: if the master conntrack vanishes
> while there are still userspace expectations, we hit an oops
> in the destroy event path for expectations.

Just wondering, how can this happen? We take a reference for
userspace expectations just as we do for kernel expectations.

Ok, I see, we are releasing it again at the end of
ctnetlink_create_expect(), that seems to be the actual problem
if I'm not mistaken.

> 
> The idea to fix this is to extend the iptables CT target to
> explicit allocate the helper extension for conntracks that
> are suppose to behave as master for user-space expectations.
> 
> In the case of the userspace FTP helper, people would need
> to add the following rule:
> 
> iptables -A PREROUTING -t raw \
> 	-p tcp --dport 21 -j CT --userspace-helper
> 
> Thus, we can store the list of expectations that belong to
> one master, and delete them in case that the master vanishes.

Re: [PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

On 13/04/11 13:37, Patrick McHardy wrote:
> Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
>> Hi Patrick,
>>
>> The following patches rework the userspace expectation support
>> to fix one problematic scenario: if the master conntrack vanishes
>> while there are still userspace expectations, we hit an oops
>> in the destroy event path for expectations.
> 
> Just wondering, how can this happen? We take a reference for
> userspace expectations just as we do for kernel expectations.
> 
> Ok, I see, we are releasing it again at the end of
> ctnetlink_create_expect(), that seems to be the actual problem
> if I'm not mistaken.

Indeed, we have keep that reference, that would fix the problem.

Still, with the curent approach the userspace expectation will be valid
after the master conntrack has expired.

So we can do the following: Fix this refcount issue in -stable and
current, and schedule these patches for nf-next to change the behaviour.

What do you think?

Re: [PATCH 0/2] rework of userspace expectation support

[Patrick McHardy]

Am 13.04.2011 13:47, schrieb Pablo Neira Ayuso:
> On 13/04/11 13:37, Patrick McHardy wrote:
>> Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
>>> Hi Patrick,
>>>
>>> The following patches rework the userspace expectation support
>>> to fix one problematic scenario: if the master conntrack vanishes
>>> while there are still userspace expectations, we hit an oops
>>> in the destroy event path for expectations.
>>
>> Just wondering, how can this happen? We take a reference for
>> userspace expectations just as we do for kernel expectations.
>>
>> Ok, I see, we are releasing it again at the end of
>> ctnetlink_create_expect(), that seems to be the actual problem
>> if I'm not mistaken.
> 
> Indeed, we have keep that reference, that would fix the problem.

We definitely need to hold it anyways since destroy_conntrack()
releases it again.

> Still, with the curent approach the userspace expectation will be valid
> after the master conntrack has expired.
> 
> So we can do the following: Fix this refcount issue in -stable and
> current, and schedule these patches for nf-next to change the behaviour.
> 
> What do you think?

I don't know, what's the difference to non-userspace expectations?
The same applies to them, we don't require the master to still be
active for expectations.

Re: [PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

On 13/04/11 13:55, Patrick McHardy wrote:
> Am 13.04.2011 13:47, schrieb Pablo Neira Ayuso:
>> On 13/04/11 13:37, Patrick McHardy wrote:
>>> Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
>>>> Hi Patrick,
>>>>
>>>> The following patches rework the userspace expectation support
>>>> to fix one problematic scenario: if the master conntrack vanishes
>>>> while there are still userspace expectations, we hit an oops
>>>> in the destroy event path for expectations.
>>>
>>> Just wondering, how can this happen? We take a reference for
>>> userspace expectations just as we do for kernel expectations.
>>>
>>> Ok, I see, we are releasing it again at the end of
>>> ctnetlink_create_expect(), that seems to be the actual problem
>>> if I'm not mistaken.
>>
>> Indeed, we have keep that reference, that would fix the problem.
> 
> We definitely need to hold it anyways since destroy_conntrack()
> releases it again.
> 
>> Still, with the curent approach the userspace expectation will be valid
>> after the master conntrack has expired.
>>
>> So we can do the following: Fix this refcount issue in -stable and
>> current, and schedule these patches for nf-next to change the behaviour.
>>
>> What do you think?
> 
> I don't know, what's the difference to non-userspace expectations?
> The same applies to them, we don't require the master to still be
> active for expectations.

kernelspace expectations are released if the master is destroyed.
userspace expectations will not.

The helper extension is used to store a list of expectations for this
master conntrack, so we can release the expectations that are
associated. This is not true for userspace expectations, since the
master has no list with expectations.

Re: [PATCH 0/2] rework of userspace expectation support

[Patrick McHardy]

Am 13.04.2011 14:11, schrieb Pablo Neira Ayuso:
> On 13/04/11 13:55, Patrick McHardy wrote:
>> Am 13.04.2011 13:47, schrieb Pablo Neira Ayuso:
>>> On 13/04/11 13:37, Patrick McHardy wrote:
>>>> Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
>>>>> Hi Patrick,
>>>>>
>>>>> The following patches rework the userspace expectation support
>>>>> to fix one problematic scenario: if the master conntrack vanishes
>>>>> while there are still userspace expectations, we hit an oops
>>>>> in the destroy event path for expectations.
>>>>
>>>> Just wondering, how can this happen? We take a reference for
>>>> userspace expectations just as we do for kernel expectations.
>>>>
>>>> Ok, I see, we are releasing it again at the end of
>>>> ctnetlink_create_expect(), that seems to be the actual problem
>>>> if I'm not mistaken.
>>>
>>> Indeed, we have keep that reference, that would fix the problem.
>>
>> We definitely need to hold it anyways since destroy_conntrack()
>> releases it again.
>>
>>> Still, with the curent approach the userspace expectation will be valid
>>> after the master conntrack has expired.
>>>
>>> So we can do the following: Fix this refcount issue in -stable and
>>> current, and schedule these patches for nf-next to change the behaviour.
>>>
>>> What do you think?
>>
>> I don't know, what's the difference to non-userspace expectations?
>> The same applies to them, we don't require the master to still be
>> active for expectations.
> 
> kernelspace expectations are released if the master is destroyed.
> userspace expectations will not.

I see, you're talking about unfulfilled expectations. Still, we're
releasing expectations in destroy_conntrack(), if we fix the refcount
issue, the master conntrack will not be destroyed while userspace
expectations exist.

> The helper extension is used to store a list of expectations for this
> master conntrack, so we can release the expectations that are
> associated. This is not true for userspace expectations, since the
> master has no list with expectations.

This raises the question - why are we treating userspace differently
in this regard?

Re: [PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

On 13/04/11 14:28, Patrick McHardy wrote:
> Am 13.04.2011 14:11, schrieb Pablo Neira Ayuso:
>> On 13/04/11 13:55, Patrick McHardy wrote:
>>> Am 13.04.2011 13:47, schrieb Pablo Neira Ayuso:
>>>> On 13/04/11 13:37, Patrick McHardy wrote:
>>>>> Am 12.04.2011 23:59, schrieb Pablo Neira Ayuso:
>>>>>> Hi Patrick,
>>>>>>
>>>>>> The following patches rework the userspace expectation support
>>>>>> to fix one problematic scenario: if the master conntrack vanishes
>>>>>> while there are still userspace expectations, we hit an oops
>>>>>> in the destroy event path for expectations.
>>>>>
>>>>> Just wondering, how can this happen? We take a reference for
>>>>> userspace expectations just as we do for kernel expectations.
>>>>>
>>>>> Ok, I see, we are releasing it again at the end of
>>>>> ctnetlink_create_expect(), that seems to be the actual problem
>>>>> if I'm not mistaken.
>>>>
>>>> Indeed, we have keep that reference, that would fix the problem.
>>>
>>> We definitely need to hold it anyways since destroy_conntrack()
>>> releases it again.
>>>
>>>> Still, with the curent approach the userspace expectation will be valid
>>>> after the master conntrack has expired.
>>>>
>>>> So we can do the following: Fix this refcount issue in -stable and
>>>> current, and schedule these patches for nf-next to change the behaviour.
>>>>
>>>> What do you think?
>>>
>>> I don't know, what's the difference to non-userspace expectations?
>>> The same applies to them, we don't require the master to still be
>>> active for expectations.
>>
>> kernelspace expectations are released if the master is destroyed.
>> userspace expectations will not.
> 
> I see, you're talking about unfulfilled expectations. Still, we're
> releasing expectations in destroy_conntrack(), if we fix the refcount
> issue, the master conntrack will not be destroyed while userspace
> expectations exist.

Exactly.

>> The helper extension is used to store a list of expectations for this
>> master conntrack, so we can release the expectations that are
>> associated. This is not true for userspace expectations, since the
>> master has no list with expectations.
> 
> This raises the question - why are we treating userspace differently
> in this regard?

I think we should treat them link the kernelspace expectations.

I'll send a patch to fix this issue, and then tell me if you're OK with
this approach so that we can apply it to nf-next.

Re: [PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

On 13/04/11 14:28, Patrick McHardy wrote:
> I see, you're talking about unfulfilled expectations. Still, we're
> releasing expectations in destroy_conntrack(), if we fix the refcount
> issue, the master conntrack will not be destroyed while userspace
> expectations exist.

I missed one point. We don't seem to increase the refcnt for unfulfilled
expectations.

The following example shows a unfulfilled FTP expectation:

# conntrack -L exp
262 proto=6 src=192.168.1.128 dst=130.89.149.226 sport=0 dport=20412
conntrack v1.0.0 (conntrack-tools): 1 expectations have been shown.

# conntrack -L
tcp      6 431979 ESTABLISHED src=192.168.1.128 dst=130.89.149.226
sport=37348 dport=21 packets=9 bytes=503 src=130.89.149.226
dst=192.168.1.128 sport=21 dport=37348 packets=8 bytes=564 [ASSURED]
mark=0 delta-time=29 use=1

The use field of the master conntrack shows 1.

So, I think that these patches are still the way to fix the problem with
the expectations created through ctnetlink.

Re: [PATCH 0/2] rework of userspace expectation support

[Patrick McHardy]

Am 20.04.2011 14:10, schrieb Pablo Neira Ayuso:
> On 13/04/11 14:28, Patrick McHardy wrote:
>> I see, you're talking about unfulfilled expectations. Still, we're
>> releasing expectations in destroy_conntrack(), if we fix the refcount
>> issue, the master conntrack will not be destroyed while userspace
>> expectations exist.
> 
> I missed one point. We don't seem to increase the refcnt for unfulfilled
> expectations.

Yes, that's not necessary since unfulfilled expectations are cleaned
up once the master is destroyed. Its just the ct->master pointer of
an expected connection that takes a reference.

I thought this is what your patches try to fix?

> The following example shows a unfulfilled FTP expectation:
> 
> # conntrack -L exp
> 262 proto=6 src=192.168.1.128 dst=130.89.149.226 sport=0 dport=20412
> conntrack v1.0.0 (conntrack-tools): 1 expectations have been shown.
> 
> # conntrack -L
> tcp      6 431979 ESTABLISHED src=192.168.1.128 dst=130.89.149.226
> sport=37348 dport=21 packets=9 bytes=503 src=130.89.149.226
> dst=192.168.1.128 sport=21 dport=37348 packets=8 bytes=564 [ASSURED]
> mark=0 delta-time=29 use=1
> 
> The use field of the master conntrack shows 1.
> 
> So, I think that these patches are still the way to fix the problem with
> the expectations created through ctnetlink.

Just for my understanding, why not simply take the reference for
the master conntrack for "fulfilled" expectations (IOW conntracks
that have a master assigned), which is necessary anyways since
destroy_conntrack() attempts to release that reference again, and
clean up userspace expectations once the master conntrack vanishes?

This would match what we do for kernel expectations and I don't
really see why we would want to keep an unfulfilled userspace
expectation without the corresponding master.

Re: [PATCH 0/2] rework of userspace expectation support

[Pablo Neira Ayuso]

On 20/04/11 16:06, Patrick McHardy wrote:
> Am 20.04.2011 14:10, schrieb Pablo Neira Ayuso:
>> I missed one point. We don't seem to increase the refcnt for unfulfilled
>> expectations.
> 
> Yes, that's not necessary since unfulfilled expectations are cleaned
> up once the master is destroyed. Its just the ct->master pointer of
> an expected connection that takes a reference.
> 
> I thought this is what your patches try to fix?

Yes, my patches add the helper extension to the conntrack by means of
the CT target. Thus, in the case of userspace helpers, the master
conntrack can store the list of unfulfilled expectations that it owns.

Sorry, I misunderstood your previous email, I thought you were telling
that we bump master's refcnt for unfulfilled expectation, you were
telling the opposite :-).

>> The following example shows a unfulfilled FTP expectation:
>>
>> # conntrack -L exp
>> 262 proto=6 src=192.168.1.128 dst=130.89.149.226 sport=0 dport=20412
>> conntrack v1.0.0 (conntrack-tools): 1 expectations have been shown.
>>
>> # conntrack -L
>> tcp      6 431979 ESTABLISHED src=192.168.1.128 dst=130.89.149.226
>> sport=37348 dport=21 packets=9 bytes=503 src=130.89.149.226
>> dst=192.168.1.128 sport=21 dport=37348 packets=8 bytes=564 [ASSURED]
>> mark=0 delta-time=29 use=1
>>
>> The use field of the master conntrack shows 1.
>>
>> So, I think that these patches are still the way to fix the problem with
>> the expectations created through ctnetlink.
> 
> Just for my understanding, why not simply take the reference for
> the master conntrack for "fulfilled" expectations (IOW conntracks
> that have a master assigned), which is necessary anyways since
> destroy_conntrack() attempts to release that reference again, and
> clean up userspace expectations once the master conntrack vanishes?

That's fine to me.

> This would match what we do for kernel expectations and I don't
> really see why we would want to keep an unfulfilled userspace
> expectation without the corresponding master.

Agreed.

Then, we need to allocate the conntrack helper extension to store the
list of unfulfilled expectations, which is what my patches do.

Re: [PATCH 0/2] rework of userspace expectation support

[Sam Roberts]

Patrick, are you going to merge these?

I don't see it in nf-next-2.6.git.

On Thu, Apr 21, 2011 at 6:14 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On 20/04/11 16:06, Patrick McHardy wrote:
>> I thought this is what your patches try to fix?
>
> Yes, my patches add the helper extension to the conntrack by means of
> the CT target. Thus, in the case of userspace helpers, the master
> conntrack can store the list of unfulfilled expectations that it owns.

Its not clear to me why the CT master isn't added on a master-less
connection at the time that a userspace expectation is set (instead of
having to use the CT target in the raw table to create the master for
all connections that might potentially need a userspace expectation).

However, I'll take any fix that stops the oopsing.

Thanks,
Sam

Re: [PATCH 0/2] rework of userspace expectation support

[Sam Roberts]

On Tue, May 17, 2011 at 2:12 PM, Sam Roberts <vieuxtech@gmail.com> wrote:
> Patrick, are you going to merge these?
>
> I don't see it in nf-next-2.6.git.

Btw, here's a reproduction tool that will oops:

https://github.com/sam-github/nfct-expect-oops

Cheers,
Sam