Patchwork netfilter: nf_ct_expect: partially implement ctnetlink_change_expect

login
register
mail settings
Submitter Kelvie Wong
Date May 3, 2012, 12:39 a.m.
Message ID <1336005564-23171-3-git-send-email-kelvie@ieee.org>
Download mbox | patch
Permalink /patch/156581/
State Accepted
Headers show

Comments

Kelvie Wong - May 3, 2012, 12:39 a.m.
This refreshes the "timeout" attribute in existing expectations if one is
given.

The use case for this would be for userspace helpers to extend the lifetime
of the expectation when requested, as this is not possible right now
without deleting/recreating the expectation.

I use this specifically for forwarding DCERPC traffic through:

DCERPC has a port mapper daemon that chooses a (seemingly) random port for
future traffic to go to. We expect this traffic (with a reasonable
timeout), but sometimes the port mapper will tell the client to continue
using the same port. This allows us to extend the expectation accordingly.

Signed-off-by: Kelvie Wong <kelvie@ieee.org>
---
 net/netfilter/nf_conntrack_netlink.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
Pablo Neira - May 6, 2012, 11:09 p.m.
Hi Kelvie,

On Wed, May 02, 2012 at 05:39:24PM -0700, Kelvie Wong wrote:
> This refreshes the "timeout" attribute in existing expectations if one is
> given.
> 
> The use case for this would be for userspace helpers to extend the lifetime
> of the expectation when requested, as this is not possible right now
> without deleting/recreating the expectation.
> 
> I use this specifically for forwarding DCERPC traffic through:
> 
> DCERPC has a port mapper daemon that chooses a (seemingly) random port for
> future traffic to go to. We expect this traffic (with a reasonable
> timeout), but sometimes the port mapper will tell the client to continue
> using the same port. This allows us to extend the expectation accordingly.
> 
> Signed-off-by: Kelvie Wong <kelvie@ieee.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index ca7e835..87a9682 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2065,6 +2065,16 @@ static int
>  ctnetlink_change_expect(struct nf_conntrack_expect *x,
>  			const struct nlattr * const cda[])
>  {
> +	/* Refresh the timeout */
> +	if (cda[CTA_EXPECT_TIMEOUT]) {
> +		if (!del_timer(&x->timeout))
> +			return -ETIME;
> +
> +		x->timeout.expires = jiffies +
> +			ntohl(nla_get_be32(cda[CTA_EXPECT_TIMEOUT])) * HZ;
> +		add_timer(&x->timeout);
> +		return 0;
> +	}

You have to protect this with nf_conntrack_lock spinlock. See
net/netfilter/nf_conntrack_expect.c for expectation handling.

>  	return -EOPNOTSUPP;

Now that we support expectation changing, this should be return 0.

We have two choices for this:

a) rework the patch with the suggestion that I made.
b) add some NF_CT_EXPECT_FIXED_TIMEOUT flag like we have in the
   connection tracking. Thus, the expectation will not ever expire.

I'd need to know more about how you're using this. Depending on that,
we can select a) or b).

BTW, I'm working on finishing some user-space framework for developing
helper in user-space. My question is: would you be interested in
integrating your DCERPC helper into it?

I expect to post some code soon, still working on it.
--
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
Kelvie Wong - May 7, 2012, 1:51 a.m.
Hey Pablo,

On Sun, May 6, 2012 at 4:09 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> You have to protect this with nf_conntrack_lock spinlock. See
> net/netfilter/nf_conntrack_expect.c for expectation handling.

ctnetlink_change_expect is not exported, and it is only called in
ctnetlink_new_expect, which is protected by the spinlock.

>
>>       return -EOPNOTSUPP;
>
> Now that we support expectation changing, this should be return 0.

I can make this change.

> We have two choices for this:
>
> a) rework the patch with the suggestion that I made.
> b) add some NF_CT_EXPECT_FIXED_TIMEOUT flag like we have in the
>   connection tracking. Thus, the expectation will not ever expire.
>
> I'd need to know more about how you're using this. Depending on that,
> we can select a) or b).

I think we need to do a). A fixed timeout won't work, as in some cases we
need to extend the expectation (the server has asked to use the same port
again, so we need to give it another 10 minutes, possibly indefinitely),
whereas in other cases we can just safely let the expectation expire.

I want to avoid leaving the expectation forever, but I can't know until I see
the DCERPC traffic.

>
> BTW, I'm working on finishing some user-space framework for developing
> helper in user-space. My question is: would you be interested in
> integrating your DCERPC helper into it?
>
> I expect to post some code soon, still working on it.

I just need something to work right now (I'm going to use my original patch
as-is, unless I made a grave error somewhere), but maybe in the future if
it will ease maintenance.

Thanks for your help!
Kelvie Wong - May 7, 2012, 1:53 a.m.
On Sun, May 6, 2012 at 6:51 PM, Kelvie Wong <kelvie@ieee.org> wrote:
> ctnetlink_change_expect is not exported, and it is only called in
> ctnetlink_new_expect, which is protected by the spinlock.
>

Also, in a similar function, ctnetlink_change_conntrack, the locking is
done by the caller, in a similar fashion.
Pablo Neira - May 7, 2012, 8:42 a.m.
On Sun, May 06, 2012 at 06:51:45PM -0700, Kelvie Wong wrote:
> Hey Pablo,
> 
> On Sun, May 6, 2012 at 4:09 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > You have to protect this with nf_conntrack_lock spinlock. See
> > net/netfilter/nf_conntrack_expect.c for expectation handling.
> 
> ctnetlink_change_expect is not exported, and it is only called in
> ctnetlink_new_expect, which is protected by the spinlock.

You're right, I've overlooked this.

> >
> >>       return -EOPNOTSUPP;
> >
> > Now that we support expectation changing, this should be return 0.
> 
> I can make this change.
> 
> > We have two choices for this:
> >
> > a) rework the patch with the suggestion that I made.
> > b) add some NF_CT_EXPECT_FIXED_TIMEOUT flag like we have in the
> >   connection tracking. Thus, the expectation will not ever expire.
> >
> > I'd need to know more about how you're using this. Depending on that,
> > we can select a) or b).
> 
> I think we need to do a). A fixed timeout won't work, as in some cases we
> need to extend the expectation (the server has asked to use the same port
> again, so we need to give it another 10 minutes, possibly indefinitely),
> whereas in other cases we can just safely let the expectation expire.
> 
> I want to avoid leaving the expectation forever, but I can't know until I see
> the DCERPC traffic.

OK, then I'll take your patch. I'll mangle it to return 0 instead.

> > BTW, I'm working on finishing some user-space framework for developing
> > helper in user-space. My question is: would you be interested in
> > integrating your DCERPC helper into it?
> >
> > I expect to post some code soon, still working on it.
> 
> I just need something to work right now (I'm going to use my original patch
> as-is, unless I made a grave error somewhere), but maybe in the future if
> it will ease maintenance.

I guess it will ease maintainance, really.
--
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
Kelvie Wong - May 7, 2012, 4:28 p.m.
On Mon, May 7, 2012 at 1:42 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> OK, then I'll take your patch. I'll mangle it to return 0 instead.

That will be fine.

Thank you,

Patch

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index ca7e835..87a9682 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2065,6 +2065,16 @@  static int
 ctnetlink_change_expect(struct nf_conntrack_expect *x,
 			const struct nlattr * const cda[])
 {
+	/* Refresh the timeout */
+	if (cda[CTA_EXPECT_TIMEOUT]) {
+		if (!del_timer(&x->timeout))
+			return -ETIME;
+
+		x->timeout.expires = jiffies +
+			ntohl(nla_get_be32(cda[CTA_EXPECT_TIMEOUT])) * HZ;
+		add_timer(&x->timeout);
+		return 0;
+	}
 	return -EOPNOTSUPP;
 }