netlink: enable skb header refcounting before sending first broadcast

PATCH In review Konstantin Khlebnikov 5 participants 18 messages Jul 10, 2015 – Jul 15, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Konstantin Khlebnikov

author

2015-07-10 11:51 UTC #0 lore raw

PATCH 1/1 netlink: enable skb header refcounting before sending first broadcast

This fixes race between non-atomic updates of adjacent bit-fields:
skb->cloned could be lost because netlink broadcast clones skb after sending it to the first listener who sets skb->peeked at the same skb. As a result atomic refcounting of skb header stays disabled and skb_release_data() frees it twice. Race leads to double-free in kmalloc-xxx.

Signed-off-by Konstantin Khlebnikov <[email protected]>

Fixes b19372273164 ("net: reorganize sk_buff for faster __copy_skb_header()")

---
 net/netlink/af_netlink.c |    6 ++++++
 1 file changed, 6 insertions(+)

net/netlink/af_netlink.c +6 -0

index dea925388a5b..921e0d8dfe3a 100644
@@ -2028,6 +2028,12 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
 	info.tx_filter = filter;
 	info.tx_data = filter_data;
 
+	/* Enable atomic refcounting in skb_release_data() before first send:
+	 * non-atomic set of that bit-field in __skb_clone() could race with
+	 * __skb_recv_datagram() which touches the same set of bit-fields.
+	 */
+	skb->cloned = 1;
+
 	/* While we sleep in clone, do not allow to change socket list */
 
 	netlink_lock_table();

Eric Dumazet

Acked-by on v1 1/1

2015-07-10 13:49 UTC #1 lore raw

Quoted 19 lines · Konstantin Khlebnikov

This fixes race between non-atomic updates of adjacent bit-fields:
skb->cloned could be lost because netlink broadcast clones skb after sending it to the first listener who sets skb->peeked at the same skb. As a result atomic refcounting of skb header stays disabled and skb_release_data() frees it twice. Race leads to double-free in kmalloc-xxx.
Signed-off-by Konstantin Khlebnikov <[email protected]>
Fixes b19372273164 ("net: reorganize sk_buff for faster __copy_skb_header()")
---
 net/netlink/af_netlink.c |    6 ++++++
 1 file changed, 6 insertions(+)
net/netlink/af_netlink.c +0 -0
index dea925388a5b..921e0d8dfe3a 100644
@@ -2028,6 +2028,12 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
 	info.tx_filter = filter;
 	info.tx_data = filter_data;

net/netlink/af_netlink.c int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid · line 2039

Quoted 10 lines

net/netlink/af_netlink.c +6 -0

 
+	/* Enable atomic refcounting in skb_release_data() before first send:
+	 * non-atomic set of that bit-field in __skb_clone() could race with
+	 * __skb_recv_datagram() which touches the same set of bit-fields.
+	 */
+	skb->cloned = 1;
+
 	/* While we sleep in clone, do not allow to change socket list */
 
 	netlink_lock_table();

Wow, this is tricky.

I wonder how you found this bug ????

Acked-by Eric Dumazet <[email protected]>

Konstantin Khlebnikov

author

2015-07-10 14:08 UTC #2 lore raw

Quoted 29 lines · Eric Dumazet

Quoted 28 lines · Konstantin Khlebnikov
This fixes race between non-atomic updates of adjacent bit-fields:
skb->cloned could be lost because netlink broadcast clones skb after sending it to the first listener who sets skb->peeked at the same skb. As a result atomic refcounting of skb header stays disabled and skb_release_data() frees it twice. Race leads to double-free in kmalloc-xxx.
Signed-off-by Konstantin Khlebnikov <[email protected]>
Fixes b19372273164 ("net: reorganize sk_buff for faster __copy_skb_header()")
---
  net/netlink/af_netlink.c |    6 ++++++
  1 file changed, 6 insertions(+)
net/netlink/af_netlink.c +6 -0
index dea925388a5b..921e0d8dfe3a 100644
@@ -2028,6 +2028,12 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
  	info.tx_filter = filter;
  	info.tx_data = filter_data;
 
+	/* Enable atomic refcounting in skb_release_data() before first send:
+	 * non-atomic set of that bit-field in __skb_clone() could race with
+	 * __skb_recv_datagram() which touches the same set of bit-fields.
+	 */
+	skb->cloned = 1;
+
  	/* While we sleep in clone, do not allow to change socket list */
 

Quoted 5 lines

net/netlink/af_netlink.c int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid · line 2039
Quoted 1 line
net/netlink/af_netlink.c +0 -0
  	netlink_lock_table();
Wow, this is tricky.
I wonder how you found this bug ????

In some setups race happens quite often: once or twice per hour. I guess the main trigger was the openvswitch which generates a lot of netlink traffic. Though debugging was a real pain.

Quoted 5 lines

Acked-by Eric Dumazet <[email protected]>

Signature · 1 line

Konstantin

Herbert Xu

2015-07-13 07:23 UTC #3 lore raw

In reply to Konstantin Khlebnikov

Quoted 19 lines · Konstantin Khlebnikov

This fixes race between non-atomic updates of adjacent bit-fields:
skb->cloned could be lost because netlink broadcast clones skb after sending it to the first listener who sets skb->peeked at the same skb. As a result atomic refcounting of skb header stays disabled and skb_release_data() frees it twice. Race leads to double-free in kmalloc-xxx.
Signed-off-by Konstantin Khlebnikov <[email protected]>
Fixes b19372273164 ("net: reorganize sk_buff for faster __copy_skb_header()")
---
 net/netlink/af_netlink.c |    6 ++++++
 1 file changed, 6 insertions(+)
net/netlink/af_netlink.c +0 -0
index dea925388a5b..921e0d8dfe3a 100644
@@ -2028,6 +2028,12 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
 	info.tx_filter = filter;
 	info.tx_data = filter_data;

net/netlink/af_netlink.c int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid · line 2039

Quoted 10 lines

net/netlink/af_netlink.c +6 -0

 
+	/* Enable atomic refcounting in skb_release_data() before first send:
+	 * non-atomic set of that bit-field in __skb_clone() could race with
+	 * __skb_recv_datagram() which touches the same set of bit-fields.
+	 */
+	skb->cloned = 1;
+
 	/* While we sleep in clone, do not allow to change socket list */
 
 	netlink_lock_table();

Your effort in finding this bug is wonderful. However I think the fix is a bit dirty.

The real issue here is that the recv path no longer handles shared skbs. So either we need to fix the recv path to not touch skbs without cloning them, or we need to get rid of the use of shared skbs in netlink.

In fact it looks I introduced the bug way back in

commit a59322be07c964e916d15be3df473fb7ba20c41e

Author: Herbert Xu <[email protected]>
Date:   Wed Dec 5 01:53:40 2007 -0800

[UDP]: Only increment counter on first peek/recv

I will try to mend this error :)

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2015-07-13 08:04 UTC #4 lore raw

net: Clone skb before setting peeked flag

Shared skbs must not be modified and this is crucial for broadcast and/or multicast paths where we use it as an optimisation to avoid unnecessary cloning.

The function skb_recv_datagram breaks this rule by setting peeked without cloning the skb first. This causes funky races which leads to double-free.

This patch fixes this by cloning the skb and replacing the skb in the list when setting skb->peeked.

Fixes a59322be07c9 ("[UDP]: Only increment counter on first peek/recv")

Reported-by Konstantin Khlebnikov <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

net/core/datagram.c +38 -3

index b80fb91..4e9a3f6 100644
@@ -131,6 +131,35 @@ out_noerr:
 	goto out;
 }
 
+static int skb_set_peeked(struct sk_buff *skb)
+{
+	struct sk_buff *nskb;
+
+	if (skb->peeked)
+		return 0;
+
+	/* We have to unshare an skb before modifying it. */
+	if (!skb_shared(skb))
+		goto done;
+
+	nskb = skb_clone(skb, GFP_ATOMIC);
+	if (!nskb)
+		return -ENOMEM;
+
+	skb->prev->next = nskb;
+	skb->next->prev = nskb;
+	nskb->prev = skb->prev;
+	nskb->next = skb->next;
+
+	consume_skb(skb);
+	skb = nskb;
+
+done:
+	skb->peeked = 1;
+
+	return 0;
+}
+
 /**
  *	__skb_recv_datagram - Receive a datagram skbuff
  *	@sk: socket

@@ -165,7 +194,9 @@ out_noerr:
 struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 				    int *peeked, int *off, int *err)
 {
+	struct sk_buff_head *queue = &sk->sk_receive_queue;
 	struct sk_buff *skb, *last;
+	unsigned long cpu_flags;
 	long timeo;
 	/*
 	 * Caller is allowed not to check sk->sk_err before skb_recv_datagram()

@@ -184,8 +215,6 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 		 * Look at current nfs client by the way...
 		 * However, this function was correct in any case. 8)
 		 */
-		unsigned long cpu_flags;
-		struct sk_buff_head *queue = &sk->sk_receive_queue;
 		int _off = *off;
 
 		last = (struct sk_buff *)queue;

@@ -199,7 +228,11 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 					_off -= skb->len;
 					continue;
 				}
-				skb->peeked = 1;
+
+				error = skb_set_peeked(skb);
+				if (error)
+					goto unlock_err;
+
 				atomic_inc(&skb->users);
 			} else
 				__skb_unlink(skb, queue);

@@ -223,6 +256,8 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 
 	return NULL;
 
+unlock_err:
+	spin_unlock_irqrestore(&queue->lock, cpu_flags);
 no_packet:
 	*err = error;
 	return NULL;

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

2015-07-13 08:05 UTC #6 lore raw

In reply to Herbert Xu

Quoted 8 lines · Herbert Xu

The real issue here is that the recv path no longer handles shared skbs. So either we need to fix the recv path to not touch skbs without cloning them, or we need to get rid of the use of shared skbs in netlink.
In fact it looks I introduced the bug way back in
commit a59322be07c964e916d15be3df473fb7ba20c41e

Quoted 8 lines

Author: Herbert Xu <[email protected]>
Date:   Wed Dec 5 01:53:40 2007 -0800
[UDP]: Only increment counter on first peek/recv
I will try to mend this error :)
Cheers,

Herbert, UDP peek support is very buggy anyway, because of deferred checksums

__skb_checksum_complete() will happily manipulate csum, ip_summed, csum_complete_sw & csum_valid

Ideally, peek should never touch skb (but skb->users)

Herbert Xu

2015-07-13 08:10 UTC #7 lore raw

Quoted 8 lines · Eric Dumazet

Herbert, UDP peek support is very buggy anyway, because of deferred checksums
__skb_checksum_complete() will happily manipulate csum, ip_summed, csum_complete_sw & csum_valid
Ideally, peek should never touch skb (but skb->users)

I think UDP should be OK because the main creator of shared skbs is af_packet and in that cast the IP stack will clone the skb upon entry. AFAIK there aren't any entities doing the shared skb trick within the IP stack.

IOW the UDP stack does not have to worry about share skbs, unlike netlink.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

2015-07-13 08:22 UTC #8 lore raw

Quoted 10 lines · Herbert Xu

Quoted 8 lines · Eric Dumazet
Herbert, UDP peek support is very buggy anyway, because of deferred checksums
__skb_checksum_complete() will happily manipulate csum, ip_summed, csum_complete_sw & csum_valid
Ideally, peek should never touch skb (but skb->users)

Quoted 7 lines

I think UDP should be OK because the main creator of shared skbs is af_packet and in that cast the IP stack will clone the skb upon entry. AFAIK there aren't any entities doing the shared skb trick within the IP stack.
IOW the UDP stack does not have to worry about share skbs, unlike netlink.

It should worry, in case multiple threads are using MSG_PEEK on same udp socket ;)

Problem here is not the producer (might be unicast packets btw), but multiple 'consumers'

It turns out your patch would also solve this problem.

Herbert Xu

2015-07-13 08:25 UTC #9 lore raw

Quoted 3 lines · Eric Dumazet

It should worry, in case multiple threads are using MSG_PEEK on same udp socket ;)

That should be fine because we already hold a spinlock on the queue.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

2015-07-13 08:28 UTC #10 lore raw

Quoted 8 lines · Herbert Xu

Quoted 3 lines · Eric Dumazet
It should worry, in case multiple threads are using MSG_PEEK on same udp socket ;)
That should be fine because we already hold a spinlock on the queue.

Except that udp checksum are checked outside of spinlock protection.

Herbert Xu

2015-07-13 08:31 UTC #11 lore raw

Quoted 2 lines · Eric Dumazet

Except that udp checksum are checked outside of spinlock protection.

Good point. I wonder when this got broken. I'll do some digging.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Konstantin Khlebnikov

author

2015-07-13 08:56 UTC #16 lore raw

In reply to Herbert Xu

Quoted 34 lines · Herbert Xu

Quoted 19 lines · Konstantin Khlebnikov
This fixes race between non-atomic updates of adjacent bit-fields:
skb->cloned could be lost because netlink broadcast clones skb after sending it to the first listener who sets skb->peeked at the same skb. As a result atomic refcounting of skb header stays disabled and skb_release_data() frees it twice. Race leads to double-free in kmalloc-xxx.
Signed-off-by Konstantin Khlebnikov <[email protected]>
Fixes b19372273164 ("net: reorganize sk_buff for faster __copy_skb_header()")
---
  net/netlink/af_netlink.c |    6 ++++++
  1 file changed, 6 insertions(+)
net/netlink/af_netlink.c +0 -0
index dea925388a5b..921e0d8dfe3a 100644
@@ -2028,6 +2028,12 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
  	info.tx_filter = filter;
  	info.tx_data = filter_data;
net/netlink/af_netlink.c int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid · line 2039
Quoted 10 lines
net/netlink/af_netlink.c +6 -0
 
+	/* Enable atomic refcounting in skb_release_data() before first send:
+	 * non-atomic set of that bit-field in __skb_clone() could race with
+	 * __skb_recv_datagram() which touches the same set of bit-fields.
+	 */
+	skb->cloned = 1;
+
  	/* While we sleep in clone, do not allow to change socket list */
 
  	netlink_lock_table();
Your effort in finding this bug is wonderful. However I think the fix is a bit dirty.

Quoted 4 lines

The real issue here is that the recv path no longer handles shared skbs. So either we need to fix the recv path to not touch skbs without cloning them, or we need to get rid of the use of shared skbs in netlink.

I don't think that recv path should care about shared skb -- skb can be delivered into only one socket anyway.

Less dirty fix for that: do not send original skb. That adds one extra clone but makes code much cleaner.

net/netlink/af_netlink.c +10 ~~-12~~

@@ -1957,17 +1957,16 @@ static void do_one_broadcast(struct sock *sk,
        }
 
        sock_hold(sk);
-       if (p->skb2 == NULL) {
-               if (skb_shared(p->skb)) {
-                       p->skb2 = skb_clone(p->skb, p->allocation);
-               } else {
-                       p->skb2 = skb_get(p->skb);
-                       /*
-                        * skb ownership may have been set when
-                        * delivered to a previous socket.
-                        */
-                       skb_orphan(p->skb2);
-               }
+       if (p->skb2 == NULL || skb_shared(p->skb2)) {
+               kfree_skb(p->skb2);
+               p->skb2 = skb_clone(p->skb, p->allocation);
+       } else {
+               skb_get(p->skb2);
+               /*
+                * skb ownership may have been set when
+                * delivered to a previous socket.
+                */
+               skb_orphan(p->skb2);
        }
        if (p->skb2 == NULL) {
                netlink_overrun(sk);

@@ -1997,7 +1996,6 @@ static void do_one_broadcast(struct sock *sk,
        } else {
                p->congested |= val;
                p->delivered = 1;
-               p->skb2 = NULL;
        }
 out:
        sock_put(sk);

Quoted 13 lines

In fact it looks I introduced the bug way back in
commit a59322be07c964e916d15be3df473fb7ba20c41e
Author: Herbert Xu <[email protected]>
Date:   Wed Dec 5 01:53:40 2007 -0800
[UDP]: Only increment counter on first peek/recv
I will try to mend this error :)
Cheers,

Signature · 1 line

Konstantin

Herbert Xu

2015-07-13 09:04 UTC #17 lore raw

Quoted 3 lines · Konstantin Khlebnikov

I don't think that recv path should care about shared skb -- skb can be delivered into only one socket anyway.

The fact is that this is how the original code worked and was expected to do. The broadcast side would generate a shared skb and the recv side is supposed to only read it, not modify it.

In fact apart from the skb->peeked bug netlink_recvmsg does all the right things and never modifies skb.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2015-07-13 12:01 UTC #12 lore raw

net: Fix skb csum races when peeking

In reply to Herbert Xu

Quoted 5 lines · Herbert Xu

Quoted 2 lines · Eric Dumazet
Except that udp checksum are checked outside of spinlock protection.
Good point. I wonder when this got broken. I'll do some digging.

OK looks like I can claim credit for this bug too :)

commit fb286bb2990a107009dbf25f6ffebeb7df77f9be

Author: Herbert Xu <[email protected]>
Date:   Thu Nov 10 13:01:24 2005 -0800

[NET]: Detect hardware rx checksum faults correctly

Although others have made the hole bigger more recently.

PS we seem to no longer use the hardware checksum in case of CHECKSUM_COMPLETE, I wonder why that is?

---8<---
When we calculate the checksum on the recv path, we store the result in the skb as an optimisation in case we need the checksum again down the line.

This is in fact bogus for the MSG_PEEK case as this is done without any locking. So multiple threads can peek and then store the result to the same skb, potentially resulting in bogus skb states.

This patch fixes this by only storing the result if the skb is not shared. This preserves the optimisations for the few cases where it can be done safely due to locking or other reasons, e.g., SIOCINQ.

Signed-off-by Herbert Xu <[email protected]>

net/core/datagram.c +9 -6

index b80fb91..4967262 100644
@@ -622,7 +657,8 @@ __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
 		    !skb->csum_complete_sw)
 			netdev_rx_csum_fault(skb->dev);
 	}
-	skb->csum_valid = !sum;
+	if (!skb_shared(skb))
+		skb->csum_valid = !sum;
 	return sum;
 }
 EXPORT_SYMBOL(__skb_checksum_complete_head);

@@ -642,11 +678,13 @@ __sum16 __skb_checksum_complete(struct sk_buff *skb)
 			netdev_rx_csum_fault(skb->dev);
 	}
 
-	/* Save full packet checksum */
-	skb->csum = csum;
-	skb->ip_summed = CHECKSUM_COMPLETE;
-	skb->csum_complete_sw = 1;
-	skb->csum_valid = !sum;
+	if (!skb_shared(skb)) {
+		/* Save full packet checksum */
+		skb->csum = csum;
+		skb->ip_summed = CHECKSUM_COMPLETE;
+		skb->csum_complete_sw = 1;
+		skb->csum_valid = !sum;
+	}
 
 	return sum;
 }

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2015-07-13 14:25 UTC #13 lore raw

Re: net: Fix skb csum races when peeking

Quoted 3 lines · Herbert Xu

PS we seem to no longer use the hardware checksum in case of CHECKSUM_COMPLETE, I wonder why that is?

Nevermind, it's still there. I was just looking in the wrong place.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Dumazet

Acked-by

2015-07-14 06:11 UTC #14 lore raw

Re: net: Fix skb csum races when peeking

In reply to Herbert Xu

Quoted 38 lines · Herbert Xu

---8<---
When we calculate the checksum on the recv path, we store the result in the skb as an optimisation in case we need the checksum again down the line.
This is in fact bogus for the MSG_PEEK case as this is done without any locking. So multiple threads can peek and then store the result to the same skb, potentially resulting in bogus skb states.
This patch fixes this by only storing the result if the skb is not shared. This preserves the optimisations for the few cases where it can be done safely due to locking or other reasons, e.g., SIOCINQ.
Signed-off-by Herbert Xu <[email protected]>
net/core/datagram.c +2 -6
index b80fb91..4967262 100644
@@ -622,7 +657,8 @@ __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len)
 		    !skb->csum_complete_sw)
 			netdev_rx_csum_fault(skb->dev);
 	}
-	skb->csum_valid = !sum;
+	if (!skb_shared(skb))
+		skb->csum_valid = !sum;
 	return sum;
 }
 EXPORT_SYMBOL(__skb_checksum_complete_head);
@@ -642,11 +678,13 @@ __sum16 __skb_checksum_complete(struct sk_buff *skb)
 			netdev_rx_csum_fault(skb->dev);
 	}
 
-	/* Save full packet checksum */
-	skb->csum = csum;
-	skb->ip_summed = CHECKSUM_COMPLETE;
-	skb->csum_complete_sw = 1;
-	skb->csum_valid = !sum;

net/core/datagram.c __sum16 __skb_checksum_complete(struct sk_buff *skb) · line 690

Quoted 10 lines

net/core/datagram.c +7 -0

+	if (!skb_shared(skb)) {
+		/* Save full packet checksum */
+		skb->csum = csum;
+		skb->ip_summed = CHECKSUM_COMPLETE;
+		skb->csum_complete_sw = 1;
+		skb->csum_valid = !sum;
+	}
 
 	return sum;
 }

Acked-by Eric Dumazet <[email protected]>

Thanks !

David Miller

maintainer

2015-07-15 23:13 UTC #5 lore raw

Re: net: Clone skb before setting peeked flag

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Mon, 13 Jul 2015 16:04:13 +0800

Quoted 8 lines

Shared skbs must not be modified and this is crucial for broadcast and/or multicast paths where we use it as an optimisation to avoid unnecessary cloning.
The function skb_recv_datagram breaks this rule by setting peeked without cloning the skb first. This causes funky races which leads to double-free.

Quoted 6 lines

This patch fixes this by cloning the skb and replacing the skb in the list when setting skb->peeked.
Fixes a59322be07c9 ("[UDP]: Only increment counter on first peek/recv")
Reported-by Konstantin Khlebnikov <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Applied and queued up for -stable.

David Miller

maintainer

2015-07-15 23:14 UTC #15 lore raw

Re: net: Fix skb csum races when peeking

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Mon, 13 Jul 2015 20:01:42 +0800

Quoted 8 lines

When we calculate the checksum on the recv path, we store the result in the skb as an optimisation in case we need the checksum again down the line.
This is in fact bogus for the MSG_PEEK case as this is done without any locking. So multiple threads can peek and then store the result to the same skb, potentially resulting in bogus skb states.

Quoted 5 lines

This patch fixes this by only storing the result if the skb is not shared. This preserves the optimisations for the few cases where it can be done safely due to locking or other reasons, e.g., SIOCINQ.
Signed-off-by Herbert Xu <[email protected]>

Also applied and queued up for -stable, thanks!