netdev - Re: [PATCH net] bnx2x: bail out if unable to acquire stats

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:	Wed, 4 Sep 2013 13:17:19 -0400
From:	Neal Cardwell <ncardwell@...gle.com>
To:	Dmitry Kravkov <dmitry@...adcom.com>
Cc:	Michal Schmidt <mschmidt@...hat.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Ariel Elior <ariele@...adcom.com>,
	Eilon Greenstein <eilong@...adcom.com>
Subject: Re: [PATCH net] bnx2x: bail out if unable to acquire stats_sema

On Tue, Sep 3, 2013 at 11:51 AM, Dmitry Kravkov <dmitry@...adcom.com> wrote:
>> -----Original Message-----
>> From: Michal Schmidt [mailto:mschmidt@...hat.com]
>> Sent: Tuesday, September 03, 2013 6:46 PM
>> To: davem@...emloft.net
>> Cc: netdev@...r.kernel.org; Dmitry Kravkov; Ariel Elior; Eilon Greenstein
>> Subject: [PATCH net] bnx2x: bail out if unable to acquire stats_sema
>>
>> If we fail to acquire stats_sema in the specified time limit, the chip is
>> probably dead. It probably does not matter whether we try to continue or
>> not, but certainly we should not up() the semaphore afterwards.
>>
>> Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
>> ---
>>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 24
>> +++++++++++++++++------

It seems like this patch has the downside that if the down_timeout()
fails, then bnx2x_stats_handle() ends up updating the stats state
machine's state without really executing the real body of the
action().

In fact it seems like there is a more general pre-existing problem of
this flavor with the bnx2x stats state machine: the
bnx2x_stats_handle() function updates the state machine
bp->stats_state while holding the spin lock, but does not execute the
action() while holding any sort of synchronization, so AFAICT there is
nothing to guarantee that the state machine actions happen in the
order the state machine wants them to happen. For example, if stats
events fire such that we want to execute actions that disable and then
enable stats, we could instead end up executing the actions in the
order that would attempt to enable and then disable them, if we get
unlucky with respect to when interrupts fire, etc.

It seems to me that instead of having all of the callees of
bnx2x_stats_handle() try to down/up the semaphore, instead
bnx2x_stats_handle() should try to down the stats_sema at the top, and
then if successful, it should change the bp->stats_state, call the
action, and up the stats_sema. Would that work?

neal
--
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