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

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.

[ ... ]

[ ... ]

+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?

+
+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().

+	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.

+
+	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.

+		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.

 	if (rc) {
 		netdev_err(netdev,

Thanks for looking at these.

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