netdev - Re: [PATCH net 3/3] r8169: increase the lifespan of the hardware counters dump area.

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:	Sun, 6 Sep 2015 12:37:32 +0200
From:	Corinna Vinschen <vinschen@...hat.com>
To:	romieu@...zoreil.com
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	pomidorabelisima@...il.com
Subject: Re: [PATCH net 3/3] r8169: increase the lifespan of the hardware
 counters dump area.

On Sep  5 14:18, romieu@...zoreil.com wrote:
> From: Francois Romieu <romieu@...zoreil.com>
> 
> net/core/net-sysfs.c::netstat_show retrieves stats with spinlock held.
> 
> This change avoids sleepable allocation and performs some housekeeping:
> - receive ring, transmit ring and counters dump area allocation failures
>   are all considered fatal during open()
> - netif_warn is now redundant with rtl_reset_counters_cond built-in
>   failure message.
> - rtl_reset_counters_cond induced failures in open() are also considered
>   fatal: it takes acceptable work to unwind comfortably.

Why?  The counter reset is not a fatal condition for the operation of
the NIC.  Even if the counters show incorrect values, the normal
operation of the NIC is not affected.  And to top that off, even if
resetting the counters actually doesn't work, the driver keeps the
offset values, so a failed reset won't even harm the statistics.  It
just doesn't make sense to break the NIC operation for such a minor
problem.  And worse:

> -static bool rtl8169_reset_counters(struct net_device *dev)
> +static int rtl8169_reset_counters(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> -	struct rtl8169_counters *counters;
> -	dma_addr_t paddr;
> -	bool ret = true;
>  
>  	/*
>  	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
>  	 * tally counters.
>  	 */
>  	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
> -		return true;
> -
> -	counters = rtl8169_map_counters(dev, &paddr, CounterReset);
> -	if (!counters)
> -		return false;
> -
> -	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
> -		ret = false;
> -
> -	rtl8169_unmap_counters(dev, paddr, counters);
> +		return -EINVAL;

This returns -EINVAL even for older chip versions which are not capable
of resetting the counters.  The result is, this driver will not work at
all on chip versions prior to RTL_GIGA_MAC_VER_19 anymore, because
rtl_open will always fail.

Other then that, I can test the patch probably tomorrow.

Corinna

Content of type "application/pgp-signature" skipped