Re: [PATCH] arp_queue: serializing unlink + kfree_skb

David S. Miller
2005-02-03T01:04:07Z

On Mon, 31 Jan 2005 22:33:26 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> We're using atomic integers to signal that we're done with an object.
> The object is usually represented by a piece of memory.
>
> The problem is that in most of the places where we do this (and that's
> not just in the networking systems), there are no memory barriers between
> the last reference to that object and the decrease on the atomic counter.

I agree.

> if (atomic_read(&skb->users) != 1) {
> smp_mb__before_atomic_dec();
> if (!atomic_dec_and_test(&skb->users))
> return;
> }
> __kfree_skb(skb);

This looks good. Olaf can you possibly ask the reproducer if
this patch makes the ARP problem go away?

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2005/02/02 15:53:55-08:00 herbert@gondor.apana.org.au
# [NET]: Add missing memory barrier to SKB release.
#
# Here, we are using atomic counters to signal that we are done
# with an object. The object is usually represented by a piece
# of memory.
#
# The problem is that in most of the places we do this, there are
# no memory barriers between the last reference to that object
# and the decrease on the atomic counter.
#
# Based upon a race spotted in arp_queue handling by Olaf Kirch.
#
# Signed-off-by: David S. Miller <davem@davemloft.net>
#
# include/linux/skbuff.h
# 2005/02/02 15:51:57-08:00 herbert@gondor.apana.org.au +6 -2
# [NET]: Add missing memory barrier to SKB release.
#
diff -Nru a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h 2005-02-02 15:54:13 -08:00
+++ b/include/linux/skbuff.h 2005-02-02 15:54:13 -08:00
@@ -353,8 +353,12 @@
*/
static inline void kfree_skb(struct sk_buff *skb)
{
- if (atomic_read(&skb->users) == 1 || atomic_dec_and_test(&skb->users))
- __kfree_skb(skb);
+ if (atomic_read(&skb->users) != 1) {
+ smp_mb__before_atomic_dec();
+ if (!atomic_dec_and_test(&skb->users))
+ return;
+ }
+ __kfree_skb(skb);
}

/* Use this if you didn't touch the skb state [for fast switching] */