Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag

[sashiko-bot@kernel.org]

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

New issues:
- [High] Forcing `port_is_up = true` in the `mana_attach()` error path leaves the port state inconsistent, leading to a double-teardown of hardware queues and a NULL pointer dereference in stats gathering.
- [High] The device restart sequence in `mana_set_priv_flags()` lacks `vport_mutex` synchronization and does not set `apc->channel_changing`, creating a race condition with concurrent RDMA operations.

Pre-existing issues:
- [High] Bulk pre-allocation of RX buffers uses GFP_ATOMIC and emergency memory reserves in a process context (ethtool), which can lead to memory depletion and spurious failures.
- [High] Pre-allocated RX buffers with `from_pool = false` are leaked when dropped in a queue configured with `frag_count > 1`.
--

commit 40ed2f3da9bc06c10116aa35575da8c740d52002
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

net: mana: force full-page RX buffers via ethtool private flag
    
This patch adds an ethtool private flag "full-page-rx" to force one RX
buffer per page on ARM64 platforms. The single-buffer-per-page logic is
consolidated into a new helper function. The driver is detached and
reattached when the flag is modified to apply the new buffer configurations.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf433..24f6ce63b031b 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
>  	return 0;
>  }
>  
> +static u32 mana_get_priv_flags(struct net_device *ndev)
> +{
> +	struct mana_port_context *apc = netdev_priv(ndev);
> +
> +	return apc->priv_flags;
> +}
> +
> +static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
> +{
> +	struct mana_port_context *apc = netdev_priv(ndev);
> +	u32 changed = apc->priv_flags ^ priv_flags;
> +	u32 old_priv_flags = apc->priv_flags;
> +	bool schedule_port_reset = false;
> +	int err = 0;
> +
> +	if (!changed)
> +		return 0;
> +
> +	/* Reject unknown bits */
> +	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
> +		return -EINVAL;
> +
> +	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> +		apc->priv_flags = priv_flags;
> +
> +		if (!apc->port_is_up) {
> +			/* Port is down, flag updated to apply on next up
> +			 * so just return.
> +			 */
> +			return 0;
> +		}
> +
> +		/* Pre-allocate buffers to prevent failure in mana_attach
> +		 * later
> +		 */
> +		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);

[Severity: High]
This is a pre-existing issue, but does calling mana_pre_alloc_rxbufs() from
ethtool context cause memory allocation problems?

mana_pre_alloc_rxbufs() calls dev_alloc_pages() in a bulk loop, which uses
GFP_ATOMIC and emergency memory reserves. Since this is now triggered from
process context via ethtool, could this deplete system emergency reserves
intended for atomic receive paths, leading to spurious failures or network
drops under memory pressure?

[Severity: High]
This isn't a bug introduced by this patch, but does toggling this flag
dynamically create conditions that permanently leak pre-allocated buffers?

When a packet is dropped, mana_rx_skb() checks if from_pool is false (which
it is for pre-allocated buffers) and unconditionally saves the buffer to
rxq->xdp_save_va. However, mana_get_rxfrag() only reuses xdp_save_va if
rxq->frag_count == 1. 

If this flag causes the queue to operate with frag_count > 1 after buffers
are pre-allocated (e.g. by disabling the flag), xdp_save_va is ignored.
Subsequent drops will trigger WARN_ON_ONCE(rxq->xdp_save_va) and permanently
leak the old buffer.

> +		if (err) {
> +			netdev_err(ndev,
> +				   "Insufficient memory for new allocations\n");
> +			apc->priv_flags = old_priv_flags;
> +			return err;
> +		}
> +
> +		err = mana_detach(ndev, false);

[Severity: High]
Does this device restart sequence lack vport_mutex synchronization with
concurrent RDMA operations?

In mana_set_channels(), detach and re-attach is protected by asserting
apc->channel_changing under vport_mutex. Here, mana_detach() tears down
the vport (dropping vport_use_count to 0) without setting
apc->channel_changing. 

Can an RDMA process concurrently call mana_cfg_vport(), observe a zero use
count, and successfully claim the vport before mana_attach() blindly
attempts to configure the exact same vport, resulting in conflicting
hardware programming?

> +		if (err) {
> +			netdev_err(ndev, "mana_detach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Port is in an inconsistent state. Restore
> +			 * 'port_is_up' so that queue reset work handler
> +			 * can properly detach and re-attach.
> +			 */
> +			apc->port_is_up = true;
> +			schedule_port_reset = true;
> +			goto out;
> +		}
> +
> +		err = mana_attach(ndev);
> +		if (err) {
> +			netdev_err(ndev, "mana_attach failed: %d\n", err);
> +			apc->priv_flags = old_priv_flags;
> +
> +			/* Restore 'port_is_up' so the reset work handler
> +			 * can properly detach/attach. Without this,
> +			 * the handler sees port_is_up=false and skips
> +			 * queue allocation, leaving the port dead.
> +			 */
> +			apc->port_is_up = true;

[Severity: High]
Does forcing port_is_up to true in this error path lead to a NULL pointer
dereference in stats gathering?

The internal failure path of mana_attach() calls
mana_cleanup_port_context(), which frees apc->rxqs and sets it to NULL. 

By forcing apc->port_is_up = true here, if userspace queries stats via
ethtool -S or ip -s link, mana_get_stats64() and mana_get_ethtool_stats()
will bypass their safety checks and blindly dereference the NULL
apc->rxqs[q] array.

> +			schedule_port_reset = true;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701141808.461554-1-dipayanroy@linux.microsoft.com?part=2