netdev - Re: [PATCH] bnx2x: fix occasional statistics off-by-4GB error

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:	Mon, 18 Mar 2013 12:06:22 +0200
From:	"Eilon Greenstein" <eilong@...adcom.com>
To:	"Maciej Żenczykowski" <maze@...gle.com>
cc:	"Eric Dumazet" <eric.dumazet@...il.com>,
	"David Miller" <davem@...emloft.net>,
	"Dmitry Kravkov" <dmitry@...adcom.com>,
	"Linux NetDev" <netdev@...r.kernel.org>,
	"Mintz Yuval" <yuvalmin@...adcom.com>
Subject: Re: [PATCH] bnx2x: fix occasional statistics off-by-4GB error

On Sun, 2013-03-17 at 23:18 -0700, Maciej Żenczykowski wrote:
> This has nothing to do with atomicity or races or anything.
> 
> Look at the ADD_64 macro (which implements a += b) and compare it to
> this one (which implents a = b + c).
> 
> It simply doesn't know how to correctly add in any situation in which
> "qstats_old->t..._lo != 0".
> By this point all the stats are already in software and both firmware
> and hardware is not relevant.
> 
> Normally qstats_old starts off at 0 and the bug isn't visible (since
> with a value of 0, there can't be overflow and carry propagation is
> not needed).
> 
> The reason qstats_old was introduced was to allow for a partial nic
> reset [or whatever the proper term is] which resets fw/hw provided
> stats to not reset sw exported stats.
> During the reset old stats are copied into qstats_old, and in the
> future stats are returned as "qstats_old + current stats from fw/hw"
> except this addition is buggy and only works if qstats_old happens to
> have lower 32-bits zero (ie. basically only before first partial nic
> reset).
> 
> Imagine:
> (a) driver initializes, nic gets brought up, qstats_old = 0, stats
> from device = 0
> (b) 2GB of xfer happens
> (c) qstats_old still = 0, stats from device = 2GB
> (d) addition happens qstats = qstats_old + stats_from_dev = 0 + 2GB =
> 2GB   --- we report 2GB xfer
> (e) something partially resets nic (say mtu change), qstats_old = 2GB,
> stats from device = 0
> (f) 2GB-1 of xfer happens
> (g) qstats_old = 2GB, stats from device = 2GB-1
> (h) addition happens qstats = qstats_old + stats_from_dev = 2GB +
> 2GB-1 = 4GB-1   --- we report 4GB-1 xfer
> (i) 1 byte of xfer happens
> (j) qstats_old = 2GB, stats from device = 2GB
> (k) addition happens qstats = qstats_old + stats_from_dev = 2GB + 2GB
> = 0   --- we report 0 xfer
> (l) 2GB of xfer happens
> (m) qstats_old = 2GB, stats from device = 4GB
> (n) addition happens qstats = qstats_old + stats_from_dev = 2GB + 4GB
> = 6GB   --- we report 6GB xfer
> 
> And yes the macro in question when asked to do 2GB+2GB will return 0.
> 
> (btw. at least on the driver I tested there's still some weirdness
> with mtu change not-resetting per-queue stats but resetting
> coalesced-from-all-queues stats)
> 
> Now, to be fair, I don't yet have confirmation that my patch fixes the
> problem our SREs saw in deployment, but I expect to have that soon.
> (and even if it doesn't fix the problem, this code is still buggy and
> should be fixed)
> 
> Maciej

Maciej - thanks for the detailed information. You are right - it has
nothing to do with the HW/FW and it is simply a bug that needs to be
fixed. I withdraw my objections and add my ACK.

Acked-by: Eilon Greenstein <eilong@...adcom.com>





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