Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy <kaber@trash.net>]

[-- 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*/