Re: [PATCH net-next v5 08/13] net: mana: do not bail out of mana_detach on dealloc failure
From: sashiko-bot@kernel.org
Date: 2026-09-10 22:25:20
Also in:
linux-rdma
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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) {
[ ... ]
quoted hunk ↗ jump to hunk
--- 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. [ ... ]
quoted hunk ↗ jump to hunk
--- 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