skb_ip_make_writable and skbs not owned by a socket

skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

Hi,

skb_ip_make_writable copies the packet as soon as the data area needs
to be touched. This is of course necessary for packets generated locally,
but can't we mangle the data area of skbs with skb->sk == NULL without
copying them ?

Regards
Patrick

Re: skb_ip_make_writable and skbs not owned by a socket

[Thomas Graf]

* Patrick McHardy <41D86571.6070501@trash.net> 2005-01-02 22:19
> skb_ip_make_writable copies the packet as soon as the data area needs
> to be touched. This is of course necessary for packets generated locally,
> but can't we mangle the data area of skbs with skb->sk == NULL without
> copying them ?

Theoretically there could be a driver for a S/G capable nic producing
pskbs. I'm not aware of such a driver though. Assuming there is no
such driver at the moment, the question is whether to make all the
paths aware as a precaution (net/sched/ is not aware as of now). I
think everyone would agree if there wasn't such ia high possible
performance impact respectively saving. Thoughts?

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

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

Thomas Graf wrote:

>* Patrick McHardy <41D86571.6070501@trash.net> 2005-01-02 22:19
>  
>
>>skb_ip_make_writable copies the packet as soon as the data area needs
>>to be touched. This is of course necessary for packets generated locally,
>>but can't we mangle the data area of skbs with skb->sk == NULL without
>>copying them ?
>>    
>>
>
>Theoretically there could be a driver for a S/G capable nic producing
>pskbs. I'm not aware of such a driver though. Assuming there is no
>such driver at the moment, the question is whether to make all the
>paths aware as a precaution (net/sched/ is not aware as of now). I
>think everyone would agree if there wasn't such ia high possible
>performance impact respectively saving. Thoughts?
>
I was refering to a different problem. skb_ip_make_writable copies
skbs when the data area needs to be mangled to avoid disturbing the
tcp retransmission queue. Not sure why it is done for UDP and ICMP.
My question is if we can asume it is safe to alter the data area of
a skb, if it is not shared, or cloned, in the linear range and
skb->sk == NULL. See the attached patch. A different question is
why we can't simply do this:

(cut-n-paste, I would cut at least one goto in a patch)

int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)
{
        struct sk_buff *nskb;

        if (writable_len > (*pskb)->len)
                return 0;

        /* Not exclusive use of packet?  Must copy. */
        if (skb_shared(*pskb) || skb_cloned(*pskb))
                goto copy_skb;

        if (skb_headlen(skb) <= writable_len)
                return 1;
        goto pull_skb;

copy_skb:
        nskb = skb_copy(*pskb, GFP_ATOMIC);
        if (!nskb)
                return 0;
        BUG_ON(skb_is_nonlinear(nskb));

        /* Rest of kernel will get very unhappy if we pass it a
           suddenly-orphaned skbuff */
        if ((*pskb)->sk)
                skb_set_owner_w(nskb, (*pskb)->sk);
        kfree_skb(*pskb);
        *pskb = nskb;
        return 1;

pull_skb:
        return pskb_may_pull(*pskb, writable_len);
}

Packets cloned or shared with the TCP retransmission queue are
already caught by the second condition. This should avoid lots
of copies compared to the current code.

Regards
Patrick


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

===== net/core/netfilter.c 1.37 vs edited =====
--- 1.37/net/core/netfilter.c	2004-11-13 14:41:07 +01:00
+++ edited/net/core/netfilter.c	2005-01-03 02:17:16 +01:00
@@ -691,6 +691,12 @@
 	if (writable_len <= (*pskb)->nh.iph->ihl*4)
 		return 1;
 
+	if (skb->sk == NULL) {
+		if (skb_headlen(skb) <= writable_len)
+			return 1;
+		goto pull_skb;
+	}
+
 	iplen = writable_len - (*pskb)->nh.iph->ihl*4;
 
 	/* DaveM says protocol headers are also modifiable. */

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

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

Patrick McHardy wrote:

> A different question is why we can't simply do this:
>
> (cut-n-paste, I would cut at least one goto in a patch)
>
> int skb_ip_make_writable(struct sk_buff **pskb, unsigned int 
> writable_len)
> {
>        struct sk_buff *nskb;
>
>        if (writable_len > (*pskb)->len)
>                return 0;
>
>        /* Not exclusive use of packet?  Must copy. */
>        if (skb_shared(*pskb) || skb_cloned(*pskb))
>                goto copy_skb;
>
>        if (skb_headlen(skb) <= writable_len)
>                return 1;

This should have been >=, but is not necessary anyway, pskb_may_pull
already does the same test. Real patch attached.



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

===== net/core/netfilter.c 1.37 vs edited =====
--- 1.37/net/core/netfilter.c	2004-11-13 14:41:07 +01:00
+++ edited/net/core/netfilter.c	2005-01-03 02:50:29 +01:00
@@ -678,7 +678,6 @@
 int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)
 {
 	struct sk_buff *nskb;
-	unsigned int iplen;
 
 	if (writable_len > (*pskb)->len)
 		return 0;
@@ -687,35 +686,7 @@
 	if (skb_shared(*pskb) || skb_cloned(*pskb))
 		goto copy_skb;
 
-	/* Alexey says IP hdr is always modifiable and linear, so ok. */
-	if (writable_len <= (*pskb)->nh.iph->ihl*4)
-		return 1;
-
-	iplen = writable_len - (*pskb)->nh.iph->ihl*4;
-
-	/* DaveM says protocol headers are also modifiable. */
-	switch ((*pskb)->nh.iph->protocol) {
-	case IPPROTO_TCP: {
-		struct tcphdr _hdr, *hp;
-		hp = skb_header_pointer(*pskb, (*pskb)->nh.iph->ihl*4,
-					sizeof(_hdr), &_hdr);
-		if (hp == NULL)
-			goto copy_skb;
-		if (writable_len <= (*pskb)->nh.iph->ihl*4 + hp->doff*4)
-			goto pull_skb;
-		goto copy_skb;
-	}
-	case IPPROTO_UDP:
-		if (writable_len<=(*pskb)->nh.iph->ihl*4+sizeof(struct udphdr))
-			goto pull_skb;
-		goto copy_skb;
-	case IPPROTO_ICMP:
-		if (writable_len
-		    <= (*pskb)->nh.iph->ihl*4 + sizeof(struct icmphdr))
-			goto pull_skb;
-		goto copy_skb;
-	/* Insert other cases here as desired */
-	}
+	return pskb_may_pull(*pskb, writable_len);
 
 copy_skb:
 	nskb = skb_copy(*pskb, GFP_ATOMIC);
@@ -730,9 +701,6 @@
 	kfree_skb(*pskb);
 	*pskb = nskb;
 	return 1;
-
-pull_skb:
-	return pskb_may_pull(*pskb, writable_len);
 }
 EXPORT_SYMBOL(skb_ip_make_writable);
 #endif /*CONFIG_INET*/

Re: skb_ip_make_writable and skbs not owned by a socket

[jamal]

Doesnt netfilter these days have some low level bridge level and
arp level munging? 
Also i know there are UDP based apps which retransmit, but they probably
keep their own copies of skbs. It does sound rude to make this change
but if you are absolutely sure it doesnt affect anyone, why not.

cheers,
jamal

On Sun, 2005-01-02 at 20:53, Patrick McHardy wrote:
> Patrick McHardy wrote:
> 
> > A different question is why we can't simply do this:
> >
> > (cut-n-paste, I would cut at least one goto in a patch)
> >
> > int skb_ip_make_writable(struct sk_buff **pskb, unsigned int 
> > writable_len)
> > {
> >        struct sk_buff *nskb;
> >
> >        if (writable_len > (*pskb)->len)
> >                return 0;
> >
> >        /* Not exclusive use of packet?  Must copy. */
> >        if (skb_shared(*pskb) || skb_cloned(*pskb))
> >                goto copy_skb;
> >
> >        if (skb_headlen(skb) <= writable_len)
> >                return 1;
> 
> This should have been >=, but is not necessary anyway, pskb_may_pull
> already does the same test. Real patch attached.
> 
> 
> 
> ______________________________________________________________________
> 
> ===== net/core/netfilter.c 1.37 vs edited =====
> --- 1.37/net/core/netfilter.c	2004-11-13 14:41:07 +01:00
> +++ edited/net/core/netfilter.c	2005-01-03 02:50:29 +01:00
> @@ -678,7 +678,6 @@
>  int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)
>  {
>  	struct sk_buff *nskb;
> -	unsigned int iplen;
>  
>  	if (writable_len > (*pskb)->len)
>  		return 0;
> @@ -687,35 +686,7 @@
>  	if (skb_shared(*pskb) || skb_cloned(*pskb))
>  		goto copy_skb;
>  
> -	/* Alexey says IP hdr is always modifiable and linear, so ok. */
> -	if (writable_len <= (*pskb)->nh.iph->ihl*4)
> -		return 1;
> -
> -	iplen = writable_len - (*pskb)->nh.iph->ihl*4;
> -
> -	/* DaveM says protocol headers are also modifiable. */
> -	switch ((*pskb)->nh.iph->protocol) {
> -	case IPPROTO_TCP: {
> -		struct tcphdr _hdr, *hp;
> -		hp = skb_header_pointer(*pskb, (*pskb)->nh.iph->ihl*4,
> -					sizeof(_hdr), &_hdr);
> -		if (hp == NULL)
> -			goto copy_skb;
> -		if (writable_len <= (*pskb)->nh.iph->ihl*4 + hp->doff*4)
> -			goto pull_skb;
> -		goto copy_skb;
> -	}
> -	case IPPROTO_UDP:
> -		if (writable_len<=(*pskb)->nh.iph->ihl*4+sizeof(struct udphdr))
> -			goto pull_skb;
> -		goto copy_skb;
> -	case IPPROTO_ICMP:
> -		if (writable_len
> -		    <= (*pskb)->nh.iph->ihl*4 + sizeof(struct icmphdr))
> -			goto pull_skb;
> -		goto copy_skb;
> -	/* Insert other cases here as desired */
> -	}
> +	return pskb_may_pull(*pskb, writable_len);
>  
>  copy_skb:
>  	nskb = skb_copy(*pskb, GFP_ATOMIC);
> @@ -730,9 +701,6 @@
>  	kfree_skb(*pskb);
>  	*pskb = nskb;
>  	return 1;
> -
> -pull_skb:
> -	return pskb_may_pull(*pskb, writable_len);
>  }
>  EXPORT_SYMBOL(skb_ip_make_writable);
>  #endif /*CONFIG_INET*/

Re: skb_ip_make_writable and skbs not owned by a socket

[David S. Miller]

On Sun, 02 Jan 2005 22:19:45 +0100
Patrick McHardy <kaber@trash.net> wrote:

> skb_ip_make_writable copies the packet as soon as the data area needs
> to be touched. This is of course necessary for packets generated locally,
> but can't we mangle the data area of skbs with skb->sk == NULL without
> copying them ?

Not if they are cloned.  tcpdump can still share access to the
packet.

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

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

David S. Miller wrote:

>On Sun, 02 Jan 2005 22:19:45 +0100
>Patrick McHardy <kaber@trash.net> wrote:
>
>  
>
>>skb_ip_make_writable copies the packet as soon as the data area needs
>>to be touched. This is of course necessary for packets generated locally,
>>but can't we mangle the data area of skbs with skb->sk == NULL without
>>copying them ?
>>    
>>
>
>Not if they are cloned.  tcpdump can still share access to the
>packet.
>
>  
>
I wasn't very clear in my question, it only makes sense if you look at
skb_ip_make_writable :) It already checks for skb_shared || skb_cloned
to decide when to copy, but additionally makes some guesses based on the
protocol. I think the checks for skb_shared || skb_clones should already
catch all cases where copying is necessary, and the additional cases
could be removed.

Regards
Patrick


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

===== net/core/netfilter.c 1.37 vs edited =====
--- 1.37/net/core/netfilter.c	2004-11-13 14:41:07 +01:00
+++ edited/net/core/netfilter.c	2005-01-03 02:50:29 +01:00
@@ -678,7 +678,6 @@
 int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)
 {
 	struct sk_buff *nskb;
-	unsigned int iplen;
 
 	if (writable_len > (*pskb)->len)
 		return 0;
@@ -687,35 +686,7 @@
 	if (skb_shared(*pskb) || skb_cloned(*pskb))
 		goto copy_skb;
 
-	/* Alexey says IP hdr is always modifiable and linear, so ok. */
-	if (writable_len <= (*pskb)->nh.iph->ihl*4)
-		return 1;
-
-	iplen = writable_len - (*pskb)->nh.iph->ihl*4;
-
-	/* DaveM says protocol headers are also modifiable. */
-	switch ((*pskb)->nh.iph->protocol) {
-	case IPPROTO_TCP: {
-		struct tcphdr _hdr, *hp;
-		hp = skb_header_pointer(*pskb, (*pskb)->nh.iph->ihl*4,
-					sizeof(_hdr), &_hdr);
-		if (hp == NULL)
-			goto copy_skb;
-		if (writable_len <= (*pskb)->nh.iph->ihl*4 + hp->doff*4)
-			goto pull_skb;
-		goto copy_skb;
-	}
-	case IPPROTO_UDP:
-		if (writable_len<=(*pskb)->nh.iph->ihl*4+sizeof(struct udphdr))
-			goto pull_skb;
-		goto copy_skb;
-	case IPPROTO_ICMP:
-		if (writable_len
-		    <= (*pskb)->nh.iph->ihl*4 + sizeof(struct icmphdr))
-			goto pull_skb;
-		goto copy_skb;
-	/* Insert other cases here as desired */
-	}
+	return pskb_may_pull(*pskb, writable_len);
 
 copy_skb:
 	nskb = skb_copy(*pskb, GFP_ATOMIC);
@@ -730,9 +701,6 @@
 	kfree_skb(*pskb);
 	*pskb = nskb;
 	return 1;
-
-pull_skb:
-	return pskb_may_pull(*pskb, writable_len);
 }
 EXPORT_SYMBOL(skb_ip_make_writable);
 #endif /*CONFIG_INET*/

Re: skb_ip_make_writable and skbs not owned by a socket

[David S. Miller]

On Fri, 14 Jan 2005 06:59:00 +0100
Patrick McHardy <kaber@trash.net> wrote:

> I think the checks for skb_shared || skb_clones should already
> catch all cases where copying is necessary, and the additional cases
> could be removed.

You're probably right, let me think about this some more.

Re: skb_ip_make_writable and skbs not owned by a socket

[David S. Miller]

On Fri, 14 Jan 2005 06:59:00 +0100
Patrick McHardy <kaber@trash.net> wrote:

> I wasn't very clear in my question, it only makes sense if you look at
> skb_ip_make_writable :) It already checks for skb_shared || skb_cloned
> to decide when to copy, but additionally makes some guesses based on the
> protocol. I think the checks for skb_shared || skb_clones should already
> catch all cases where copying is necessary, and the additional cases
> could be removed.

This code is doing something different.  Even if we unclone and
unshare the SKB, the packet header portion can still be non-linear.

It will be possible for devices to create this situation on receive.
That's why we have all the pskb_may_pull() checks in all the major
ipv4 protocol input paths.

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

David S. Miller wrote:

>This code is doing something different.  Even if we unclone and
>unshare the SKB, the packet header portion can still be non-linear.
>
>It will be possible for devices to create this situation on receive.
>That's why we have all the pskb_may_pull() checks in all the major
>ipv4 protocol input paths.
>
The patch already calls pskb_may_pull for unshared/uncloned skbs, so it
linearizes them up to the requested size. It just replaces skb_copy by
pskb_may_pull when the data area of unshared/uncloned skbs needs to be
mangled.

Regards
Patrick

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

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

Dave, could you please have another look at this ? I think the patch is
correct, and even necessary to make the ipt action work with targets
that mangle the data area. Jamal asked me to put back the pskb_expand_head
call in the ipt action, so iptables won't see cloned skbs and copy them
(actions must not replace the skb), but the protocol-dependant cases
in skb_ip_make_writable can cause copying anyway. I think you missed
my reply to your last mail, please see below.

Thanks,
Patrick

Patrick McHardy wrote:

> David S. Miller wrote:
>
>> This code is doing something different.  Even if we unclone and
>> unshare the SKB, the packet header portion can still be non-linear.
>>
>> It will be possible for devices to create this situation on receive.
>> That's why we have all the pskb_may_pull() checks in all the major
>> ipv4 protocol input paths.
>>
> The patch already calls pskb_may_pull for unshared/uncloned skbs, so it
> linearizes them up to the requested size. It just replaces skb_copy by
> pskb_may_pull when the data area of unshared/uncloned skbs needs to be
> mangled.
>
> Regards
> Patrick
>
>


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

===== net/core/netfilter.c 1.37 vs edited =====
--- 1.37/net/core/netfilter.c	2004-11-13 14:41:07 +01:00
+++ edited/net/core/netfilter.c	2005-01-03 02:50:29 +01:00
@@ -678,7 +678,6 @@
 int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)
 {
 	struct sk_buff *nskb;
-	unsigned int iplen;
 
 	if (writable_len > (*pskb)->len)
 		return 0;
@@ -687,35 +686,7 @@
 	if (skb_shared(*pskb) || skb_cloned(*pskb))
 		goto copy_skb;
 
-	/* Alexey says IP hdr is always modifiable and linear, so ok. */
-	if (writable_len <= (*pskb)->nh.iph->ihl*4)
-		return 1;
-
-	iplen = writable_len - (*pskb)->nh.iph->ihl*4;
-
-	/* DaveM says protocol headers are also modifiable. */
-	switch ((*pskb)->nh.iph->protocol) {
-	case IPPROTO_TCP: {
-		struct tcphdr _hdr, *hp;
-		hp = skb_header_pointer(*pskb, (*pskb)->nh.iph->ihl*4,
-					sizeof(_hdr), &_hdr);
-		if (hp == NULL)
-			goto copy_skb;
-		if (writable_len <= (*pskb)->nh.iph->ihl*4 + hp->doff*4)
-			goto pull_skb;
-		goto copy_skb;
-	}
-	case IPPROTO_UDP:
-		if (writable_len<=(*pskb)->nh.iph->ihl*4+sizeof(struct udphdr))
-			goto pull_skb;
-		goto copy_skb;
-	case IPPROTO_ICMP:
-		if (writable_len
-		    <= (*pskb)->nh.iph->ihl*4 + sizeof(struct icmphdr))
-			goto pull_skb;
-		goto copy_skb;
-	/* Insert other cases here as desired */
-	}
+	return pskb_may_pull(*pskb, writable_len);
 
 copy_skb:
 	nskb = skb_copy(*pskb, GFP_ATOMIC);
@@ -730,9 +701,6 @@
 	kfree_skb(*pskb);
 	*pskb = nskb;
 	return 1;
-
-pull_skb:
-	return pskb_may_pull(*pskb, writable_len);
 }
 EXPORT_SYMBOL(skb_ip_make_writable);
 #endif /*CONFIG_INET*/

Re: skb_ip_make_writable and skbs not owned by a socket

[David S. Miller]

On Thu, 27 Jan 2005 05:10:42 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Dave, could you please have another look at this ? I think the patch is
> correct, and even necessary to make the ipt action work with targets
> that mangle the data area. Jamal asked me to put back the pskb_expand_head
> call in the ipt action, so iptables won't see cloned skbs and copy them
> (actions must not replace the skb), but the protocol-dependant cases
> in skb_ip_make_writable can cause copying anyway. I think you missed
> my reply to your last mail, please see below.

It's in my backlog, don't worry.  I'll try to get to it tomorrow.

Is it the end of the world if I defer it to 2.6.12?

Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy]

David S. Miller wrote:

>It's in my backlog, don't worry.  I'll try to get to it tomorrow.
>
>Is it the end of the world if I defer it to 2.6.12?
>
>
No, certainly not. I just wanted to make sure it didn't get lost.

Regards
Patrick