PROBLEM: skb_clone SMP race?

PROBLEM: skb_clone SMP race?

[Santiago Font Arquer]

Hello,
   I'm studying the implementation of sk_buff and I think there's a
possible race condition in skb_clone (2.6.22.9)
   The code is:


struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
{
 struct sk_buff *n;

 n = skb + 1;
 if (skb->fclone == SKB_FCLONE_ORIG &&
    n->fclone == SKB_FCLONE_UNAVAILABLE) {
 atomic_t *fclone_ref = (atomic_t *) (n + 1);
 n->fclone = SKB_FCLONE_CLONE;
 atomic_inc(fclone_ref);
 } else {
 n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 if (!n)
  return NULL;
 n->fclone = SKB_FCLONE_UNAVAILABLE;
 }

   If an skb with fast clone available (first "if" true) has
references in different CPUs (skb->users>1) (I do not find explicit
checks for this to be impossible), if skb_clone is called
simultaneously over that skb, both callers can get the same clone (the
"fast" clone) and different problems follow: wrong "clone_skb->users"
(1 as expected by the caller, but it should be, to be true, 2),
fclone_ref set to 3 involving further problems, ...

  IMO, the same problem arises although the calls to skb_clone are
not simultaneous: there isn´t a memory barrier after the change of
"n->fclone" to guarantee the visibility of that change to other CPUs
(but that barrier will not solve anything; I mentioned this only to
reflect another reason I see for the race to happen).

  Is that correct? Thank you in advance.


       Santiago Font Arquer

Re: PROBLEM: skb_clone SMP race?

[Herbert Xu]

Santiago Font Arquer <sfa.linux@gmail.com> wrote:
>
>   If an skb with fast clone available (first "if" true) has
> references in different CPUs (skb->users>1) (I do not find explicit
> checks for this to be impossible), if skb_clone is called
> simultaneously over that skb, both callers can get the same clone (the
> "fast" clone) and different problems follow: wrong "clone_skb->users"
> (1 as expected by the caller, but it should be, to be true, 2),
> fclone_ref set to 3 involving further problems, ...

Fast clones are only used by TCP where the original skb is
never given to the outside world.  This plus the fact that
a given TCP socket is single-threaded makes it safe.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PROBLEM: skb_clone SMP race?

[Santiago Font Arquer]

Thank you.
I was thinking about something like that when I said "not find
explicit checks", but I was confused because the sk_buff code is
plenty of explicit checks and I understood those checks as a try to
make the code self-protected against wrong uses: "I, sk_buff, offer
you this interface. Don´t worry too much about my internals: I will
complain on your mistakes"

The fact the name of the functionality is generic "fast_clone" and not
specific "tcp_fast_clone" drove me to think: "Ok, the allocation of
fast-clonable skbs is NOW only in ONE place inside TCP code and sure
that they have the implicit checks to avoid this race when they call
skb_clone, but -NOW- and -ONE- are not a big guarantee..."

A newbie's thought. :)



2007/10/11, Herbert Xu <herbert@gondor.apana.org.au>:
> Santiago Font Arquer <sfa.linux@gmail.com> wrote:
> >
> >   If an skb with fast clone available (first "if" true) has
> > references in different CPUs (skb->users>1) (I do not find explicit
> > checks for this to be impossible), if skb_clone is called
> > simultaneously over that skb, both callers can get the same clone (the
> > "fast" clone) and different problems follow: wrong "clone_skb->users"
> > (1 as expected by the caller, but it should be, to be true, 2),
> > fclone_ref set to 3 involving further problems, ...
>
> Fast clones are only used by TCP where the original skb is
> never given to the outside world.  This plus the fact that
> a given TCP socket is single-threaded makes it safe.
>
> Cheers,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>