Re: [PATCH net-next v5 08/13] net: mana: do not bail out of mana_detach on dealloc failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dereferencing IP and IPv6 headers without ensuring they are in the linear region of the socket buffer.
- [Medium] Concurrent writers to `tx_stats->syncp` without mutual exclusion in `mana_xdp_xmit`.
--

commit 2b1d4ae0b77c96383014b2acf0502fef542484c7
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

net: mana: do not bail out of mana_detach on dealloc failure

Remove the early return after mana_dealloc_queues() so detach continues
its device and port-context cleanup. The return is currently unreachable:
mana_dealloc_queues() only rejects an up port, and mana_detach() clears
port_is_up before calling it. This is a robustness cleanup, not a fix for
a reachable reset failure.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 62c11af2fc422..5752e3745ccc6 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -4340,10 +4340,8 @@ int mana_detach(struct net_device *ndev, bool from_close)
>  
>  	if (apc->port_st_save) {
>  		err = mana_dealloc_queues(ndev);
> -		if (err) {
> +		if (err)
>  			netdev_err(ndev, "%s failed to deallocate queues: %d\n", __func__, err);
> -			return err;
> -		}
>  	}
>  
>  	if (!from_close) {

[ ... ]

> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> @@ -76,9 +76,9 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames,
>  	}
>  
>  	tx_stats = &apc->tx_qp[q_idx]->txq.stats;
>  
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->xdp_xmit += count;
>  	u64_stats_update_end(&tx_stats->syncp);

[Severity: Medium]
This is a pre-existing issue, but does mana_xdp_xmit() safely update
the stats sync seqcount without mutual exclusion?

The u64_stats_sync API requires writers to be mutually exclusive per syncp
instance. Here, q_idx is calculated as
smp_processor_id() % ndev->real_num_tx_queues, and tx_stats->syncp is
updated without holding __netif_tx_lock.

Because q_idx can be targeted by normal stack TX via
skb_get_queue_mapping(skb), could another CPU concurrently execute
mana_start_xmit() for the same queue (which correctly holds
__netif_tx_lock and updates tx_stats->syncp)?

Also, if there are more online CPUs than configured TX queues, could
multiple CPUs running mana_xdp_xmit() map to the same q_idx and execute
u64_stats_update_begin() simultaneously?

This concurrent write access violates the u64_stats_sync writer rules,
which can corrupt the sequence counter and cause lost or torn updates.

[ ... ]

> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -167,7 +167,7 @@ static unsigned int mana_checksum_info(struct sk_buff *skb)
>  {
>  	if (skb->protocol == htons(ETH_P_IP)) {
>  		struct iphdr *ip = ip_hdr(skb);
>  
>  		if (ip->protocol == IPPROTO_TCP)

[Severity: High]
This isn't a bug introduced by this patch, but does this code safely
dereference the IP and IPv6 headers without ensuring they are in the linear
region of the socket buffer?

In mana_start_xmit(), if skb->ip_summed == CHECKSUM_PARTIAL, it calls
mana_checksum_info(skb). This function directly calls ip_hdr(skb) and
ipv6_hdr(skb), and immediately dereferences ip->protocol and ip6->nexthdr
without calling pskb_may_pull() or using skb_header_pointer().

Since packet headers injected from userspace (e.g. via AF_PACKET/tun with
virtio_net_hdr) can place the IP header entirely in paged fragments rather
than the linear data area (skb->data), couldn't failing to linearize the
header before dereferencing risk reading garbage data or causing
out-of-bounds page faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909222416.884246-1-longli@microsoft.com?part=8