OOP in ip_cmsg_recv (net-next)

Le lundi 03 mai 2010 à 15:23 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 03 May 2010 19:21:09 +0200
> 
> >  
> > -	/* skb is now orphaned, might be freed outside of locked section */
> > -	consume_skb(skb);
> > +	/* skb is now orphaned, can be freed outside of locked section */
> > +	__kfree_skb(skb);
> >  }
> >  EXPORT_SYMBOL(skb_free_datagram_locked);
> 
> Eric, if you do this you undo the utility of the SKB packet drop tracing
> that Neil wrote.
> 
> consome_skb() says that the application actually took in the packet and
> we didn't drop it due to some error or similar.
> 
> Whereas __kfree_skb() is going to be tagged as a packet drop and the
> data didn't reach the application.
> 
> So if you need to use __kfree_skb() to fix this you'll need to somehow
> add some appropriate annotations for the tracer.  Perhaps add a
> __consume_skb() that is marked for the tracing stuff and does what
> you need.
> --

David, if I am not mistaken (not thea yet for me this early morning) the
tracer you mention is included in kfree_skb(), not in __kfree_skb() :

void kfree_skb(struct sk_buff *skb)
{
        if (unlikely(!skb))
                return;
        if (likely(atomic_read(&skb->users) == 1))
                smp_rmb();
        else if (likely(!atomic_dec_and_test(&skb->users)))
                return;
        trace_kfree_skb(skb, __builtin_return_address(0));
        __kfree_skb(skb);
}
EXPORT_SYMBOL(kfree_skb);



I only copied part of consume_skb() which doesnt call
trace_kfree_skb() :

void consume_skb(struct sk_buff *skb)
{
        if (unlikely(!skb))
                return;
        if (likely(atomic_read(&skb->users) == 1))
                smp_rmb();
        else if (likely(!atomic_dec_and_test(&skb->users)))
                return;
        __kfree_skb(skb);
}
EXPORT_SYMBOL(consume_skb);


So I believe my second patch is a bit better : We dont even lock the
socket in the (rare) case we should not orphan the skb ;)

We keep the two slab calls outside of sock lock, so we keep sock locked
for a very very short time period (remember we now use lock_sock_bh() :
producers now might spin on the lock instead of queueing packet in
backlog)

Thanks !

[PATCH net-next-2.6] net: skb_free_datagram_locked() fix

Commit 4b0b72f7dd617b ( net: speedup udp receive path )
introduced a bug in skb_free_datagram_locked().

We should not skb_orphan() skb if we dont have the guarantee we are the
last skb user, this might happen with MSG_PEEK concurrent users.

To keep socket locked for the smallest period of time, we split
consume_skb() logic, inlined in skb_free_datagram_locked()

Reported-by: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/datagram.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 04 May 2010 06:43:45 +0200

> David, if I am not mistaken (not thea yet for me this early morning) the
> tracer you mention is included in kfree_skb(), not in __kfree_skb() :
 ...
> I only copied part of consume_skb() which doesnt call
> trace_kfree_skb() :
 ...
> So I believe my second patch is a bit better : We dont even lock the
> socket in the (rare) case we should not orphan the skb ;)

Right you are.

> [PATCH net-next-2.6] net: skb_free_datagram_locked() fix

I'll apply this, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html