netdev - RE: [PATCH v2 net] bnx2x: Fix statistics locking scheme

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: <B5657A6538887040AD3A81F1008BEC63C1401F@avmb3.qlogic.org>
Date:	Thu, 19 Mar 2015 07:26:25 +0000
From:	Yuval Mintz <Yuval.Mintz@...gic.com>
To:	David Miller <davem@...emloft.net>
CC:	netdev <netdev@...r.kernel.org>,
	Ariel Elior <Ariel.Elior@...gic.com>
Subject: RE: [PATCH v2 net] bnx2x: Fix statistics locking scheme

> > +int bnx2x_stats_safe_exec(struct bnx2x *bp,
> > +			  void (func_to_exec)(void *cookie),
> > +			  void *cookie)
>  ...
> > +	if (bp->stats_pending) {
> > +		BNX2X_ERR("Failed to wait for stats pending to clear\n");
> > +		return -EBUSY;
> 
> Buggy, this returns with the stats_lock still held.

Ouch; You're right - I'll fix it.

> 
> In my humble opinion, you are putting very little care and effort into this bug fix.

Well, we've put quite a bit in the logic behind it; But you're probably right
About the effort put into v2.

> Also I disagree with your timeout logic.
> 
> I suspect that the real reason these timeouts are present, is that the locking
> hierarchy is uncertain.
> 
> Nothing really should create the situation those trylocks seem to be dealing
> with.

Actually, you're probably right in the assumption - I have a feeling that due to
the races in the previous implementation, there was lack of confidence in the
general process.
According to my current analysis there shouldn't be any real need for the limit
on time, since there are no limitless loops taking place while the locks are being
held [at most, HW/FW is asserted and nothing happens; But it still should take
only a couple of mili-seconds].
The only reason I've left this was to leave the implementation close as I can to
original [which used that locking mechanism] given that it's intended to `net'.
Do you prefer I simply switch over into using a regular mutex?
--
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