[PATCH net-next v4 0/9] ibmveth: fix hangs, use-after-frees and netpoll races

[PATCH net-next v4 0/9] ibmveth: fix hangs, use-after-frees and netpoll races

[Mingming Cao]

Hi,

Nine fixes for serious bugs in the ibmveth driver: two hang the
system, four are use-after-frees, one corrupts memory, one lets a
frame from another partition stall or oops the receiver, and one makes
ethtool -L report success when it failed.

  1. Netpoll races with RX replenish (memory corruption, NULL
     dereference): remove ndo_poll_controller, as was done for
     ibmvnic, skip RX replenish in netpoll's budget-0 poll, and
     enable NAPI only once the RX resources exist.
     Fixes: 6b4223748895, bea3348eef27

  2. Hang after a failed internal reopen: the next close() waits in
     napi_disable() forever with RTNL held, and only a reboot
     recovers. Skip close() when open() did not succeed.
     Fixes: 860f242eb534

  3. Use-after-free in remove(): a reset queued from NAPI could run
     on the freed adapter. Disable the reset work first.
     Fixes: 2c91e2319ed9

  4. Hang or NULL dereference on a bad RX correlator from PHYP: poll
     spun on that slot until RCU stalled, and a correlator naming an
     inactive pool dereferenced NULL. A frame that does not fit its
     buffer was copied past the end. Move past the bad slot and stay
     in poll, and drop a frame that does not fit.
     Fixes: 2c91e2319ed9, 860f242eb534

  5. Use-after-free after a failed probe: the pool kobjects stay in
     sysfs after the adapter is freed. Put them, as remove() does,
     and give them a release() that probe and remove() wait for, so
     CONFIG_DEBUG_KOBJECT_RELEASE cannot free them early either.
     Fixes: 860f242eb534

  6. ethtool -L returns 0 when it cannot allocate the new TX queues.
     Return the allocation error.
     Fixes: 10c2aba89cc0

  7. Use-after-free of TX buffers: close() frees them without
     waiting for a running transmit. It is called directly for MTU,
     offload and buffer pool changes, and through dev_close(), which
     does not wait either with a noqueue qdisc. Use
     netif_tx_disable().
     Fixes: d6832ca48d8a

  8. Use-after-free of the RX queue: napi_disable() returns before
     the poll has finished, and the rest of the poll re-enables the
     interrupt and reads the RX queue that close() then frees. Wait
     with synchronize_net().
     Fixes: bea3348eef27

  9. Stall or oops from a frame sent by another partition: the RX
     checksum helper passes the IPv4 ihl to ip_fast_csum(), which on
     ppc64 reads up to 16 GiB for ihl 0, and both RX offload helpers
     trust the IP and TCP header lengths. Check the headers before
     use, with KUnit cases.
     Fixes: 9c7e8bc584f5, 7b5967389f5a, 66aa0678efc2

Patches 1-8 were found by AI-assisted review of the ibmveth
multi-queue RX series [1] (the patch 4 frame bound by the Sashiko
review of v2), and patch 9 by the Sashiko review of v3 and the review
that followed it. Landing them first also shrinks that
series, which then only extends this handling per queue.

Testing: the new and extended KUnit cases in patches 4 and 9 fail or
oops on the unfixed driver and pass on qemu pseries (ppc64le). On a
POWER10 LPAR: the v3 suite (netconsole under printk and ping floods,
MTU and buffer pool changes under traffic, a forced open() failure
followed by down and up, unbind/bind under traffic, a forced
register_netdev() failure in probe, ethtool -L with a forced TX
buffer allocation failure, and rapid link down/up and MTU cycles under
a ping flood for patch 8) and, for v4, patch 9 large-send regression
(iperf3 over IPv4 and IPv6 at MTU 1500 and 9000, threaded NAPI and
busy polling; no new drops or InCsumErrors). The forced failures used
test-only module parameters that are not part of this series.

The triggers are rare and there are no field reports, so the series
targets net-next. It is based on net-next d8674294aefe ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"), which
includes the two open() error-path fixes (af0524bf4ce1,
84bec0bf0352). Patch 2 explains how it relates to them. It also
applies cleanly to net. All carry Fixes: tags; I am happy to
repost against net, or add Cc: stable, if you prefer.

Changes in v4:

Patches 1-3 and 5-8 are unchanged.

Patch 4: the poll tail queues the NAPI instead of polling again, so
it cannot race a threaded NAPI under busy polling, and harvest uses
the correlator poll already read (Sashiko review of v3). The message
no longer claims the frame bound protects the offload helpers.

New patch 9: the RX offload helpers' header checks that the review of
v3 patch 4 asked for, as their own patch with their own Fixes: tags
and KUnit cases.

v3: https://lore.kernel.org/netdev/20261009183258.18624-1-mmc@linux.ibm.com/

Changes in v3:

From the Sashiko review of v2:
- Patch 4: on a bad RX slot, stay in the poll loop and return
  budget - 1 after napi_schedule() has queued the napi. Breaking
  out was queuing it twice.
- Patch 4: drop a frame that does not fit its buffer, instead of
  copying past the end.
- Patch 4: also drop a frame shorter than an Ethernet header, and
  take the pool for the buffer bound from the correlator that was
  validated, read once.
- Patch 2: commit message: the two open() fixes are now in net-next.

v2: https://lore.kernel.org/netdev/cover.1791178212.git.mmc@linux.ibm.com/

Changes in v2:

From the Sashiko review of v1:
- Patch 5: give the pool kobjects a release() and wait for it before
  free_netdev() in probe and remove(), which closes the
  CONFIG_DEBUG_KOBJECT_RELEASE window.
- New patch 8: wait for the poll to return before close() frees the
  RX queue (raised on patch 1 as a pre-existing bug).

Other small changes:
- Patch 1: also skip RX replenish in netpoll's budget-0 poll.
- Patch 4: count rx_dropped when recycling an invalid buffer fails.
- Commit messages: say how each bug was found and tested, with small
  clarifications in patches 2 and 7.

Rebased on current net-next.

v1: https://lore.kernel.org/netdev/cover.1790991039.git.mmc@linux.ibm.com/

[1] https://lore.kernel.org/netdev/cover.1790319558.git.mmc@linux.ibm.com/

Thanks,
Mingming

Mingming Cao (9):
  ibmveth: fix netpoll races with RX replenish
  ibmveth: do not close twice after a failed reopen
  ibmveth: disable the reset work before unregister in remove
  ibmveth: step past bad RX correlators instead of spinning or oopsing
  ibmveth: release the pool kobjects when probe fails
  ibmveth: return the error when set_channels cannot add TX queues
  ibmveth: wait for in-flight transmits in ibmveth_close()
  ibmveth: wait for the RX poll to return before freeing the RX queue
  ibmveth: check headers in the RX large send and checksum helpers

 drivers/net/ethernet/ibm/ibmveth.c | 647 +++++++++++++++++++++++++----
 drivers/net/ethernet/ibm/ibmveth.h |   5 +
 2 files changed, 561 insertions(+), 91 deletions(-)


base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
-- 
2.50.1 (Apple Git-155)

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

[Mingming Cao]

ibmveth_poll_controller() runs RX replenish outside NAPI and without
a lock, racing NAPI's replenish on another CPU. Both can fill the
same slot, so an skb and its DMA mapping leak and PHYP can write
into an unmapped buffer. netpoll calls it from netconsole and from
netpoll-enabled bonds.

ibmveth_open() also enables NAPI before the RX resources exist.
ibmveth_change_mtu(), veth_pool_store(), ibmveth_set_csum_offload()
and ibmveth_set_tso() call close() and open() directly, so while
open() is still setting up, netpoll and the direct ibmveth_interrupt()
calls can replenish NULL pools and read freed memory.

Remove the callback, as Eric Dumazet did for many drivers after
commit ac3d9dd034e5 ("netpoll: make ndo_poll_controller() optional"),
including ibmvnic in commit 0c3b9d1b37df ("ibmvnic: remove
ndo_poll_controller"). netpoll then polls NAPI itself with budget 0.
napi->poll_owner serializes that with NAPI, but not with a NAPI poll
that was already running when netpoll was set up, so skip RX
replenish at budget 0, which netpoll uses for TX only. TX completes
synchronously, so ibmveth_poll() has nothing else to do for netpoll.

Enable NAPI just before request_irq(), once everything
ibmveth_poll() touches exists.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection of poll_one_napi() and the direct
close()/open() callers; neither race was reproduced. Tested on a POWER10
LPAR with netconsole over ibmveth: a ping flood (678,470 packets, no
loss) during a printk flood, and MTU changes and buffer pool toggles
under traffic, with no warnings. No kernel selftests cover ibmveth.

Fixes: 6b4223748895 ("[PATCH] ibmveth: Add netpoll function")
Fixes: bea3348eef27 ("[NET]: Make NAPI polling independent of struct net_device objects.")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v2:
- skip RX replenish when ibmveth_poll() runs with budget 0:
  napi->poll_owner does not serialize netpoll with a NAPI poll
  that was already running when netpoll was set up

 drivers/net/ethernet/ibm/ibmveth.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index abebdb1fc262..4a5869183be6 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -628,8 +628,6 @@ static int ibmveth_open(struct net_device *netdev)
 
 	netdev_dbg(netdev, "open starting\n");
 
-	napi_enable(&adapter->napi);
-
 	for(i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
 		rxq_entries += adapter->rx_buff_pool[i].size;
 
@@ -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;
@@ -765,7 +771,6 @@ static int ibmveth_open(struct net_device *netdev)
 out_free_buffer_list:
 	free_page((unsigned long)adapter->buffer_list_addr);
 out:
-	napi_disable(&adapter->napi);
 	return rc;
 }
 
@@ -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;
@@ -1682,14 +1691,6 @@ static int ibmveth_change_mtu(struct net_device *dev, int new_mtu)
 	return -EINVAL;
 }
 
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void ibmveth_poll_controller(struct net_device *dev)
-{
-	ibmveth_replenish_task(netdev_priv(dev));
-	ibmveth_interrupt(dev->irq, dev);
-}
-#endif
-
 /**
  * ibmveth_get_desired_dma - Calculate IO memory desired by the driver
  *
@@ -1791,9 +1792,6 @@ static const struct net_device_ops ibmveth_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address    = ibmveth_set_mac_addr,
 	.ndo_features_check	= ibmveth_features_check,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller	= ibmveth_poll_controller,
-#endif
 };
 
 static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 2/9] ibmveth: do not close twice after a failed reopen

[Mingming Cao]

ibmveth_change_mtu(), veth_pool_store(), ibmveth_set_csum_offload()
and ibmveth_set_tso() call close() and open() directly. If open() fails,
NAPI is left disabled while IFF_UP stays set, so the next close()
(ifdown, unregister or another reconfiguration) calls napi_disable()
again and waits forever with RTNL held. Networking and shutdown
hang; only a reboot recovers. ethtool -L in that state also wakes
queues that have no TX buffer and dereferences NULL in
ibmveth_start_xmit().

Any open() failure on those paths triggers it, for example an
allocation failure on an MTU change to jumbo frames.

Track a successful open in adapter->opened. close() returns early
when it is clear, and set_channels() checks it instead of IFF_UP.

The open() error-path leaks were fixed separately in net by
commit af0524bf4ce1 ("ibmveth: h_free logical LAN on open-fail after
register") and commit 84bec0bf0352 ("ibmveth: fix TX LTB and filter
unwind on open-fail"), which are now in net-next too, so a failed
open() no longer leaves TX buffers or the logical LAN registration
behind for the early return to skip.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection. Tested on a POWER10 LPAR with
ibmveth_open() forced to fail by a test-only module parameter (not part
of this patch): 'ip link set dev eth1 mtu 9000' fails, then 'ip link set
dev eth1 down' returns at once and 'ip link set dev eth1 up' recovers
the interface. No kernel selftests cover ibmveth.

Fixes: 860f242eb534 ("[PATCH] ibmveth change buffer pools dynamically")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v3:
- commit message: the two open() fixes are now in net-next

Changes in v2:
- commit message: say what a failed open() leaves behind without the
  two net fixes

 drivers/net/ethernet/ibm/ibmveth.c | 24 +++++++++++++++++-------
 drivers/net/ethernet/ibm/ibmveth.h |  2 ++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 4a5869183be6..4665447997ea 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -739,6 +739,7 @@ static int ibmveth_open(struct net_device *netdev)
 
 	netif_tx_start_all_queues(netdev);
 
+	adapter->opened = true;
 	netdev_dbg(netdev, "open complete\n");
 
 	return 0;
@@ -781,6 +782,14 @@ static int ibmveth_close(struct net_device *netdev)
 	long lpar_rc;
 	int i;
 
+	/* change_mtu, pool sysfs, set_csum and set_tso call close() and
+	 * open() directly. If that open() fails, IFF_UP stays set and
+	 * NAPI is disabled; a second close() would hang in napi_disable().
+	 */
+	if (!adapter->opened)
+		return 0;
+	adapter->opened = false;
+
 	netdev_dbg(netdev, "close starting\n");
 
 	napi_disable(&adapter->napi);
@@ -832,10 +841,10 @@ static int ibmveth_close(struct net_device *netdev)
  *
  * @w: pointer to work_struct embedded in adapter structure
  *
- * Context: This routine acquires rtnl_mutex and disables its NAPI through
- *          ibmveth_close. It can't be called directly in a context that has
- *          already acquired rtnl_mutex or disabled its NAPI, or directly from
- *          a poll routine.
+ * Context: This routine acquires rtnl_mutex and, if the device is open,
+ *          disables its NAPI through ibmveth_close. It can't be called
+ *          directly in a context that has already acquired rtnl_mutex or
+ *          disabled its NAPI, or directly from a poll routine.
  *
  * Return: void
  */
@@ -1129,10 +1138,11 @@ static int ibmveth_set_channels(struct net_device *netdev,
 		     goal = channels->tx_count;
 	int rc, i;
 
-	/* If ndo_open has not been called yet then don't allocate, just set
-	 * desired netdev_queue's and return
+	/* If the device is not open (including a failed close/open with
+	 * IFF_UP still set) then don't allocate, just set desired
+	 * netdev_queue's and return
 	 */
-	if (!(netdev->flags & IFF_UP))
+	if (!adapter->opened)
 		return netif_set_real_num_tx_queues(netdev, goal);
 
 	/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
index d87713668ed3..3f2240823f6a 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -172,6 +172,8 @@ struct ibmveth_adapter {
 	int rx_csum;
 	int large_send;
 	bool is_active_trunk;
+	/* Set by a successful ibmveth_open(), cleared by ibmveth_close(). */
+	bool opened;
 	unsigned int rx_buffers_per_hcall;
 
 	u64 fw_ipv6_csum_support;
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 3/9] ibmveth: disable the reset work before unregister in remove

[Mingming Cao]

ibmveth_remove() cancels the reset work before unregister_netdev(),
but NAPI keeps running until unregister closes the device and can
queue the reset again, on an interrupt enable failure, a bad
free_map entry or a bad RX slot. The reset can then run on the
adapter after free_netdev(), or reopen the device during unregister.

Use disable_work_sync(), which waits for a running reset and keeps
the work from being queued again. A bad RX slot also makes poll
spin, which can stall unregister; a later patch in this series
fixes that.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection; it was not reproduced. Tested on a POWER10
LPAR with unbind and bind cycles under traffic. No kernel selftests
cover ibmveth.

Fixes: 2c91e2319ed9 ("net: ibmveth: Reset the adapter when unexpected states are detected")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 4665447997ea..cee0e9783b2a 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1956,7 +1956,7 @@ static void ibmveth_remove(struct vio_dev *dev)
 	struct ibmveth_adapter *adapter = netdev_priv(netdev);
 	int i;
 
-	cancel_work_sync(&adapter->work);
+	disable_work_sync(&adapter->work);
 
 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
 		kobject_put(&adapter->rx_buff_pool[i].kobj);
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 4/9] ibmveth: step past bad RX correlators instead of spinning or oopsing

[Mingming Cao]

ibmveth_poll() mishandles a bad RX correlator from PHYP in two ways.

If harvest fails or ibmveth_rxq_get_buffer() returns NULL, poll
breaks out without advancing the ring and restarts on the same slot
forever. For an out-of-range correlator it also fires a WARN_ON() and
schedules a reset each pass, but the reset is queued on that CPU and
never runs, and the CPU stalls RCU, so RTNL holders hang too. This
dates from the first commit in Fixes:, which turned the BUG_ON()s
here into WARN_ON() plus a reset.

The range check also accepts a correlator naming an inactive buffer
pool (pools 2 and 3 by default), whose skbuff array is NULL, so the
lookup dereferences NULL in softirq. Pools became inactive with the
second commit in Fixes:.

Validate the correlator in one helper that also rejects a pool with
no skbuff array. On a bad slot, advance the ring, count the drop in
rx_dropped and still schedule the reset, which rebuilds the pools.
The slot has moved, so stay in the budget loop and count it in
frames_processed. Breaking out would complete NAPI, re-arm, and
napi_schedule() with budget left. When the final check finds more
work, queue the NAPI and return instead of going back to
restart_poll: after napi_complete_done() the NAPI is no longer this
call's, and with threaded NAPI and busy polling the kthread can
already be polling it on another CPU. frames_processed is below
budget there, so busy_poll_stop() does not queue it a second time.
Mask the interrupt before queueing, as ibmveth_interrupt() does;
masking after napi_schedule() could land after that kthread has
finished and unmasked, leaving RX masked with nothing scheduled.

Drop a frame whose offset + length does not fit the pool buffer:
copybreak would read past the RX buffer and skb_put() would BUG().
Also drop one shorter than an Ethernet header, which eth_type_trans()
would read past. Read the correlator once per slot and use that value
for the lookup, the bound and the harvest.

The base driver never checked frame bounds, and they have not been
seen in the field, so they have no Fixes: tag of their own. The tags
below are for the correlator hang.

The correlator comes from PHYP, and with the ring advancing a burst
of bad slots would WARN once per slot (and panic with
panic_on_warn), so use a ratelimited netdev_err() instead. Also free
the rx_copybreak skb if harvest fails there.

Add a KUnit case for harvest advancing on errors and extend the
existing cases to an inactive pool. Run against the unfixed driver,
with the calls changed back to its signatures, the first fails and
the others oops.

Found by AI-assisted review of the ibmveth multi-queue RX series (the
frame bound by the Sashiko review of v2 of this series) and confirmed
by code inspection and the KUnit cases above. Hitting either
correlator bug needs PHYP to return a bad correlator, so neither was
reproduced on hardware. Tested with KUnit on qemu pseries (ppc64le),
and on a POWER10 LPAR with a ping flood and MTU changes under traffic.
No kernel selftests cover ibmveth.

Fixes: 2c91e2319ed9 ("net: ibmveth: Reset the adapter when unexpected states are detected")
Fixes: 860f242eb534 ("[PATCH] ibmveth change buffer pools dynamically")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v4:
- after the final check finds more work, queue the NAPI and return,
  instead of going back to restart_poll; drop the budget - 1
  return (Sashiko review of v3: threaded NAPI with busy polling)
- mask the interrupt before queueing the NAPI, as the interrupt
  handler does, so a threaded NAPI cannot unmask first (found while
  reviewing the above)
- pass the correlator that poll read to harvest, instead of reading
  the ring entry again (Sashiko review of v3)
- the message no longer claims the frame bound protects the checksum
  helpers (Sashiko review of v3), and gives the Ethernet header
  minimum its own reason; the helper checks are patch 9

Changes in v3:
- after a bad slot, keep polling and count the slot against the NAPI
  budget instead of breaking out with budget left; once poll has
  rescheduled itself, return budget - 1 so busy_poll_stop() cannot
  queue the NAPI a second time (Sashiko review of v2)
- drop a frame whose offset + length does not fit its pool buffer
  (Sashiko review of v2, pre-existing) or that is shorter than an
  Ethernet header
- read the correlator once and pass it to ibmveth_rxq_get_buffer(),
  so the buffer bound uses the validated pool

Changes in v2:
- count rx_dropped when recycling an invalid buffer fails

 drivers/net/ethernet/ibm/ibmveth.c | 265 +++++++++++++++++++++++------
 1 file changed, 216 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index cee0e9783b2a..919b5831d875 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -135,6 +135,13 @@ static inline int ibmveth_rxq_frame_length(struct ibmveth_adapter *adapter)
 	return be32_to_cpu(adapter->rx_queue.queue_addr[adapter->rx_queue.index].length);
 }
 
+static u64 ibmveth_rxq_correlator(struct ibmveth_adapter *adapter)
+{
+	unsigned int idx = adapter->rx_queue.index;
+
+	return READ_ONCE(adapter->rx_queue.queue_addr[idx].correlator);
+}
+
 static inline int ibmveth_rxq_csum_good(struct ibmveth_adapter *adapter)
 {
 	return ibmveth_rxq_flags(adapter) & IBMVETH_RXQ_CSUM_GOOD;
@@ -443,6 +450,37 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
 	}
 }
 
+/* The correlator comes back from PHYP; a bad one schedules a reset. */
+static bool ibmveth_rxq_correlator_valid(struct ibmveth_adapter *adapter,
+					 u64 correlator)
+{
+	unsigned int index = correlator & 0xffffffffUL;
+	unsigned int pool = correlator >> 32;
+
+	/* An inactive pool keeps its size but has no skbuff array. */
+	if (pool < IBMVETH_NUM_BUFF_POOLS &&
+	    index < adapter->rx_buff_pool[pool].size &&
+	    adapter->rx_buff_pool[pool].skbuff)
+		return true;
+
+	if (net_ratelimit())
+		netdev_err(adapter->netdev,
+			   "invalid RX correlator %llx, resetting\n",
+			   correlator);
+	schedule_work(&adapter->work);
+	return false;
+}
+
+static void ibmveth_rxq_no_skb(struct ibmveth_adapter *adapter,
+			       u64 correlator)
+{
+	if (net_ratelimit())
+		netdev_err(adapter->netdev,
+			   "no buffer for RX correlator %llx, resetting\n",
+			   correlator);
+	schedule_work(&adapter->work);
+}
+
 /**
  * ibmveth_remove_buffer_from_pool - remove a buffer from a pool
  * @adapter: adapter instance
@@ -451,7 +489,8 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
  *
  * Return:
  * * %0       - success
- * * %-EINVAL - correlator maps to pool or index out of range
+ * * %-EINVAL - correlator maps to pool or index out of range, or to an
+ *              inactive pool
  * * %-EFAULT - pool and index map to null skb
  */
 static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
@@ -462,15 +501,12 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
 	unsigned int free_index;
 	struct sk_buff *skb;
 
-	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
-	    WARN_ON(index >= adapter->rx_buff_pool[pool].size)) {
-		schedule_work(&adapter->work);
+	if (!ibmveth_rxq_correlator_valid(adapter, correlator))
 		return -EINVAL;
-	}
 
 	skb = adapter->rx_buff_pool[pool].skbuff[index];
-	if (WARN_ON(!skb)) {
-		schedule_work(&adapter->work);
+	if (!skb) {
+		ibmveth_rxq_no_skb(adapter, correlator);
 		return -EFAULT;
 	}
 
@@ -504,51 +540,56 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
 	return 0;
 }
 
-/* get the current buffer on the rx queue */
-static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter)
+/* get the buffer for @correlator, read once from the current rx queue entry */
+static struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *adapter,
+					      u64 correlator)
 {
-	u64 correlator = adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator;
 	unsigned int pool = correlator >> 32;
 	unsigned int index = correlator & 0xffffffffUL;
+	struct sk_buff *skb;
 
-	if (WARN_ON(pool >= IBMVETH_NUM_BUFF_POOLS) ||
-	    WARN_ON(index >= adapter->rx_buff_pool[pool].size)) {
-		schedule_work(&adapter->work);
+	if (!ibmveth_rxq_correlator_valid(adapter, correlator))
 		return NULL;
-	}
 
-	return adapter->rx_buff_pool[pool].skbuff[index];
+	skb = adapter->rx_buff_pool[pool].skbuff[index];
+	if (!skb)
+		ibmveth_rxq_no_skb(adapter, correlator);
+	return skb;
+}
+
+static void ibmveth_rxq_advance(struct ibmveth_adapter *adapter)
+{
+	if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) {
+		adapter->rx_queue.index = 0;
+		adapter->rx_queue.toggle = !adapter->rx_queue.toggle;
+	}
 }
 
 /**
  * ibmveth_rxq_harvest_buffer - Harvest buffer from pool
  *
- * @adapter: pointer to adapter
- * @reuse:   whether to reuse buffer
+ * @adapter:    pointer to adapter
+ * @correlator: correlator of the current rx queue entry, as read by poll
+ * @reuse:      whether to reuse buffer
  *
  * Context: called from ibmveth_poll
  *
+ * The ring advances even on error, so poll does not return to a bad
+ * slot before the scheduled reset can run.
+ *
  * Return:
  * * %0    - success
  * * other - non-zero return from ibmveth_remove_buffer_from_pool
  */
 static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
-				      bool reuse)
+				      u64 correlator, bool reuse)
 {
-	u64 cor;
 	int rc;
 
-	cor = adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator;
-	rc = ibmveth_remove_buffer_from_pool(adapter, cor, reuse);
-	if (unlikely(rc))
-		return rc;
+	rc = ibmveth_remove_buffer_from_pool(adapter, correlator, reuse);
+	ibmveth_rxq_advance(adapter);
 
-	if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) {
-		adapter->rx_queue.index = 0;
-		adapter->rx_queue.toggle = !adapter->rx_queue.toggle;
-	}
-
-	return 0;
+	return rc;
 }
 
 static void ibmveth_free_tx_ltb(struct ibmveth_adapter *adapter, int idx)
@@ -1469,31 +1510,60 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 	struct net_device *netdev = adapter->netdev;
 	int frames_processed = 0;
 	unsigned long lpar_rc;
+	u64 correlator;
 	u16 mss = 0;
+	int rc;
 
-restart_poll:
 	while (frames_processed < budget) {
 		if (!ibmveth_rxq_pending_buffer(adapter))
 			break;
 
 		smp_rmb();
+		correlator = ibmveth_rxq_correlator(adapter);
 		if (!ibmveth_rxq_buffer_valid(adapter)) {
 			wmb(); /* suggested by larson1 */
 			adapter->rx_invalid_buffer++;
 			netdev_dbg(netdev, "recycling invalid buffer\n");
-			if (unlikely(ibmveth_rxq_harvest_buffer(adapter, true)))
-				break;
+			rc = ibmveth_rxq_harvest_buffer(adapter, correlator, true);
+			if (unlikely(rc)) {
+				netdev->stats.rx_dropped++;
+				frames_processed++;
+				continue;
+			}
 		} else {
 			struct sk_buff *skb, *new_skb;
 			int length = ibmveth_rxq_frame_length(adapter);
 			int offset = ibmveth_rxq_frame_offset(adapter);
 			int csum_good = ibmveth_rxq_csum_good(adapter);
 			int lrg_pkt = ibmveth_rxq_large_packet(adapter);
+			unsigned int pool, room, off, len;
 			__sum16 iph_check = 0;
 
-			skb = ibmveth_rxq_get_buffer(adapter);
-			if (unlikely(!skb))
-				break;
+			skb = ibmveth_rxq_get_buffer(adapter, correlator);
+			if (unlikely(!skb)) {
+				ibmveth_rxq_advance(adapter);
+				netdev->stats.rx_dropped++;
+				frames_processed++;
+				continue;
+			}
+
+			pool = correlator >> 32;
+			room = min_t(unsigned int, skb_tailroom(skb),
+				     adapter->rx_buff_pool[pool].buff_size);
+			off = offset;
+			len = length;
+			if (unlikely(len < ETH_HLEN || off >= room ||
+				     len > room - off)) {
+				if (net_ratelimit())
+					netdev_err(netdev,
+						   "bad RX frame offset %u length %u (buffer %u), dropping\n",
+						   off, len, room);
+				ibmveth_rxq_harvest_buffer(adapter, correlator,
+							   true);
+				netdev->stats.rx_dropped++;
+				frames_processed++;
+				continue;
+			}
 
 			/* if the large packet bit is set in the rx queue
 			 * descriptor, the mss will be written by PHYP eight
@@ -1517,12 +1587,25 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 				if (rx_flush)
 					ibmveth_flush_buffer(skb->data,
 						length + offset);
-				if (unlikely(ibmveth_rxq_harvest_buffer(adapter, true)))
-					break;
+				rc = ibmveth_rxq_harvest_buffer(adapter,
+								correlator,
+								true);
+				if (unlikely(rc)) {
+					dev_kfree_skb_any(new_skb);
+					netdev->stats.rx_dropped++;
+					frames_processed++;
+					continue;
+				}
 				skb = new_skb;
 			} else {
-				if (unlikely(ibmveth_rxq_harvest_buffer(adapter, false)))
-					break;
+				rc = ibmveth_rxq_harvest_buffer(adapter,
+								correlator,
+								false);
+				if (unlikely(rc)) {
+					netdev->stats.rx_dropped++;
+					frames_processed++;
+					continue;
+				}
 				skb_reserve(skb, offset);
 			}
 
@@ -1578,10 +1661,13 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 		goto out;
 	}
 
-	if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
+	/* Mask before queueing, as ibmveth_interrupt() does: a threaded
+	 * NAPI can start polling, and unmask, as soon as it is queued.
+	 */
+	if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule_prep(napi)) {
 		lpar_rc = h_vio_signal(adapter->vdev->unit_address,
 				       VIO_IRQ_DISABLE);
-		goto restart_poll;
+		__napi_schedule(napi);
 	}
 
 out:
@@ -2206,8 +2292,7 @@ static void ibmveth_reset_kunit(struct work_struct *w)
  * @test: pointer to kunit structure
  *
  * Tests the error returns from ibmveth_remove_buffer_from_pool.
- * ibmveth_remove_buffer_from_pool also calls WARN_ON, so dmesg should be
- * checked to see that these warnings happened.
+ * Each error also logs a ratelimited netdev_err.
  *
  * Return: void
  */
@@ -2216,6 +2301,7 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)
 	struct ibmveth_adapter *adapter = kunit_kzalloc(test, sizeof(*adapter), GFP_KERNEL);
 	struct ibmveth_buff_pool *pool;
 	u64 correlator;
+	int ret;
 
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adapter);
 
@@ -2239,6 +2325,13 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, -EINVAL, ibmveth_remove_buffer_from_pool(adapter, correlator, false));
 	KUNIT_EXPECT_EQ(test, -EINVAL, ibmveth_remove_buffer_from_pool(adapter, correlator, true));
 
+	/* Pool 2 is in range but has no skbuff array, like an inactive pool. */
+	correlator = ((u64)2 << 32) | 0;
+	ret = ibmveth_remove_buffer_from_pool(adapter, correlator, false);
+	KUNIT_EXPECT_EQ(test, -EINVAL, ret);
+	ret = ibmveth_remove_buffer_from_pool(adapter, correlator, true);
+	KUNIT_EXPECT_EQ(test, -EINVAL, ret);
+
 	correlator = (u64)0 | 0;
 	pool->skbuff[0] = NULL;
 	KUNIT_EXPECT_EQ(test, -EFAULT, ibmveth_remove_buffer_from_pool(adapter, correlator, false));
@@ -2251,9 +2344,8 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)
  * ibmveth_rxq_get_buffer_test - unit test for ibmveth_rxq_get_buffer
  * @test: pointer to kunit structure
  *
- * Tests ibmveth_rxq_get_buffer. ibmveth_rxq_get_buffer also calls WARN_ON for
- * the NULL returns, so dmesg should be checked to see that these warnings
- * happened.
+ * Tests ibmveth_rxq_get_buffer. Each NULL return also logs a ratelimited
+ * netdev_err.
  *
  * Return: void
  */
@@ -2262,6 +2354,7 @@ static void ibmveth_rxq_get_buffer_test(struct kunit *test)
 	struct ibmveth_adapter *adapter = kunit_kzalloc(test, sizeof(*adapter), GFP_KERNEL);
 	struct sk_buff *skb = kunit_kzalloc(test, sizeof(*skb), GFP_KERNEL);
 	struct ibmveth_buff_pool *pool;
+	u64 cor;
 
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adapter);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, skb);
@@ -2285,14 +2378,87 @@ static void ibmveth_rxq_get_buffer_test(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pool->skbuff);
 
 	adapter->rx_queue.queue_addr[0].correlator = (u64)IBMVETH_NUM_BUFF_POOLS << 32 | 0;
-	KUNIT_EXPECT_PTR_EQ(test, NULL, ibmveth_rxq_get_buffer(adapter));
+	cor = ibmveth_rxq_correlator(adapter);
+	KUNIT_EXPECT_PTR_EQ(test, NULL,
+			    ibmveth_rxq_get_buffer(adapter, cor));
 
 	adapter->rx_queue.queue_addr[0].correlator = (u64)0 << 32 | adapter->rx_buff_pool[0].size;
-	KUNIT_EXPECT_PTR_EQ(test, NULL, ibmveth_rxq_get_buffer(adapter));
+	cor = ibmveth_rxq_correlator(adapter);
+	KUNIT_EXPECT_PTR_EQ(test, NULL,
+			    ibmveth_rxq_get_buffer(adapter, cor));
+
+	/* Pool 2 is in range but has no skbuff array, like an inactive pool. */
+	adapter->rx_queue.queue_addr[0].correlator = (u64)2 << 32 | 0;
+	cor = ibmveth_rxq_correlator(adapter);
+	KUNIT_EXPECT_PTR_EQ(test, NULL,
+			    ibmveth_rxq_get_buffer(adapter, cor));
 
 	pool->skbuff[0] = skb;
 	adapter->rx_queue.queue_addr[0].correlator = (u64)0 << 32 | 0;
-	KUNIT_EXPECT_PTR_EQ(test, skb, ibmveth_rxq_get_buffer(adapter));
+	cor = ibmveth_rxq_correlator(adapter);
+	KUNIT_EXPECT_PTR_EQ(test, skb,
+			    ibmveth_rxq_get_buffer(adapter, cor));
+
+	flush_work(&adapter->work);
+}
+
+/**
+ * ibmveth_rxq_harvest_buffer_test - unit test for ibmveth_rxq_harvest_buffer
+ * @test: pointer to kunit structure
+ *
+ * A bad correlator must still advance the RX ring, wrapping and flipping
+ * the toggle at the end. This covers the harvest path; the advance after
+ * ibmveth_rxq_get_buffer() fails in ibmveth_poll() is not tested here.
+ *
+ * Return: void
+ */
+static void ibmveth_rxq_harvest_buffer_test(struct kunit *test)
+{
+	struct ibmveth_adapter *adapter;
+	struct ibmveth_buff_pool *pool;
+	u64 cor;
+	int ret;
+
+	adapter = kunit_kzalloc(test, sizeof(*adapter), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adapter);
+
+	INIT_WORK(&adapter->work, ibmveth_reset_kunit);
+
+	adapter->rx_queue.num_slots = 2;
+	adapter->rx_queue.index = 0;
+	adapter->rx_queue.toggle = 1;
+	adapter->rx_queue.queue_addr =
+		kunit_kcalloc(test, 2, sizeof(struct ibmveth_rx_q_entry),
+			      GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adapter->rx_queue.queue_addr);
+
+	/* Set sane values for buffer pools */
+	for (int i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
+		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[i], i,
+					 pool_count[i], pool_size[i],
+					 pool_active[i]);
+
+	pool = &adapter->rx_buff_pool[0];
+	pool->skbuff = kunit_kcalloc(test, pool->size, sizeof(void *),
+				     GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pool->skbuff);
+
+	/* Slot 0: pool out of range. Slot 1: valid, but no skb. */
+	adapter->rx_queue.queue_addr[0].correlator =
+		(u64)IBMVETH_NUM_BUFF_POOLS << 32 | 0;
+	adapter->rx_queue.queue_addr[1].correlator = (u64)0 << 32 | 0;
+
+	cor = ibmveth_rxq_correlator(adapter);
+	ret = ibmveth_rxq_harvest_buffer(adapter, cor, true);
+	KUNIT_EXPECT_EQ(test, -EINVAL, ret);
+	KUNIT_EXPECT_EQ(test, 1ULL, adapter->rx_queue.index);
+	KUNIT_EXPECT_EQ(test, 1ULL, adapter->rx_queue.toggle);
+
+	cor = ibmveth_rxq_correlator(adapter);
+	ret = ibmveth_rxq_harvest_buffer(adapter, cor, true);
+	KUNIT_EXPECT_EQ(test, -EFAULT, ret);
+	KUNIT_EXPECT_EQ(test, 0ULL, adapter->rx_queue.index);
+	KUNIT_EXPECT_EQ(test, 0ULL, adapter->rx_queue.toggle);
 
 	flush_work(&adapter->work);
 }
@@ -2300,6 +2466,7 @@ static void ibmveth_rxq_get_buffer_test(struct kunit *test)
 static struct kunit_case ibmveth_test_cases[] = {
 	KUNIT_CASE(ibmveth_remove_buffer_from_pool_test),
 	KUNIT_CASE(ibmveth_rxq_get_buffer_test),
+	KUNIT_CASE(ibmveth_rxq_harvest_buffer_test),
 	{}
 };
 
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 5/9] ibmveth: release the pool kobjects when probe fails

[Mingming Cao]

If register_netdev() fails in ibmveth_probe(), for example with -EINTR
when the binding task is killed, probe frees the netdev but leaves the
pool%d kobjects embedded in it registered in sysfs. Reading
/sys/devices/vio/<unit>/pool0/num is then a use-after-free, and the
next probe cannot add pool0.

Put the kobjects before freeing the netdev, as ibmveth_remove() does,
on this path and on the netif_set_real_num_tx_queues() one.

The kobjects also have no release(). With CONFIG_DEBUG_KOBJECT_RELEASE,
kobject_put() defers their cleanup, including removing the sysfs files,
to a work item, so free_netdev() can free them first, here and in
remove(). Add a release() that signals a per-pool completion, and wait
for it in both places before free_netdev(). Without that config,
release() runs from kobject_put() and the wait returns at once.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection; the release() part was raised by the
Sashiko AI review of the first version. Tested on a POWER10 LPAR with
register_netdev() forced to fail with -EINTR by a test-only module
parameter (not part of this patch): no pool%d directories remain and the
device binds again. No kernel selftests cover ibmveth.

Fixes: 860f242eb534 ("[PATCH] ibmveth change buffer pools dynamically")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v2:
- give the pool kobjects a release() that signals a per-pool
  completion, and wait for it before free_netdev() in probe and
  remove(); with CONFIG_DEBUG_KOBJECT_RELEASE the deferred cleanup
  could run after the free (Sashiko review of v1)
- put the kobjects through one helper, ibmveth_put_pool_kobjs(),
  and an err_put_pools label for both probe failure paths

 drivers/net/ethernet/ibm/ibmveth.c | 52 +++++++++++++++++++++++++-----
 drivers/net/ethernet/ibm/ibmveth.h |  3 ++
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 919b5831d875..42aa9f7bd49d 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1890,6 +1890,40 @@ static const struct net_device_ops ibmveth_netdev_ops = {
 	.ndo_features_check	= ibmveth_features_check,
 };
 
+/**
+ * ibmveth_pool_kobj_release - Mark a pool kobject finished
+ * @kobj: kobject embedded in the pool
+ *
+ * The pool kobjects live in netdev_priv(), so the last put must wait
+ * for this before free_netdev().
+ */
+static void ibmveth_pool_kobj_release(struct kobject *kobj)
+{
+	struct ibmveth_buff_pool *pool = container_of(kobj,
+						      struct ibmveth_buff_pool,
+						      kobj);
+
+	complete(&pool->released);
+}
+
+/**
+ * ibmveth_put_pool_kobjs - Drop the pool kobjects and wait for release
+ * @adapter: ibmveth adapter
+ *
+ * With CONFIG_DEBUG_KOBJECT_RELEASE the cleanup, including removing the
+ * sysfs files, runs later from a work item in the kobject; wait for it
+ * so free_netdev() cannot free the pools first.
+ */
+static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter)
+{
+	int i;
+
+	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
+		kobject_put(&adapter->rx_buff_pool[i].kobj);
+	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
+		wait_for_completion(&adapter->rx_buff_pool[i].released);
+}
+
 static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 {
 	int rc, i, mac_len;
@@ -2000,6 +2034,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[i], i,
 					 pool_count[i], pool_size[i],
 					 pool_active[i]);
+		init_completion(&adapter->rx_buff_pool[i].released);
 		error = kobject_init_and_add(kobj, &ktype_veth_pool,
 					     &dev->dev.kobj, "pool%d", i);
 		if (!error)
@@ -2011,8 +2046,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	if (rc) {
 		netdev_dbg(netdev, "failed to set number of tx queues rc=%d\n",
 			   rc);
-		free_netdev(netdev);
-		return rc;
+		goto err_put_pools;
 	}
 	adapter->tx_ltb_size = PAGE_ALIGN(IBMVETH_MAX_TX_BUF_SIZE);
 	for (i = 0; i < IBMVETH_MAX_QUEUES; i++)
@@ -2027,25 +2061,27 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 
 	if (rc) {
 		netdev_dbg(netdev, "failed to register netdev rc=%d\n", rc);
-		free_netdev(netdev);
-		return rc;
+		goto err_put_pools;
 	}
 
 	netdev_dbg(netdev, "registered\n");
 
 	return 0;
+
+err_put_pools:
+	ibmveth_put_pool_kobjs(adapter);
+	free_netdev(netdev);
+	return rc;
 }
 
 static void ibmveth_remove(struct vio_dev *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(&dev->dev);
 	struct ibmveth_adapter *adapter = netdev_priv(netdev);
-	int i;
 
 	disable_work_sync(&adapter->work);
 
-	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
-		kobject_put(&adapter->rx_buff_pool[i].kobj);
+	ibmveth_put_pool_kobjs(adapter);
 
 	unregister_netdev(netdev);
 
@@ -2221,7 +2257,7 @@ static const struct sysfs_ops veth_pool_ops = {
 };
 
 static struct kobj_type ktype_veth_pool = {
-	.release        = NULL,
+	.release        = ibmveth_pool_kobj_release,
 	.sysfs_ops      = &veth_pool_ops,
 	.default_groups = veth_pool_groups,
 };
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
index 3f2240823f6a..be0939caa328 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -14,6 +14,8 @@
 #ifndef _IBMVETH_H
 #define _IBMVETH_H
 
+#include <linux/completion.h>
+
 /* constants for H_MULTICAST_CTRL */
 #define IbmVethMcastReceptionModifyBit     0x80000UL
 #define IbmVethMcastReceptionEnableBit     0x20000UL
@@ -143,6 +145,7 @@ struct ibmveth_buff_pool {
     struct sk_buff **skbuff;
     int active;
     struct kobject kobj;
+	struct completion released;
 };
 
 struct ibmveth_rx_q {
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 6/9] ibmveth: return the error when set_channels cannot add TX queues

[Mingming Cao]

When ibmveth_set_channels() cannot allocate a TX buffer for a new
queue, it falls back to the old queue count, and the successful
netif_set_real_num_tx_queues() call then overwrites rc. ethtool -L
reports success while the queue count is unchanged.

Return the allocation error when the fallback succeeds.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection. Tested on a POWER10 LPAR with the TX
buffer allocation forced to fail by a test-only module parameter (not
part of this patch): ethtool -L tx 8 now fails with -ENOMEM and the
device keeps its four queues and passes traffic. No kernel selftests
cover ibmveth.

Fixes: 10c2aba89cc0 ("ibmveth: Ethtool set queue support")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 42aa9f7bd49d..48bc4406396b 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1177,7 +1177,7 @@ static int ibmveth_set_channels(struct net_device *netdev,
 	struct ibmveth_adapter *adapter = netdev_priv(netdev);
 	unsigned int old = netdev->real_num_tx_queues,
 		     goal = channels->tx_count;
-	int rc, i;
+	int rc, i, alloc_rc = 0;
 
 	/* If the device is not open (including a failed close/open with
 	 * IFF_UP still set) then don't allocate, just set desired
@@ -1203,6 +1203,7 @@ static int ibmveth_set_channels(struct net_device *netdev,
 		/* if something goes wrong, free everything we just allocated */
 		netdev_err(netdev, "Failed to allocate more tx queues, returning to %d queues\n",
 			   old);
+		alloc_rc = rc;
 		goal = old;
 		old = i;
 		break;
@@ -1213,6 +1214,8 @@ static int ibmveth_set_channels(struct net_device *netdev,
 			   old);
 		goal = old;
 		old = i;
+	} else if (alloc_rc) {
+		rc = alloc_rc;
 	}
 	/* Free any that are no longer needed */
 	for (i = old; i > goal; i--) {
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 7/9] ibmveth: wait for in-flight transmits in ibmveth_close()

[Mingming Cao]

ibmveth_close() frees the TX buffers after netif_tx_stop_all_queues(),
which does not wait for an ibmveth_start_xmit() already running on
another CPU. That transmit can copy into a freed buffer and hand PHYP
a stale DMA address.

MTU, csum/TSO and buffer pool changes call ibmveth_close() directly
while traffic flows. ifdown and the reset work go through dev_close(),
which waits for running transmits only when the qdisc has an enqueue
function, so with noqueue they race the same way.

Use netif_tx_disable(), which waits for running transmits.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection; the race was not reproduced. Tested on a
POWER10 LPAR with MTU changes during a ping flood, with no warnings. No
kernel selftests cover ibmveth.

Fixes: d6832ca48d8a ("ibmveth: Copy tx skbs into a premapped buffer")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v2:
- commit message: dev_close() waits for running transmits only when
  the qdisc has an enqueue function, so ifdown and the reset work race
  the same way with noqueue

 drivers/net/ethernet/ibm/ibmveth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 48bc4406396b..f2570d637301 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -835,7 +835,7 @@ static int ibmveth_close(struct net_device *netdev)
 
 	napi_disable(&adapter->napi);
 
-	netif_tx_stop_all_queues(netdev);
+	netif_tx_disable(netdev);
 
 	h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
 
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 8/9] ibmveth: wait for the RX poll to return before freeing the RX queue

[Mingming Cao]

napi_disable() returns once ibmveth_poll() has called
napi_complete_done(), but the poll is not finished: it then re-enables
the interrupt and calls ibmveth_rxq_pending_buffer(), which reads the
RX queue. ibmveth_close() can free that queue in the meantime, and the
late enable can leave the interrupt unmasked after close() masked it.
The request_irq() failure path in ibmveth_open() frees the same memory
after napi_disable() too.

Call synchronize_net() after napi_disable() on both paths. Every
caller of ibmveth_poll() runs it with bottom halves or interrupts
disabled, so this waits for the poll to return.

Found by AI-assisted review of the ibmveth multi-queue RX series and
confirmed by code inspection; also raised by the Sashiko AI review of
the first version of this series. The race was not reproduced. Tested
on a POWER10 LPAR under an incoming ping flood with 30 rapid link
down/up cycles and 30 rapid MTU cycles (1500 <-> 9000); ran cleanly
with no warnings or faults. No kernel selftests cover ibmveth.

Fixes: bea3348eef27 ("[NET]: Make NAPI polling independent of struct net_device objects.")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
Changes in v2:
- new patch; raised by the Sashiko review of v1 patch 1 as a
  pre-existing bug

 drivers/net/ethernet/ibm/ibmveth.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index f2570d637301..85e176d0ed32 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -768,6 +768,7 @@ static int ibmveth_open(struct net_device *netdev)
 			 netdev);
 	if (rc != 0) {
 		napi_disable(&adapter->napi);
+		synchronize_net();
 		netdev_err(netdev, "unable to request irq 0x%x, rc %d\n",
 			   netdev->irq, rc);
 		goto out_free_buffer_pools;
@@ -834,6 +835,11 @@ static int ibmveth_close(struct net_device *netdev)
 	netdev_dbg(netdev, "close starting\n");
 
 	napi_disable(&adapter->napi);
+	/* napi_disable() returns once ibmveth_poll() has called
+	 * napi_complete_done(), but the poll still re-enables the
+	 * interrupt and reads the RX queue after that.
+	 */
+	synchronize_net();
 
 	netif_tx_disable(netdev);
 
-- 
2.50.1 (Apple Git-155)

[PATCH net-next v4 9/9] ibmveth: check headers in the RX large send and checksum helpers

[Mingming Cao]

ibmveth_rx_mss_helper() and ibmveth_rx_csum_helper() trust the IP and
TCP headers of received frames, which come from another partition on
the virtual LAN.

The worst case is an IPv4 frame with ihl 0 and iph->check 0xffff: the
checksum helper calls ip_fast_csum(iph, 0), and the ppc64 version
loops ihl - 1 times on an unsigned count, reading up to 16 GiB in
softirq: a long stall, or an oops once the read leaves mapped memory.
Both helpers also read and write header fields past the end of a short
frame. The large send helper takes the segment size from the TCP
checksum field and gso_segs from a TCP header length it never checks,
and treats an IPv4 TCP fragment as a large send, which can zero two of
its bytes; the checksum helper can write a checksum into a later
fragment's payload.

The ihl problem dates from the first commit in Fixes:, the large send
helper from the second, and the checksum helper from the third.

Before using a header, check that it fits the frame, that the IPv4 ihl
is at least 5, and that an IPv4 TCP frame is not a fragment. In the
large send helper, also require a TCP doff of at least 5 and TCP
payload within the IP length, the length the stack trims the frame to,
so Ethernet padding does not count; compute gso_segs from the IP
length; and keep the segment size at or above TCP_MIN_GSO_SIZE, as
virtio_net_hdr_to_skb() does since
commit c27c449d455a ("virtio-net: Ensure that TCP packets don't
overflow gso_segs"),
and below GSO_BY_FRAGS, which virtio rejects. With the headers checked
and gso_segs computed, the skb is not marked SKB_GSO_DODGY, which
would also keep it from hardware TSO on the egress device.

A frame that fails a check is passed up without the GSO fields or
CHECKSUM_UNNECESSARY, so the stack checks it. The skb is linear, so
the checks use skb_headlen(); pskb_may_pull() could reallocate the
head under the helpers' header pointers.

Add KUnit cases for both helpers: valid IPv4 and IPv6 large sends, the
segment size clamp, and refused frames. With any one of the ihl, doff,
IP length, fragment or clamp checks, or the checksum helper's IPv4
options, IPv6 header or TCP header length checks, removed, a case
fails; without the checksum helper's ihl check, ip_fast_csum() oopses
on qemu.

Found by the Sashiko AI review of the previous version of this series
and by AI-assisted review, and confirmed by code inspection and the
KUnit cases above. Tested with KUnit on qemu pseries (ppc64le) and on
a POWER10 LPAR with iperf3 large sends over IPv4 and IPv6 at MTU 1500
and 9000, threaded NAPI and busy polling, with no new drops or
InCsumErrors. No kernel selftests cover ibmveth.

Fixes: 9c7e8bc584f5 ("ibmveth: Add support for Large Receive Offload")
Fixes: 7b5967389f5a ("ibmveth: set correct gso_size and gso_type")
Fixes: 66aa0678efc2 ("ibmveth: Support to enable LSO/CSO for Trunk VEA.")
Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 263 ++++++++++++++++++++++++++++-
 1 file changed, 254 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 85e176d0ed32..1c2ac88c28ca 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1396,8 +1396,12 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
 
 }
 
-static void ibmveth_rx_mss_helper(struct sk_buff *skb, u16 mss, int lrg_pkt)
+/* Returns false when the IP or TCP headers fail the checks; the frame is
+ * then left unchanged for the stack to check.
+ */
+static bool ibmveth_rx_mss_helper(struct sk_buff *skb, u16 mss, int lrg_pkt)
 {
+	unsigned int gso_type, ip_len;
 	struct tcphdr *tcph;
 	int offset = 0;
 	int hdr_len;
@@ -1406,28 +1410,41 @@ static void ibmveth_rx_mss_helper(struct sk_buff *skb, u16 mss, int lrg_pkt)
 	if (skb->protocol == htons(ETH_P_IP)) {
 		struct iphdr *iph = (struct iphdr *)skb->data;
 
+		if (skb_headlen(skb) < sizeof(*iph) || iph->ihl < 5)
+			return false;
 		if (iph->protocol == IPPROTO_TCP) {
+			/* only a whole datagram has a TCP header to segment */
+			if (ip_is_fragment(iph))
+				return false;
 			offset = iph->ihl * 4;
-			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+			ip_len = ntohs(iph->tot_len);
+			gso_type = SKB_GSO_TCPV4;
 		} else {
-			return;
+			return true;
 		}
 	} else if (skb->protocol == htons(ETH_P_IPV6)) {
 		struct ipv6hdr *iph6 = (struct ipv6hdr *)skb->data;
 
+		if (skb_headlen(skb) < sizeof(*iph6))
+			return false;
 		if (iph6->nexthdr == IPPROTO_TCP) {
 			offset = sizeof(struct ipv6hdr);
-			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+			ip_len = offset + ntohs(iph6->payload_len);
+			gso_type = SKB_GSO_TCPV6;
 		} else {
-			return;
+			return true;
 		}
 	} else {
-		return;
+		return true;
 	}
 	/* if mss is not set through Large Packet bit/mss in rx buffer,
 	 * expect that the mss will be written to the tcp header checksum.
 	 */
 	tcph = (struct tcphdr *)(skb->data + offset);
+	if (skb_headlen(skb) < offset + sizeof(*tcph) || tcph->doff < 5 ||
+	    ip_len > skb_headlen(skb) || ip_len <= offset + tcph->doff * 4)
+		return false;
+	skb_shinfo(skb)->gso_type = gso_type;
 	if (lrg_pkt) {
 		skb_shinfo(skb)->gso_size = mss;
 	} else if (offset) {
@@ -1436,11 +1453,18 @@ static void ibmveth_rx_mss_helper(struct sk_buff *skb, u16 mss, int lrg_pkt)
 	}
 
 	if (skb_shinfo(skb)->gso_size) {
+		/* chosen by the sending partition: at least the floor
+		 * virtio_net_hdr_to_skb() uses, and never GSO_BY_FRAGS
+		 */
+		skb_shinfo(skb)->gso_size =
+			clamp_t(u16, skb_shinfo(skb)->gso_size,
+				TCP_MIN_GSO_SIZE, GSO_BY_FRAGS - 1);
 		hdr_len = offset + tcph->doff * 4;
 		skb_shinfo(skb)->gso_segs =
-				DIV_ROUND_UP(skb->len - hdr_len,
+				DIV_ROUND_UP(ip_len - hdr_len,
 					     skb_shinfo(skb)->gso_size);
 	}
+	return true;
 }
 
 static void ibmveth_rx_csum_helper(struct sk_buff *skb,
@@ -1457,6 +1481,10 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
 
 	if (skb_proto == ETH_P_IP) {
 		iph = (struct iphdr *)skb->data;
+		if (skb_headlen(skb) < sizeof(*iph) || iph->ihl < 5 ||
+		    skb_headlen(skb) < iph->ihl * 4 ||
+		    (iph->protocol == IPPROTO_TCP && ip_is_fragment(iph)))
+			goto bad_hdr;
 
 		/* If the IP checksum is not offloaded and if the packet
 		 *  is large send, the checksum must be rebuilt.
@@ -1471,6 +1499,8 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
 		iph_proto = iph->protocol;
 	} else if (skb_proto == ETH_P_IPV6) {
 		iph6 = (struct ipv6hdr *)skb->data;
+		if (skb_headlen(skb) < sizeof(*iph6))
+			goto bad_hdr;
 		iphlen = sizeof(struct ipv6hdr);
 		iph_proto = iph6->nexthdr;
 	}
@@ -1493,6 +1523,8 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
 	if (iph_proto == IPPROTO_TCP) {
 		struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
 
+		if (skb_headlen(skb) < iphlen + sizeof(*tcph))
+			goto bad_hdr;
 		if (tcph->check == 0x0000) {
 			/* Recompute TCP pseudo header checksum  */
 			tcphdrlen = skb->len - iphlen;
@@ -1510,6 +1542,13 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
 			skb_reset_network_header(skb);
 		}
 	}
+	return;
+
+bad_hdr:
+	/* headers do not fit the frame, or a fragment has no whole TCP
+	 * segment; let the stack check it
+	 */
+	skb->ip_summed = CHECKSUM_NONE;
 }
 
 static int ibmveth_poll(struct napi_struct *napi, int budget)
@@ -1624,7 +1663,8 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 			/* PHYP without PLSO support places a -1 in the ip
 			 * checksum for large send frames.
 			 */
-			if (skb->protocol == cpu_to_be16(ETH_P_IP)) {
+			if (skb->protocol == cpu_to_be16(ETH_P_IP) &&
+			    skb_headlen(skb) >= sizeof(struct iphdr)) {
 				struct iphdr *iph = (struct iphdr *)skb->data;
 
 				iph_check = iph->check;
@@ -1632,7 +1672,8 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 
 			if ((length > netdev->mtu + ETH_HLEN) ||
 			    lrg_pkt || iph_check == 0xffff) {
-				ibmveth_rx_mss_helper(skb, mss, lrg_pkt);
+				if (!ibmveth_rx_mss_helper(skb, mss, lrg_pkt))
+					csum_good = 0;
 				adapter->rx_large_packets++;
 			}
 
@@ -2508,10 +2549,214 @@ static void ibmveth_rxq_harvest_buffer_test(struct kunit *test)
 	flush_work(&adapter->work);
 }
 
+KUNIT_DEFINE_ACTION_WRAPPER(ibmveth_kfree_skb, kfree_skb, struct sk_buff *);
+
+/* A TCP frame after eth_type_trans(): IP header, 20 byte TCP header and
+ * payload bytes of data, with the IP length matching the frame.
+ */
+static struct sk_buff *ibmveth_test_tcp_skb(struct kunit *test, bool ipv6,
+					    unsigned int payload)
+{
+	unsigned int iplen = ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
+	unsigned int len = iplen + sizeof(struct tcphdr) + payload;
+	struct sk_buff *skb = alloc_skb(len, GFP_KERNEL);
+	struct tcphdr *tcph;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, skb);
+	KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test,
+							   ibmveth_kfree_skb,
+							   skb));
+	skb_put_zero(skb, len);
+	if (ipv6) {
+		struct ipv6hdr *iph6 = (struct ipv6hdr *)skb->data;
+
+		skb->protocol = htons(ETH_P_IPV6);
+		iph6->version = 6;
+		iph6->nexthdr = IPPROTO_TCP;
+		iph6->payload_len = htons(len - iplen);
+	} else {
+		struct iphdr *iph = (struct iphdr *)skb->data;
+
+		skb->protocol = htons(ETH_P_IP);
+		iph->version = 4;
+		iph->ihl = 5;
+		iph->protocol = IPPROTO_TCP;
+		iph->tot_len = htons(len);
+	}
+	tcph = (struct tcphdr *)(skb->data + iplen);
+	tcph->doff = 5;
+	return skb;
+}
+
+/**
+ * ibmveth_rx_mss_helper_test - unit test for ibmveth_rx_mss_helper
+ * @test: pointer to kunit structure
+ *
+ * Large send frames get gso_size from the descriptor or the TCP checksum
+ * field, clamped, and gso_segs from the IP length. Frames whose headers
+ * do not fit, with no TCP payload within the IP length, or that are
+ * IPv4 fragments, are refused and left without GSO.
+ *
+ * Return: void
+ */
+static void ibmveth_rx_mss_helper_test(struct kunit *test)
+{
+	struct sk_buff *skb;
+	struct tcphdr *tcph;
+	struct iphdr *iph;
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	tcph = (struct tcphdr *)(skb->data + sizeof(*iph));
+	tcph->check = (__force __sum16)htons(1448);
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, 0, 0));
+	KUNIT_EXPECT_EQ(test, SKB_GSO_TCPV4, skb_shinfo(skb)->gso_type);
+	KUNIT_EXPECT_EQ(test, 1448, skb_shinfo(skb)->gso_size);
+	KUNIT_EXPECT_EQ(test, 2, skb_shinfo(skb)->gso_segs);
+	KUNIT_EXPECT_EQ(test, 0, (__force u16)tcph->check);
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, 1000, 1));
+	KUNIT_EXPECT_EQ(test, 1000, skb_shinfo(skb)->gso_size);
+	KUNIT_EXPECT_EQ(test, 3, skb_shinfo(skb)->gso_segs);
+
+	skb = ibmveth_test_tcp_skb(test, true, 2896);
+	tcph = (struct tcphdr *)(skb->data + sizeof(struct ipv6hdr));
+	tcph->check = (__force __sum16)htons(1448);
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, 0, 0));
+	KUNIT_EXPECT_EQ(test, SKB_GSO_TCPV6, skb_shinfo(skb)->gso_type);
+	KUNIT_EXPECT_EQ(test, 1448, skb_shinfo(skb)->gso_size);
+	KUNIT_EXPECT_EQ(test, 2, skb_shinfo(skb)->gso_segs);
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, 1, 1));
+	KUNIT_EXPECT_EQ(test, TCP_MIN_GSO_SIZE, skb_shinfo(skb)->gso_size);
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, GSO_BY_FRAGS, 1));
+	KUNIT_EXPECT_EQ(test, GSO_BY_FRAGS - 1, skb_shinfo(skb)->gso_size);
+
+	/* refused: ihl 0, doff 4, IP length past the frame. With ihl 0 the
+	 * TCP header would overlap the IP header; saddr makes its doff 5.
+	 */
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	((struct iphdr *)skb->data)->ihl = 0;
+	((struct iphdr *)skb->data)->saddr = htonl(0x50000000);
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+	KUNIT_EXPECT_EQ(test, 0, skb_shinfo(skb)->gso_size);
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	((struct tcphdr *)(skb->data + sizeof(*iph)))->doff = 4;
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	((struct iphdr *)skb->data)->tot_len = htons(skb->len + 1);
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+
+	/* refused: a fragment past the first, its payload left alone */
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	((struct iphdr *)skb->data)->frag_off = htons(185);
+	tcph = (struct tcphdr *)(skb->data + sizeof(*iph));
+	tcph->check = (__force __sum16)htons(1448);
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 0, 0));
+	KUNIT_EXPECT_EQ(test, 0, skb_shinfo(skb)->gso_size);
+	KUNIT_EXPECT_EQ(test, 1448, ntohs((__force __be16)tcph->check));
+
+	/* refused: only Ethernet padding past the TCP header */
+	skb = ibmveth_test_tcp_skb(test, false, 6);
+	((struct iphdr *)skb->data)->tot_len = htons(40);
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+	KUNIT_EXPECT_EQ(test, 0, skb_shinfo(skb)->gso_size);
+
+	skb = ibmveth_test_tcp_skb(test, true, 2896);
+	((struct ipv6hdr *)skb->data)->payload_len = 0;
+	KUNIT_EXPECT_FALSE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+
+	/* not TCP: accepted and left alone */
+	skb = ibmveth_test_tcp_skb(test, false, 2896);
+	((struct iphdr *)skb->data)->protocol = IPPROTO_UDP;
+	KUNIT_EXPECT_TRUE(test, ibmveth_rx_mss_helper(skb, 1448, 1));
+	KUNIT_EXPECT_EQ(test, 0, skb_shinfo(skb)->gso_size);
+}
+
+/**
+ * ibmveth_rx_csum_helper_test - unit test for ibmveth_rx_csum_helper
+ * @test: pointer to kunit structure
+ *
+ * A large send IPv4 header checksum of 0xffff is rebuilt. Headers that
+ * do not fit the frame, an IPv4 ihl below 5, or a TCP fragment, clear
+ * CHECKSUM_UNNECESSARY instead of being read.
+ *
+ * Return: void
+ */
+static void ibmveth_rx_csum_helper_test(struct kunit *test)
+{
+	struct sk_buff *skb;
+	struct iphdr *iph;
+
+	skb = ibmveth_test_tcp_skb(test, false, 100);
+	iph = (struct iphdr *)skb->data;
+	iph->check = (__force __sum16)0xffff;
+	((struct tcphdr *)(skb->data + sizeof(*iph)))->check = (__force __sum16)htons(1);
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, 0,
+			(__force u16)ip_fast_csum((unsigned char *)iph, iph->ihl));
+	KUNIT_EXPECT_EQ(test, CHECKSUM_UNNECESSARY, (int)skb->ip_summed);
+
+	skb = ibmveth_test_tcp_skb(test, false, 100);
+	iph = (struct iphdr *)skb->data;
+	iph->ihl = 0;
+	iph->check = (__force __sum16)0xffff;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, CHECKSUM_NONE, (int)skb->ip_summed);
+	KUNIT_EXPECT_EQ(test, 0xffff, (__force u16)iph->check);
+
+	/* IPv4 options past the frame; UDP, so no TCP check follows */
+	skb = ibmveth_test_tcp_skb(test, false, 0);
+	iph = (struct iphdr *)skb->data;
+	iph->protocol = IPPROTO_UDP;
+	iph->ihl = 15;
+	iph->check = (__force __sum16)0xffff;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, CHECKSUM_NONE, (int)skb->ip_summed);
+
+	/* a TCP fragment past the first: header and payload left alone */
+	skb = ibmveth_test_tcp_skb(test, false, 100);
+	iph = (struct iphdr *)skb->data;
+	iph->frag_off = htons(185);
+	iph->check = (__force __sum16)0xffff;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, CHECKSUM_NONE, (int)skb->ip_summed);
+	KUNIT_EXPECT_EQ(test, 0xffff, (__force u16)iph->check);
+	KUNIT_EXPECT_EQ(test, 0,
+			(__force u16)((struct tcphdr *)(skb->data +
+							sizeof(*iph)))->check);
+
+	/* TCP header past the frame */
+	skb = ibmveth_test_tcp_skb(test, false, 0);
+	skb_trim(skb, sizeof(struct iphdr));
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, CHECKSUM_NONE, (int)skb->ip_summed);
+
+	/* IPv6 header past the frame; UDP, so no TCP check follows */
+	skb = ibmveth_test_tcp_skb(test, true, 0);
+	((struct ipv6hdr *)skb->data)->nexthdr = IPPROTO_UDP;
+	skb_trim(skb, sizeof(struct ipv6hdr) - 1);
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	ibmveth_rx_csum_helper(skb, NULL);
+	KUNIT_EXPECT_EQ(test, CHECKSUM_NONE, (int)skb->ip_summed);
+}
+
 static struct kunit_case ibmveth_test_cases[] = {
 	KUNIT_CASE(ibmveth_remove_buffer_from_pool_test),
 	KUNIT_CASE(ibmveth_rxq_get_buffer_test),
 	KUNIT_CASE(ibmveth_rxq_harvest_buffer_test),
+	KUNIT_CASE(ibmveth_rx_mss_helper_test),
+	KUNIT_CASE(ibmveth_rx_csum_helper_test),
 	{}
 };
 
-- 
2.50.1 (Apple Git-155)