Re: [net-next,v6,09/15] ibmveth: Harden RX poll path with helpers

[mingming cao <mmc@linux.ibm.com>]

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
>
> Critical: 0 · High: 2 (1) · Medium: 2 · Low: 0
>
> - [High] The new ibmveth_poll_stopping() check placed between
>    napi_complete_done() and ibmveth_enable_irq() cannot prevent a…
> - [High] The new oversize guard in ibmveth_poll_deliver_frame() bounds
>    the PHYP-reported offset+length against skb_tailroom(skb) instead of…
> - [Medium] A real, user-visible sk_buff leak fix is buried inside a
>    199-line helper-extraction patch with no Fixes: tag and no Cc: stable.
> - [Medium] All 'skip the slot' outcomes (invalid descriptor, bad
>    correlator, oversize offset+length) neither increment frames_processed…
>
> Pre-existing issues:
> - [High] ibmveth_remove() calls cancel_work_sync(&adapter->work) while
>    the netdev is still registered and RX NAPI/IRQ paths are live, then…
>
> --
>
> ibmveth: Harden RX poll path with helpers
>
> This splits the RX slot handling in ibmveth_poll() into a set of small
> helpers (poll_stopping, harvest_slot, recycle_invalid, skip_bad_correlator,
> drop_oversize, deliver_frame, bump_invalid) so the NAPI callback becomes a
> thin budget loop.  Along with the motion it adds an offset+length bounds
> check before skb_put(), frees the copybreak skb on a harvest error, and
> adds stopping checks so close/napi_disable does not re-arm PHYP delivery.
>
> A few questions below.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 46f68f5b52e70..f726d1e3f0b11 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -2382,125 +2380,236 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
>>   	}
>>   }
>>   
> [ ... ]
>
>> +static bool ibmveth_poll_harvest_slot(struct ibmveth_adapter *adapter,
>> +				      int queue_index, bool reuse)
>> +{
>> +	int rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, reuse);
>> +
>> +	return !rc || rc == -EINVAL || rc == -EFAULT;
>> +}
> [Severity: Medium]
> Can this helper ever return false?  ibmveth_rxq_harvest_buffer() documents
> and returns only 0, -EINVAL or -EFAULT:
>
> 	rc = ibmveth_remove_buffer_from_pool(adapter, cor, queue_index, reuse);
> 	if (unlikely(rc)) {
> 		/* Skip a corrupt slot without claiming pool ownership. */
> 		if (rc == -EINVAL || rc == -EFAULT)
> 			ibmveth_rxq_advance(rxq);
> 		return rc;
> 	}
>
> so every "break" in ibmveth_poll_recycle_invalid(),
> ibmveth_poll_skip_bad_correlator() and ibmveth_poll_drop_oversize() looks
> unreachable.
>
> Combined with the budget accounting in the loop below, is there anything
> left that bounds one ibmveth_poll() invocation?  A skipped slot returns 0
> from ibmveth_poll_deliver_frame(), so neither the break nor
> frames_processed++ runs:
>
> 		rc = ibmveth_poll_deliver_frame(napi, adapter, netdev,
> 						queue_index);
> 		if (rc < 0)
> 			break;
> 		if (rc > 0)
> 			frames_processed++;
>
> and each skip recycles the slot with reuse=true, after which
> ibmveth_replenish_task() re-posts it.  The tail of ibmveth_poll() then does:
>
> 	if (ibmveth_rxq_pending_buffer(adapter, queue_index) &&
> 	    napi_schedule(napi)) {
> 		ibmveth_disable_irq(adapter, queue_index);
> 		goto restart_poll;
> 	}
>
> which re-enters the loop in the same invocation with frames_processed
> unchanged.  If PHYP keeps publishing skippable slots (stale ring contents,
> malformed completions), does the frames_processed == budget exit ever
> become reachable, and does this poll ever return?
>
> The reset escalation in ibmveth_poll_skip_bad_correlator() uses
> schedule_work(), which queues on the current CPU via system_percpu_wq, so
> would the worker be able to run while that CPU is stuck in the poll?  The
> ibmveth_poll_drop_oversize() path escalates nothing at all.
>
> The commit message states:
>
>      Skipped and dropped slots do not count against the NAPI budget; only
>      a delivered frame does.
>
> Is that intentional given it removes the only bound on the loop?
Yes. harvest only returns 0/-EINVAL/-EFAULT, so
harvest_slot is always true. That break is not a
bound.

The loop caps delivered frames, not slots, so
there is no strict per-invocation slot bound.
Each skip advances the ring, so any finite
backlog drains, but a stream of invalid
completions would keep this poll running.
restart_poll can re-enter if PHYP posts more.
skip_bad queues a reset; that worker cannot run
if this poll never returns. drop_oversize
harvests only. I am not counting skips toward
budget: that would starve real packets on a
corrupt ring.
>> +
>> +static bool ibmveth_poll_recycle_invalid(struct net_device *netdev,
>> +					 struct ibmveth_adapter *adapter,
>> +					 int queue_index)
>> +{
>> +	netdev_dbg(netdev, "recycling invalid buffer\n");
>> +	ibmveth_poll_bump_invalid(adapter, queue_index);
>> +	return ibmveth_poll_harvest_slot(adapter, queue_index, true);
>> +}
>> +
>> +static bool ibmveth_poll_skip_bad_correlator(struct net_device *netdev,
>> +					     struct ibmveth_adapter *adapter,
>> +					     int queue_index)
>> +{
>> +	if (net_ratelimit())
>> +		netdev_err(netdev,
>> +			   "bad correlator on queue %d, skipping slot\n",
>> +			   queue_index);
>> +	/* Residual stale slot after resize: recover via reset rather
>> +	 * than spinning forever. Always escalate; only the log is
>> +	 * rate-limited.
>> +	 */
>> +	schedule_work(&adapter->work);
> [Severity: High]
> This isn't a bug introduced by this patch, but this adds another NAPI-side
> producer of adapter->work while ibmveth_remove() still cancels that work
> before the device is quiesced:
>
> 	cancel_work_sync(&adapter->work);
>
> 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> 		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>
> 	unregister_netdev(netdev);
>
> 	free_netdev(netdev);
>
> Can a poll running on another CPU call schedule_work(&adapter->work) from
> here (or from the ibmveth_enable_irq() failure path in ibmveth_poll())
> after cancel_work_sync() has returned?  Nothing cancels or flushes it
> again, and adapter is netdev_priv(netdev), so the queued work_struct and
> everything ibmveth_reset() touches would be freed by free_netdev().
>
> For the record, the ordering appears to be repaired later in this series:
> at the series tip ibmveth_remove() moves the cancellation after
> unregister_netdev(), with a comment that cancel_work_sync() before
> unregister left a window where poll could schedule_work() and the worker
> ran after free_netdev().
Yes, at this commit. Patch 10 unregisters
first, then cancel_work_sync, and the reset worker
is gated on NETREG_REGISTERED. The order and the
gate belong together with the reset producer;
that pair is in Patch 10.
>> +	ibmveth_poll_bump_invalid(adapter, queue_index);
>> +	return ibmveth_poll_harvest_slot(adapter, queue_index, true);
>> +}
>> +
> [ ... ]
>
>> +static int ibmveth_poll_deliver_frame(struct napi_struct *napi,
>> +				      struct ibmveth_adapter *adapter,
>> +				      struct net_device *netdev,
>> +				      int queue_index)
>> +{
> [ ... ]
>
>> +	room = skb_tailroom(skb);
>> +	off = offset;
>> +	len = length;
>> +	if (unlikely(off >= room || len > room - off)) {
>> +		if (!ibmveth_poll_drop_oversize(netdev, adapter, queue_index,
>> +						off, len, room))
>> +			return -1;
>> +		return 0;
>> +	}
> [Severity: High]
> Should the bound here be pool->buff_size rather than skb_tailroom(skb)?
>
> ibmveth_replenish_buffer_pool() allocates, maps and advertises exactly
> pool->buff_size to the hypervisor:
>
> 	skb = netdev_alloc_skb(adapter->netdev, pool->buff_size);
> 	...
> 	dma_addr = dma_map_single_attrs(dev, skb->data, pool->buff_size,
> 					DMA_FROM_DEVICE, DMA_ATTR_NO_WARN);
> 	...
> 	descs[filled].fields.flags_len = IBMVETH_BUF_VALID | pool->buff_size;
>
> netdev_alloc_skb() rounds up to an allocator bucket, so skb_tailroom() on
> an untouched pool buffer is larger than buff_size (a 16 KiB pool buffer
> lands in a 32 KiB allocation).  Does a PHYP-reported off+len in the
> (buff_size, tailroom] gap pass this check and then get committed and
> delivered?
>
> 	skb_put(skb, length);
> 	skb->protocol = eth_type_trans(skb, netdev);
> 	...
> 	napi_gro_receive(napi, skb);
> That would hand uninitialised slab bytes outside the DMA mapping to the
> stack, and on the copybreak path skb_copy_to_linear_data() plus
> ibmveth_flush_buffer(skb->data, length + offset) would touch addresses
> past the mapped window.  The commit message says deliver "rejects a PHYP
> offset+length that would skb_put() past the buffer" - is the buffer here
> meant to be the hardware-visible one?  The correlator has already been
> validated by ibmveth_rxq_correlator_valid(), so
> adapter->rx_buff_pool[queue_index][correlator >> 32].buff_size is
> available as the bound.
Yes — a length in (buff_size, tailroom] would pass
on v6. skb_tailroom is the skb_put bound. The DMA
map is pool->buff_size, and alloc can leave slack.
v7 uses min(tailroom, buff_size). The pool index
is the current slot's correlator; get_buffer
already validated it.
>> +
>> +	if (lrg_pkt) {
>> +		__be64 *rxmss = (__be64 *)(skb->data + 8);
>> +
>> +		mss = (u16)be64_to_cpu(*rxmss);
>> +	}
>> +
>> +	new_skb = NULL;
>> +	if (length < rx_copybreak)
>> +		new_skb = netdev_alloc_skb(netdev, length);
>> +
>> +	if (new_skb) {
>> +		skb_copy_to_linear_data(new_skb, skb->data + offset, length);
>> +		if (rx_flush)
>> +			ibmveth_flush_buffer(skb->data, length + offset);
>> +		rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
>> +		if (unlikely(rc)) {
>> +			kfree_skb(new_skb);
>> +			return -1;
>> +		}
> [Severity: Medium]
> This kfree_skb(new_skb) is a real fix for an sk_buff leak that exists at
> the baseline in ibmveth_poll():
>
> 	new_skb = netdev_alloc_skb(netdev, length);
> 	...
> 	skb_copy_to_linear_data(new_skb, skb->data + offset, length);
> 	...
> 	if (unlikely(ibmveth_rxq_harvest_buffer(adapter, true)))
> 		break;   /* new_skb is neither freed nor handed to the stack */
>
> Could this be split into its own patch with a Fixes: tag (and Cc: stable)
> so it can be routed to net and picked up by older trees?  Buried in a
> 199-insertion helper extraction it is not separable for backporting.  The
> same applies to the new offset+length validation, which the commit message
> describes as preventing an skb_put() past the buffer.
The kfree_skb on the copybreak harvest-fail path
stays here. Same for the offset+length check; I am
not peeling a Fixes: patch out of this extract.
The off/len test has no parent SHA.
Neither is the unwind standalone
(Fixes: d43732ce021f).
>> +		skb = new_skb;
>> +	} else {
>> +		rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, false);
>> +		if (unlikely(rc))
>> +			return -1;
>> +		skb_reserve(skb, offset);
>> +	}
> [ ... ]
>
>> +	return 1;
>> +}
>> +
>>   static int ibmveth_poll(struct napi_struct *napi, int budget)
>>   {
> [ ... ]
>
>>   	if (frames_processed == budget)
>>   		goto out;
>>   
>>   	if (!napi_complete_done(napi, frames_processed))
>>   		goto out;
>>   
>> -	/* We think we are done - reenable interrupts,
>> -	 * then check once more to make sure we are done.
>> +	/*
>> +	 * napi_disable() sets DISABLE then waits for this poll. Without a
>> +	 * second stopping check here, enable_irq() can re-arm PHYP after
>> +	 * resize already masked the queue; late IRQs then hit the handler
>> +	 * after num_rx_queues was published lower (lab WARN at interrupt).
>>   	 */
>> +	if (ibmveth_poll_stopping(netdev, napi))
>> +		goto out;
>> +
>>   	rc = ibmveth_enable_irq(adapter, queue_index);
> [Severity: High]
> Does this check actually close the window it describes?  It is a
> check-then-act pair, and both predicates in ibmveth_poll_stopping() go
> false exactly when teardown is finishing:
>
> 	return !netif_running(netdev) || napi_disable_pending(napi);
>
> napi_disable_locked() clears the DISABLE bit before returning:
>
> 	clear_bit(NAPI_STATE_DISABLE, &n->state);
>
> so napi_disable_pending() is false once close is past its wait.  And
> netif_running() stays true for the driver's internal close callers -
> ibmveth_change_mtu(), ibmveth_set_mac_addr(), the features paths and
> veth_pool_store() all do "if (netif_running(dev)) ibmveth_close(dev);".
>
> Sequence:
>
> CPU0 ibmveth_poll()
> 	napi_complete_done()		/* clears SCHED */
> 					/* delayed: hard IRQ, or vCPU
> 					   dispatch preemption on a
> 					   shared-processor LPAR */
>
> CPU1 ibmveth_change_mtu() -> ibmveth_close() -> ibmveth_cleanup_rx_interrupts()
> 	napi_disable()			/* clears DISABLE on return */
> 	ibmveth_disable_irq(adapter, i);
> 	synchronize_irq(adapter->queue_irq[i]);
> 	free_irq(...)
>
> CPU0 resumes:
> 	if (ibmveth_poll_stopping(netdev, napi))	/* false */
> 		goto out;
> 	rc = ibmveth_enable_irq(adapter, queue_index);	/* re-arms PHYP */
>
> Can the queue end up unmasked after the final remask and after free_irq()
> removed the handler?  That is the case the commit message claims to close:
>
>      ibmveth_poll_stopping() ensures close/napi_disable does not re-arm PHYP.
>
> Would moving the unmask before napi_complete_done(), or moving close's
> final remask after synchronize_net(), be a more reliable ordering than
> adding another check here?  This looks unchanged at the series tip.
The check is not a close barrier. After
napi_disable() returns, DISABLE is clear, and an
internal close+open can leave IFF_UP set. Teardown
already masks PHYP before napi_disable. Inverting
that left IFF_UP with PHYP unmasked. I am not
moving unmask before napi_complete_done or
moving the remask after synchronize_net().

Thanks,
Mingming
>>   	if (rc) {
>>   		netdev_err(netdev,
> Thanks for looking at these.
>