[PATCHv2] netfilter: Remove skb_is_nonlinear check from nf_conntrack_sip

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.

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html