Re: [PATCH net-next v5 1/2] net: usb: lan78xx: do not update statistics after a failed read

[David Laight <david.laight.linux@gmail.com>]

On Wed, 11 Nov 2026 09:34:53 +0800
Wei Jie Law <law.wei.jie@gt.tech.gov.sg> wrote:

> lan78xx_update_stats() reads the hardware counters into an on-stack
> struct lan78xx_statstage. If lan78xx_read_stats() fails, only the
> rollover check is skipped: the counters are still recomputed from the
> uninitialized stack buffer and stored in dev->stats.curr_stat, which is
> then reported to user space by ethtool -S.
> 
> Keep the previous values when the read fails.
> 
> Found by code review with LLM assistance. Tested together with the next
> patch, which makes short reads fail: with an emulated device, ethtool -S
> then keeps reporting the previous counters.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Wei Jie Law <law.wei.jie@gt.tech.gov.sg>
> ---
>  drivers/net/usb/lan78xx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d73002b2c21a..76c2d98b49df 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -789,11 +789,13 @@ static void lan78xx_update_stats(struct lan78xx_net *dev)
>  
>  	mutex_lock(&dev->stats.access_lock);
>  
> -	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0)
> +	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0) {
>  		lan78xx_check_stat_rollover(dev, &lan78xx_stats);
>  
> -	for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
> -		data[i] = (u64)p[i] + ((u64)count[i] * ((u64)max[i] + 1));
> +		for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
> +			data[i] = (u64)p[i] +
> +				  ((u64)count[i] * ((u64)max[i] + 1));

You only need to promote one of the values to u64.
Even just changing the 1 to 1uLL will do.

That made be look at the related code.

This function has a large on-stack buffer (about 800 bytes from a quick
count). But lan78xx_read_stats() (now) calls kmalloc() - probably because
it cant use an on-stack buffer for the usb transfer.
If you need to call kmalloc() it might as well be for the larger structure.

There are a lot of stats, and two structures (one u32 and on u64) that have
to match, not only that but a lot (but not all) the code ends up treating
the structures as arrays.
It is probably worth just changing them to arrays and using an enum for
the indexes.

Oh - glad to see you fixed (finally) the massive 'truesize' bug.
When I was testing this driver (may years ago, and the product that would
have used it got cancelled) I found a bug with one of the usb3 host
controllers that it only processed one request from one of the control
rings for each 'doorbell' write. The setup code could send them too fast
and it all got very confusing - don't know if that got fixed.
I've actually got the host that failed, but I don't think I've got the
usb ethernet device.

David



> +	}
>  
>  	mutex_unlock(&dev->stats.access_lock);
>