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 <[email protected]> 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/[email protected]?part=8
