[PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Jason Gunthorpe]

At least the XEN net front driver always produces non linear skbs,
so the SIP module does nothing at all when used with that NIC.

Unconditionally linearize the skb..

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 net/netfilter/nf_conntrack_sip.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

Patrick/Jan, thanks.. This is what I wanted to do in the first place,
but I couldn't convince myself it was safe, as no other nf code does
this..

Unfortunately I can no longer test it :(
 
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 4b57216..02d0b59 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1275,13 +1275,10 @@ static int sip_help(struct sk_buff *skb,
 
 	nf_ct_refresh(ct, skb, sip_timeout * HZ);
 
-	if (!skb_is_nonlinear(skb))
-		dptr = skb->data + dataoff;
-	else {
-		pr_debug("Copy of skbuff not supported yet.\n");
-		return NF_ACCEPT;
-	}
+	if (unlikely(skb_linearize(skb)))
+		return NF_DROP;
 
+	dptr = skb->data + dataoff;
 	datalen = skb->len - dataoff;
 	if (datalen < strlen("SIP/2.0 200"))
 		return NF_ACCEPT;
-- 
1.6.0.4

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

Jason Gunthorpe wrote:
> At least the XEN net front driver always produces non linear skbs,
> so the SIP module does nothing at all when used with that NIC.
> 
> Unconditionally linearize the skb..
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  net/netfilter/nf_conntrack_sip.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> Patrick/Jan, thanks.. This is what I wanted to do in the first place,
> but I couldn't convince myself it was safe, as no other nf code does
> this..

Your patch is based on an old version, the current version also
supports TCP. I'll commit this patch to my tree after some testing.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 881 bytes --]

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index b20f427..45750cc 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
 
 	nf_ct_refresh(ct, skb, sip_timeout * HZ);
 
-	if (skb_is_nonlinear(skb)) {
-		pr_debug("Copy of skbuff not supported yet.\n");
+	if (unlikely(skb_linearize(skb)))
 		return NF_ACCEPT;
-	}
 
 	dptr = skb->data + dataoff;
 	datalen = skb->len - dataoff;
@@ -1455,10 +1453,8 @@ static int sip_help_udp(struct sk_buff *skb, unsigned int protoff,
 
 	nf_ct_refresh(ct, skb, sip_timeout * HZ);
 
-	if (skb_is_nonlinear(skb)) {
-		pr_debug("Copy of skbuff not supported yet.\n");
+	if (unlikely(skb_linearize(skb)))
 		return NF_ACCEPT;
-	}
 
 	dptr = skb->data + dataoff;
 	datalen = skb->len - dataoff;

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Jason Gunthorpe]

On Fri, May 14, 2010 at 08:13:03PM +0200, Patrick McHardy wrote:
> Your patch is based on an old version, the current version also
> supports TCP. I'll commit this patch to my tree after some testing.

Thanks!

> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index b20f427..45750cc 100644
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
>  
>  	nf_ct_refresh(ct, skb, sip_timeout * HZ);
>  
> -	if (skb_is_nonlinear(skb)) {
> -		pr_debug("Copy of skbuff not supported yet.\n");
> +	if (unlikely(skb_linearize(skb)))
>  		return NF_ACCEPT;
> -	}

Should this be NF_DROP? As I understand it skb_linearize only failes
if it runs out of memory, which probably means dropping is OK. But
passing a packet that might need rewriting could be harmful..

Jason

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Jan Engelhardt]

On Friday 2010-05-14 20:13, Patrick McHardy wrote:
>Jason Gunthorpe wrote:
>> At least the XEN net front driver always produces non linear skbs,
>> so the SIP module does nothing at all when used with that NIC.
>> 
>> Unconditionally linearize the skb..
>> 
>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> ---
>>  net/netfilter/nf_conntrack_sip.c |    9 +++------
>>  1 files changed, 3 insertions(+), 6 deletions(-)
>> 
>> Patrick/Jan, thanks.. This is what I wanted to do in the first place,
>> but I couldn't convince myself it was safe, as no other nf code does
>> this..
>
>Your patch is based on an old version, the current version also
>supports TCP. I'll commit this patch to my tree after some testing.

nf_defrag defragments the packets, but then they're still non-linear?
I'm clearly missing something, could somenoe elaborate?

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Patrick McHardy]

Jason Gunthorpe wrote:
> On Fri, May 14, 2010 at 08:13:03PM +0200, Patrick McHardy wrote:
>> Your patch is based on an old version, the current version also
>> supports TCP. I'll commit this patch to my tree after some testing.
> 
> Thanks!
> 
>> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
>> index b20f427..45750cc 100644
>> +++ b/net/netfilter/nf_conntrack_sip.c
>> @@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
>>  
>>  	nf_ct_refresh(ct, skb, sip_timeout * HZ);
>>  
>> -	if (skb_is_nonlinear(skb)) {
>> -		pr_debug("Copy of skbuff not supported yet.\n");
>> +	if (unlikely(skb_linearize(skb)))
>>  		return NF_ACCEPT;
>> -	}
> 
> Should this be NF_DROP? As I understand it skb_linearize only failes
> if it runs out of memory, which probably means dropping is OK. But
> passing a packet that might need rewriting could be harmful..

We so far also didn't rewrite the packet. But agreed, its
a corner case and dropping it is the safer choice.

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Patrick McHardy]

Jan Engelhardt wrote:
> On Friday 2010-05-14 20:13, Patrick McHardy wrote:
>> Jason Gunthorpe wrote:
>>> At least the XEN net front driver always produces non linear skbs,
>>> so the SIP module does nothing at all when used with that NIC.
>>>
>>> Unconditionally linearize the skb..
>>>
>>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>> ---
>>>  net/netfilter/nf_conntrack_sip.c |    9 +++------
>>>  1 files changed, 3 insertions(+), 6 deletions(-)
>>>
>>> Patrick/Jan, thanks.. This is what I wanted to do in the first place,
>>> but I couldn't convince myself it was safe, as no other nf code does
>>> this..
>> Your patch is based on an old version, the current version also
>> supports TCP. I'll commit this patch to my tree after some testing.
> 
> nf_defrag defragments the packets, but then they're still non-linear?
> I'm clearly missing something, could somenoe elaborate?

We're talking about packets with non-linear data, which is unrelated
to fragments. Reassembled fragments are non-linear as well though.

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Patrick McHardy wrote:
> Jason Gunthorpe wrote:
>> On Fri, May 14, 2010 at 08:13:03PM +0200, Patrick McHardy wrote:
>>> Your patch is based on an old version, the current version also
>>> supports TCP. I'll commit this patch to my tree after some testing.
>> Thanks!
>>
>>> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
>>> index b20f427..45750cc 100644
>>> +++ b/net/netfilter/nf_conntrack_sip.c
>>> @@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
>>>  
>>>  	nf_ct_refresh(ct, skb, sip_timeout * HZ);
>>>  
>>> -	if (skb_is_nonlinear(skb)) {
>>> -		pr_debug("Copy of skbuff not supported yet.\n");
>>> +	if (unlikely(skb_linearize(skb)))
>>>  		return NF_ACCEPT;
>>> -	}
>> Should this be NF_DROP? As I understand it skb_linearize only failes
>> if it runs out of memory, which probably means dropping is OK. But
>> passing a packet that might need rewriting could be harmful..
> 
> We so far also didn't rewrite the packet. But agreed, its
> a corner case and dropping it is the safer choice.

This is what I've added to my tree. Tested with asterisk and TSO
enabled NIC, which fails without this patch.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1462 bytes --]

commit a1d7c1b4b8dfbc5ecadcff9284d64bb6ad4c0196
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri May 14 21:18:17 2010 +0200

    netfilter: nf_ct_sip: handle non-linear skbs
    
    Handle non-linear skbs by linearizing them instead of silently failing.
    Long term the helper should be fixed to either work with non-linear skbs
    directly by using the string search API or work on a copy of the data.
    
    Based on patch by Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index b20f427..53d8922 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
 
 	nf_ct_refresh(ct, skb, sip_timeout * HZ);
 
-	if (skb_is_nonlinear(skb)) {
-		pr_debug("Copy of skbuff not supported yet.\n");
-		return NF_ACCEPT;
-	}
+	if (unlikely(skb_linearize(skb)))
+		return NF_DROP;
 
 	dptr = skb->data + dataoff;
 	datalen = skb->len - dataoff;
@@ -1455,10 +1453,8 @@ static int sip_help_udp(struct sk_buff *skb, unsigned int protoff,
 
 	nf_ct_refresh(ct, skb, sip_timeout * HZ);
 
-	if (skb_is_nonlinear(skb)) {
-		pr_debug("Copy of skbuff not supported yet.\n");
-		return NF_ACCEPT;
-	}
+	if (unlikely(skb_linearize(skb)))
+		return NF_DROP;
 
 	dptr = skb->data + dataoff;
 	datalen = skb->len - dataoff;

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Jan Engelhardt]

On Friday 2010-05-14 21:26, Patrick McHardy wrote:
>>> Should this be NF_DROP? As I understand it skb_linearize only failes
>>> if it runs out of memory, which probably means dropping is OK. But
>>> passing a packet that might need rewriting could be harmful..
>> 
>> We so far also didn't rewrite the packet. But agreed, its
>> a corner case and dropping it is the safer choice.
>
>This is what I've added to my tree. Tested with asterisk and TSO
>enabled NIC, which fails without this patch.
>

[..patch..]

Shouldn't we do this for the other nf_conntrack_xyz too?
That would mean getting rid of the size-limited locked packet
buffer.

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Patrick McHardy]

Jan Engelhardt wrote:
> On Friday 2010-05-14 21:26, Patrick McHardy wrote:
>>>> Should this be NF_DROP? As I understand it skb_linearize only failes
>>>> if it runs out of memory, which probably means dropping is OK. But
>>>> passing a packet that might need rewriting could be harmful..
>>> We so far also didn't rewrite the packet. But agreed, its
>>> a corner case and dropping it is the safer choice.
>> This is what I've added to my tree. Tested with asterisk and TSO
>> enabled NIC, which fails without this patch.
>>
> 
> [..patch..]
> 
> Shouldn't we do this for the other nf_conntrack_xyz too?
> That would mean getting rid of the size-limited locked packet
> buffer.

Those got introduced to avoid the linearization. SIP is kind of special
because its the only helper mangling packets multiple times with
variable sizes and is currently unable to use the data copying scheme.
Amanda does this as well, but uses the string search API, which works
fine.

The best fix to get rid of the copying in other helpers would be to
convert them to the string search API. I started doing that a few
years ago, but never finished it. One related improvement we could
add would be to make only those parts of the skb writable that are
actually written to when mangling packets, at least for non-linear
non-paged skbs (using frag_list).

Re: [PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

[Jason Gunthorpe]

On Fri, May 14, 2010 at 08:42:43PM +0200, Patrick McHardy wrote:

> > Should this be NF_DROP? As I understand it skb_linearize only failes
> > if it runs out of memory, which probably means dropping is OK. But
> > passing a packet that might need rewriting could be harmful..
> 
> We so far also didn't rewrite the packet. But agreed, its
> a corner case and dropping it is the safer choice.

I was just thinking that, say, a request goes out, gets rewritten but
the reply comes back and does not get rewritten = bad. Better to drop.

Looks OK to me..

Jason