Message-ID: <20110822181420.GD12337@hmsreliant.think-freely.org>
Date:	Mon, 22 Aug 2011 14:14:20 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in
 ndo_start_xmit methods (v3)

On Mon, Aug 22, 2011 at 05:17:37PM +0100, Ben Hutchings wrote:
> On Sun, 2011-08-21 at 20:27 -0400, Neil Horman wrote:
> > On Wed, Aug 17, 2011 at 04:07:17PM +0100, Ben Hutchings wrote:
> > > On Tue, 2011-07-26 at 12:05 -0400, Neil Horman wrote:
> > > > Ok, after considering all your comments, Dave suggested this as an alternate
> > > > approach:
> > > > 
> > > > 1) We create a new priv_flag, IFF_SKB_TX_SHARED, to identify drivers capable of
> > > > handling shared skbs.  Default is to not set this flag
> > > > 
> > > > 2) Modify ether_setup to enable this flag, under the assumption that any driver
> > > > calling this  function is initalizing a real ethernet device and as such can
> > > > handle shared skbs since they don't tend to store state in the skb struct.
> > > > Pktgen can then query this flag when a user script attempts to issue the
> > > > clone_skb command and decide if it is to be alowed or not.
> > > [...]
> > > 
> > > A bunch of Ethernet drivers do skb_pad() or skb_padto() in their
> > > ndo_start_xmit implementations, either to avoid hardware bugs or because
> > > the MAC doesn't automatically pad to the minimum frame length.  This
> > > presumably means they can't generally handle shared skbs, though in the
> > > specific case of pktgen it should be safe as long as a single skb is not
> > > submitted by multiple threads at once.
> > > 
> > Agreed, given that pktgen is doing skb sharing in a serialized manner (i.e. one
> > thread of execution increasing skb->users rather than in multiple threads), the
> > skb_pad[to] cases are safe.  Are there cases in which shared skbs are
> > transmitted in parallel threads that we need to check for?
> 
> Not that I'm aware of.  However, you have defined the flag to mean 'safe
> to send shared skbs', and this is not generally true for those drivers.
> 
> So please decide whether:
> a. The flag means 'safe to send shared skbs' (i.e. the TX path does not
> modify the skb) and drivers which pad must clear it on their devices.
> b. The flag means 'safe to send shared skbs in the way ptkgen does', and
> the restrictions this places on the user and driver should be specified.
> 

The flag means safe to send shared skbs.

Actually looking closer at this, I don't think this is much of a problem at all.
The flag is cleared on devices for which it is unsafe to send shared skbs, not
because there are multiple users of the skb in parallel, but because the driver
expects to have continued exclusive access to the skb after the drivers
ndo_start_xmit method returns.

In the pktgen case, skbs have skb->users > 1, but all users exist in the same
execution context, making their transmission serialized.

In the additional cases you are concerned with, multiple users of an skb may or
may not run in parallel.  In the parallel case however, to avoid additional
races, one of the following must be true:

1) All n contexts treat the skb as immutible and wind up sending to the same
device and queue, at which point the xmit_lock in the net_device serializes
their operation.

2) Each of the n contexts (or a subset thereof) will modify the skb data, which
requires that they do their own locking between each other, which must encompass
the transmission of the skb in each context (lest they corrupt the skb during
transmission).  Such locking again serializes the transmit peth.

In either case, access to the skb is serialized such that it is only ever
handled by one driver at one time, and during that window drivers may opt to
modify the state of the skb, as long as they dont' expect the sate to remain in
their control after the return from ndo_start_xmit.


So even though drivers that call skb_pad[to] modify the skb, its perfectly ok to
do so, as long as they don't assume that the struct skb will remain in that
state after the driver is done with it.

This works out perfectly well for skb_padto calls, since the function reduces to
a no-op after the minimal tail size has been reached.

Its a bit more problematic for skb_pad, since the possibility exists for
successive calls to skb_pad to iteratively increase the pad until the skb
tailroom is exhausted, causing a transmission error.  That said however, there
are only 6 drivers that use skb_pad (sfx, niu, rt2800, rt61pci, rt37usb, and
claw), and they all only pad if the skb is not already a minimum length.  These
calls should probably be audited and converted to skb_padto anyway.

So, in my view, these aren't really problems.  If someone just does a blind skb
pad in the driver, then we have something of a problem, but one that is easily
fixed if and when the problem should arise.
Neil

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

Powered by blists - more mailing lists