Re: [PATCH] niu: Fix races between up/down and get_stats.

[Flavio Leitner <fleitner@redhat.com>]

On Thu, Feb 03, 2011 at 04:25:29PM -0800, David Miller wrote:
> 
> As reported by Flavio Leitner, there is no synchronization to protect
> NIU's get_stats method from seeing a NULL pointer in either
> np->rx_rings or np->tx_rings.  In fact, as far as ->ndo_get_stats
> is concerned, these values are set completely asynchronously.
> 
> Flavio attempted to fix this using a RW semaphore, which in fact
> works most of the time.  However, dev_get_stats() can be invoked
> from non-sleepable contexts in some cases, so this fix doesn't
> work in all cases.
> 
> So instead, control the visibility of the np->{rx,tx}_ring pointers
> when the device is being brough up, and use properties of the device
> down sequence to our advantage.
> 
> In niu_get_stats(), return immediately if netif_running() is false.
> The device shutdown sequence first marks the device as not running (by
> clearing the __LINK_STATE_START bit), then it performans a
> synchronize_rcu() (in dev_deactive_many()), and then finally it
> invokes the driver ->ndo_stop() method.
> 
> This guarentees that all invocations of niu_get_stats() either see
> netif_running() as false, or they see the channel pointers before
> ->ndo_stop() clears them out.
> 
> If netif_running() is true, protect against startup races by loading
> the np->{rx,tx}_rings pointer into a local variable, and punting if
> it is NULL.  Use ACCESS_ONCE to prevent the compiler from reloading
> the pointer on us.
> 
> Also, during open, control the order in which the pointers and the
> ring counts become visible globally using SMP write memory barriers.
> We make sure the np->num_{rx,tx}_rings value is stable and visible
> before np->{rx,tx}_rings is.
> 
> Such visibility control is not necessary on the niu_free_channels()
> side because of the RCU sequencing that happens during device down as
> described above.  We are always guarenteed that all niu_get_stats
> calls are finished, or will see netif_running() false, by the time
> ->ndo_stop is invoked.
> 
> Reported-by: Flavio Leitner <fleitner@redhat.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>

nice patch, clever
I got positive feedback on my patch. I'll ask for this patch as well.
thanks,
-- 
Flavio