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:	Fri, 6 Sep 2013 09:40:29 +0300
From:	Dmitry Kravkov <dkravkov@...il.com>
To:	Neal Cardwell <ncardwell@...gle.com>
Cc:	Dmitry Kravkov <dmitry@...adcom.com>,
	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 Wed, Sep 4, 2013 at 8:17 PM, Neal Cardwell <ncardwell@...gle.com> wrote:
> 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?
>
handle() is called from sleepable context (open/close) and timer
context, then it's not possible to use semaphore for pretection

> 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



-- 
Best regards,
Dmitry
--
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