netdev - Re: [PATCH net-next] ehea: Remove sleep at .ndo_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: <1316704497.29549.10.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
Date:	Thu, 22 Sep 2011 17:14:57 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	brenohl@...ibm.com
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Breno Leitao <leitao@...ux.vnet.ibm.com>
Subject: Re: [PATCH net-next] ehea: Remove sleep at .ndo_get_stats

Le jeudi 22 septembre 2011 à 11:10 -0300, brenohl@...ibm.com a écrit :
> Currently ehea ndo_get_stats can sleep in two places, in a hcall
> and in a GFP_KERNEL alloc, which is not correct.
> This patch creates a workqueue that grabs the information from time
> to time from the hardware, and place it into the device structure,
> so that, .ndo_get_stats quickly returns the device structure statistics
> block.
> 
> Signed-off-by: Breno Leitao <leitao@...ux.vnet.ibm.com>
> ---

Hmm... trigerring a work queue everytime NAPI is run ? That seems very
expensive to me, if for example no one wants device stats at all.

I read ehea correctly, this blocking suff is only used to compute
stats->multicast and stats->rx_errors. 

They probably could be updated once per second, nobody will complain.
(So use a workqueue, and trigger the job once per second, not once per
incoming packet)

Also, you dont provide a safe ndo_get_stats() implementation.

Think that several process might read your device stats in //

So the memset(stats, 0, sizeof(*stats)) is wrong :
You can clear stats that are currently read by another thread.

This makes SNMP readers read intermediate null values, and they dont
like this.

Fix is real easy : remove the memset(stats, 0, sizeof(*stats)) :

You only write on some fields, other fields are already 0

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