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]

Message-ID: <20110211172952.GA3112@redhat.com>
Date:	Fri, 11 Feb 2011 15:29:52 -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 Fri, Feb 04, 2011 at 02:26:46PM -0200, Flavio Leitner wrote:
> 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.

Got a feedback that your patch works out too.
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