Re: [PATCH net v3 4/4] gve: Fix link flap after ethtool ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Synchronous link-status update in gve_queues_start() races
  with gve_service_task(); the carrier can be left in the wrong state.

--

gve: Fix link flap after ethtool ops

gve_queues_start() now calls gve_turnup_and_check_status() directly
instead of queueing the service task, so the carrier state is updated
before the call returns to userspace. This avoids the transient carrier
drop after ethtool reconfiguration that was breaking the
drivers/net/ping.py selftest.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 52c152a986eb7..e2a016f4430a8 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -1486,8 +1515,7 @@ static int gve_queues_start(struct gve_priv *priv,
>  			  round_jiffies(jiffies +
>  				msecs_to_jiffies(priv->stats_report_timer_period)));
>  
> -	gve_turnup(priv);
> -	queue_work(priv->gve_wq, &priv->service_task);
> +	gve_turnup_and_check_status(priv);

[Severity: Medium]
Can this race with gve_service_task() and leave the carrier in the wrong
state?

Before this change, gve_queues_start() sent its link update through
service_task. A work item never runs concurrently with itself, so these
updates were serialized.

gve_queues_start() is reached from ndo_open, from gve_adjust_config(),
and from gve_reset()->gve_recover()->gve_open(). It holds rtnl and the
netdev instance lock, and it now runs the same sequence as
gve_service_task().

gve_service_task() is queued from gve_mgmnt_intr() and holds neither
lock:

gve_service_task() {
    ...
	gve_handle_reset(priv);
	ops->report_link_status(priv);
	gve_handle_link_status(priv);
}

gve_adminq_report_link_status() reads the register and writes the
cached value in two separate steps:

	status = ioread32be(&priv->reg_bar0->device_status);
	WRITE_ONCE(priv->link_up,
		   !!(GVE_DEVICE_STATUS_LINK_STATUS_MASK & status));

Here is one possible interleaving:

gve_queues_start()                  gve_service_task()
  ioread32be() -> link down
                                    (device link up, mgmt irq)
                                    ioread32be() -> link up
                                    WRITE_ONCE(link_up, true)
                                    netif_carrier_on()
  WRITE_ONCE(link_up, false)
  netif_carrier_off()

At the end the carrier is off while the device link is up. The service
task run triggered by the interrupt has already finished, so nothing
checks the link again until the device raises another status interrupt.

gve_handle_link_status() has a similar window, because its check and
its action are not done under a lock:

	link_up = READ_ONCE(priv->link_up);
	if (link_up == netif_carrier_ok(priv->dev))
		return;

	if (link_up) {
		netdev_info(priv->dev, "Device link is up.\n");
		netif_carrier_on(priv->dev);

Suppose gve_queues_start() reads link_up == true with the carrier off,
and is then delayed in netdev_info(). In that time gve_service_task()
can write link_up = false, see that it matches netif_carrier_ok(), and
return. gve_queues_start() then calls netif_carrier_on() while the link
is down.

gve_set_xdp() and the per-queue start path already use the same
pattern without a lock. This patch also brings it to the common
gve_queues_start() path, which used to be serialized through the
service task.

Should gve_turnup_and_check_status() and gve_service_task() be
serialized against each other here?

>  	priv->interface_up_cnt++;
>  	return 0;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023204706.3112215-1-hramamurthy%40google.com