netdev - Re: [PATCH net] gve: Fix use of netif_carrier

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: <20240802123303.GC2503418@kernel.org>
Date: Fri, 2 Aug 2024 13:33:03 +0100
From: Simon Horman <horms@...nel.org>
To: Praveen Kaligineedi <pkaligineedi@...gle.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, willemb@...gle.com,
	jeroendb@...gle.com, shailend@...gle.com, hramamurthy@...gle.com,
	jfraker@...gle.com
Subject: Re: [PATCH net] gve: Fix use of netif_carrier_ok()

On Thu, Aug 01, 2024 at 01:56:19PM -0700, Praveen Kaligineedi wrote:
> GVE driver wrongly relies on netif_carrier_ok() to check the
> interface administrative state when resources are being
> allocated/deallocated for queue(s). netif_carrier_ok() needs
> to be replaced with netif_running() for all such cases.
> 
> Administrative state is the result of "ip link set dev <dev>
> up/down". It reflects whether the administrator wants to use
> the device for traffic and the corresponding resources have
> been allocated.
> 
> Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues")
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@...gle.com>
> Reviewed-by: Shailend Chand <shailend@...gle.com>
> Reviewed-by: Willem de Bruijn <willemb@...gle.com>

...

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c

...

> @@ -1847,7 +1847,7 @@ int gve_adjust_queues(struct gve_priv *priv,
>  	rx_alloc_cfg.qcfg = &new_rx_config;
>  	tx_alloc_cfg.num_rings = new_tx_config.num_queues;
>  
> -	if (netif_carrier_ok(priv->dev)) {
> +	if (netif_running(priv->dev)) {
>  		err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg);
>  		return err;
>  	}

Hi Praveen,

Not for this patch, but I am curious to know if this check is needed at
all, because gve_adjust_queues only seems to be called from
gve_set_channels if netif_running() (previously netif_carrier_ok()) is true

...