Re: skb_ip_make_writable and skbs not owned by a socket

[Patrick McHardy <kaber@trash.net>]

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