netdev - Re: [PATCH] niu: Fix races between up/down and get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 4 Feb 2011 14:26:46 -0200
From:	Flavio Leitner <fleitner@...hat.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] niu: Fix races between up/down and get_stats.

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@...hat.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>

nice patch, clever
I got positive feedback on my patch. I'll ask for this patch as well.
thanks,
-- 
Flavio
--
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