Re: [PATCH 2/2] [PATCH] ucc_geth: fix for RX skb buffers recycling

[David Miller <davem@davemloft.net>]

From: Sergey Matyukevich <geomatsi@gmail.com>
Date: Mon,  7 Jun 2010 22:38:14 +0400

> This patch implements a proper recycling of skb buffers belonging to RX error
> path. The suggested fix actually follows the recycling scheme implemented for
> TX skb buffers in the same driver (see 'ucc_geth_tx' function): skb buffers
> are checked by 'skb_recycle_check' function and deleted if can't be recycled.
> 
> This problem in recycling of skb buffers was discovered by accident in a setup
> when ethernet interface on one link end was full-duplex while another was
> half-duplex. In this case numerous corrupted frames were received by
> full-duplex interface due to late collisions. RX skb buffers with error
> frames were not properly recycled, that is why overflow occured from time to
> time on the next use of those buffers. Here is example of crush dump:

The lack of skb_recycle_check() is not the true cause of this bug.
You should never, ever, need to make skb_recycle_check() tests on
packets in this situation.  Once the skb pointers are properly adjusted
it will have sufficient room.

And that points to what the real problem is, the problem is the
skb->data assignment.  It's trying to get the SKB data pointers back
into the same state they are in when dev_alloc_skb() returns a packet
buffer.

But this assignment isn't accomplishing that, in fact it's corrupting
the SKB because after adjusting skb->data, skb->tail and skb->len will
become incorrect.  And this is what you need to fix.

That's why you get the skb_put() over panics, not because you lack
a skb_recycle_check() call here.

In fact, what your patch makes happen is that the error packets will
never get recycled.  The skb_recycle_check() will always fail.

Please fix this bug properly by correctly restoring the SKB pointers
and lengths to their initial state, then you can retain the
unconditional queueing of the error packet onto the recycle list.

Once you do that, all of the checks done by skb_recycle_check() are
superfluous and will always pass, and we know this.  The buffer is
not fragmented, there aren't any clones or external references to it,
and once you fix up the data pointers properly it will have enough
room as necessary for the RX buffer size the driver is currently using.

There are numerous helper routines in linux/skbuff.h that can be used
to do this properly, which will adjust a pointer and make the
corresponding adjustment to skb->len as well when necessary.