netdev - Re: [PATCH net-next 2/4] ixgbe: aggregate of all receive errors through netdev's rx

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: <20210409180008.1f23bb7e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 9 Apr 2021 18:00:08 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, Radoslaw Tyl <radoslawx.tyl@...el.com>,
        netdev@...r.kernel.org, sassmann@...hat.com,
        Tony Brelinski <tonyx.brelinski@...el.com>
Subject: Re: [PATCH net-next 2/4] ixgbe: aggregate of all receive errors
 through netdev's rx_errors

On Fri,  9 Apr 2021 12:03:12 -0700 Tony Nguyen wrote:
> From: Radoslaw Tyl <radoslawx.tyl@...el.com>
> 
> The global rx error does not take into account all the error counters
> that are counted by device.
> 
> Extend rx error with the following counters:
> - illegal byte error
> - number of receive fragment errors
> - receive jabber
> - receive oversize error
> - receive undersize error
> - frames marked as checksum invalid by hardware
> 
> The above were added in order to align statistics with other products.
> 
> Signed-off-by: Radoslaw Tyl <radoslawx.tyl@...el.com>
> Tested-by: Tony Brelinski <tonyx.brelinski@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 7ba1c2985ef7..7711828401d9 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7240,12 +7240,21 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
>  	hwstats->ptc1023 += IXGBE_READ_REG(hw, IXGBE_PTC1023);
>  	hwstats->ptc1522 += IXGBE_READ_REG(hw, IXGBE_PTC1522);
>  	hwstats->bptc += IXGBE_READ_REG(hw, IXGBE_BPTC);
> +	hwstats->illerrc += IXGBE_READ_REG(hw, IXGBE_ILLERRC);
>  
>  	/* Fill out the OS statistics structure */
>  	netdev->stats.multicast = hwstats->mprc;
>  
>  	/* Rx Errors */
> -	netdev->stats.rx_errors = hwstats->crcerrs + hwstats->rlec;
> +	netdev->stats.rx_errors = hwstats->crcerrs +
> +				    hwstats->illerrc +
> +				    hwstats->rlec +
> +				    hwstats->rfc +
> +				    hwstats->rjc +
> +				    hwstats->roc +
> +				    hwstats->ruc +

IDK what the HW counts exactly but perhaps rlec includes other
counters? Note that the stats you add with this patch are RFC 2819 /
RMON counters, and AFAIU they overlap with IEEE counters.

If the RMON counters are somehow exclusively counting their events you
should update rx_length_errors as well.

> +				    hw_csum_rx_error;

AFAICT this is incorrect L4 csum, that's not supposed be counted as NIC
rx_error. Let the appropriate protocol code check this and increment
its own counter.

>  	netdev->stats.rx_dropped = 0;
>  	netdev->stats.rx_length_errors = hwstats->rlec;
>  	netdev->stats.rx_crc_errors = hwstats->crcerrs;