Re: [PATCH net-next v5 1/9] ibmveth: fix netpoll races with RX replenish

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · 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] napi_disable() waits only for ownership (SCHED/NPSVC) to be
  released, not for the ibmveth_poll() callback to return.

Pre-existing issues:
- [High] Not introduced by this patch: ibmveth_remove() calls
  cancel_work_sync(&adapter->work)…
- [High] Not introduced by this patch: ibmveth_rx_mss_helper() trusts the
  IPv4 header of a received frame.

--

ibmveth: fix netpoll races with RX replenish

This patch removes ibmveth_poll_controller(), so netpoll now polls NAPI
directly with budget 0. ibmveth_poll() skips RX replenish when budget is
0. ibmveth_open() now enables NAPI just before request_irq(), after the
RX resources exist.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index abebdb1fc2628..4a5869183be66 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -717,10 +715,18 @@ static int ibmveth_open(struct net_device *netdev)
>  		}
>  	}
>  
> +	/* NAPI can run as soon as it is enabled, from netpoll during the
> +	 * direct close()/open() pairs or from a direct ibmveth_interrupt()
> +	 * call, so enable it only once everything ibmveth_poll() touches
> +	 * exists.
> +	 */
> +	napi_enable(&adapter->napi);
> +
>  	netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
>  	rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
>  			 netdev);
>  	if (rc != 0) {
> +		napi_disable(&adapter->napi);
>  		netdev_err(netdev, "unable to request irq 0x%x, rc %d\n",
>  			   netdev->irq, rc);
>  		goto out_free_buffer_pools;

[Severity: Medium]
Is napi_disable() enough here before the goto frees the RX queue?

napi_disable() waits for the NAPI ownership bits to clear. It does not
wait for ibmveth_poll() to return. After napi_complete_done(), the poll
keeps running:

ibmveth_poll()
    napi_complete_done(napi, frames_processed)
    h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_ENABLE)
    ibmveth_rxq_pending_buffer(adapter)   <- reads rx_queue.queue_addr

The out_free_buffer_pools path then reaches dma_free_coherent() on
adapter->rx_queue.queue_addr. ibmveth_close() has the same gap between
napi_disable() and its dma_free_coherent() of the RX queue.

A poll can run in this window. netpoll can poll NAPI once it is
enabled, and veth_pool_store() kicks the interrupt handler after
dropping RTNL:

veth_pool_store()
    rtnl_unlock();

    /* kick the interrupt handler to allocate/deallocate pools */
    ibmveth_interrupt(netdev->irq, netdev);

Can the end of the poll read the freed RX queue, or re-enable the
interrupt after close() has masked it?

The later commit "ibmveth: wait for the RX poll to return before
freeing the RX queue" in this series seems to fix this. It adds
synchronize_net() after napi_disable() in ibmveth_close() and in this
request_irq() failure path. The new comment says this patch hardens the
direct close()/open() window. Could that change be folded in here?

[ ... ]

> @@ -1542,7 +1547,11 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  		}
>  	}
>  
> -	ibmveth_replenish_task(adapter);
> +	/* netpoll polls with budget 0 for TX only, and is not serialized
> +	 * with a NAPI poll that was already running when it was set up
> +	 */
> +	if (budget)
> +		ibmveth_replenish_task(adapter);
>  
>  	if (frames_processed == budget)
>  		goto out;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The
RX loop in ibmveth_poll() calls ibmveth_rx_mss_helper() whenever the
IPv4 checksum field is 0xffff, whatever csum_good says:

    if ((length > netdev->mtu + ETH_HLEN) ||
        lrg_pkt || iph_check == 0xffff) {
            ibmveth_rx_mss_helper(skb, mss, lrg_pkt);

The helper uses iph->ihl * 4 as the TCP offset. It does not check
skb_headlen(), ihl >= 5, or fragmentation:

ibmveth_rx_mss_helper()
    offset = iph->ihl * 4;
    ...
    tcph = (struct tcphdr *)(skb->data + offset);
    ...
        skb_shinfo(skb)->gso_size = ntohs(tcph->check);
        tcph->check = 0;
    ...
        hdr_len = offset + tcph->doff * 4;
        skb_shinfo(skb)->gso_segs =
                DIV_ROUND_UP(skb->len - hdr_len,
                             skb_shinfo(skb)->gso_size);

Can a short IPv4/TCP frame from another partition on the virtual LAN,
with check set to 0xffff and a large ihl, make this read and write
tcph->check past the received data? On the copybreak path, those bytes
are uninitialized skb memory.

With an oversized doff, can skb->len - hdr_len underflow and put bogus
gso_segs into the skb passed up the stack?

The later commit "ibmveth: check headers in the RX large send and
checksum helpers" in this series adds skb_headlen(), ihl, fragment,
doff and ip_len checks to ibmveth_rx_mss_helper() before it touches the
headers. That looks like it fixes this.

[Severity: High]
This isn't a bug introduced by this patch, but further down,
ibmveth_poll() can still queue the reset work when re-enabling the
interrupt fails:

    lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_ENABLE);
    if (WARN_ON(lpar_rc != H_SUCCESS)) {
            schedule_work(&adapter->work);
            goto out;
    }

ibmveth_remove() cancels that work before it unregisters the device,
so NAPI can still run after the cancel:

ibmveth_remove()
    cancel_work_sync(&adapter->work);
    ...
    unregister_netdev(netdev);

    free_netdev(netdev);

If a poll queues adapter->work after cancel_work_sync() returns, can
ibmveth_reset() run on the adapter after free_netdev() has freed it?

The later commit "ibmveth: disable the reset work before unregister in
remove" in this series seems to fix this. It replaces cancel_work_sync()
with disable_work_sync(), which waits for a running reset and blocks
later queueing.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013182926.30417-1-mmc%40linux.ibm.com