Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] Lockless traversal of knod_dev_list in IPsec datapath races with list mutations in knod_dev_detach, leading to kernel panic or Use-After-Free. - [High] Lack of hot-unplug detachment and missing net_device refcounts lead to Use-After-Free if hardware is removed. - [High] knod_pass_detach uses a timeout that forces premature freeing of page pool structures, causing Use-After-Free when delayed SKBs are eventually released. - [High] Uninitialized variable 'i' is used in the free_spsc error cleanup path if memory allocation fails. - [High] Hardware SDMA target pages are prematurely recycled if the hardware fence times out, leading to silent data corruption. - [High] PP_FLAG_DMA_MAP is improperly set for a custom memory provider that supplies pre-mapped device memory. - [High] Hardcoded sdma_idx = 0 causes hardware fence polling on the wrong queue. - [Medium] Netlink operations do not reply with a full message for object creation, and notifications ignore NLM_F_ECHO requirements. - [Medium] Direct modification of netdev->features bypasses networking core feature synchronization and notifications. - [Medium] Double-attachment in knod_ipsec_attach corrupts the saved xfrmdev_ops pointer. --
--- Patch [4]: [RFC PATCH net-next 04/13] net: knod: add offload device core and control plane --- commit e070e2966937b9789951030b969d12e24910bb8f Author: Taehee Yoo <[email protected]> Subject: net: knod: add offload device core and control plane Add the KNOD core framework and its genetlink control plane. The core maintains three object types and drives the RX data path over per-queue SPSC rings. The genetlink family exposes attach/detach, inventory, and feature selection. > diff --git a/net/knod/knod_core.c b/net/knod/knod_core.c > index af50a69bca651..[snip] > --- /dev/null > +++ b/net/knod/knod_core.c [ ... ] > desc = ptr; > desc->netmem = dst; > desc->src = src; > desc->off = off; > desc->len = len; > desc->fence_val = fv; > desc->sdma_idx = 0; [Severity: High] Does this hardcoded index cause hardware fence polling on the wrong queue? By setting sdma_idx to 0 instead of the dynamically assigned napi_index, subsequent calls to d2h_fence() in knod_d2h_drain() and knod_pass_flush() will always poll queue 0. This could lead to premature delivery of incomplete DMA buffers or hang indefinitely on multi-queue configurations. [ ... ] > static struct knod_dev *knod_ipsec_find_xdev(struct net_device *dev) > { > struct knod_dev *knodev; > > list_for_each_entry(knodev, &knod_dev_list, list) { > if (knodev->netdev == dev) > return knodev; > } [Severity: High] Can this lockless iteration of knod_dev_list cause a use-after-free or kernel panic? This function is called from the IPsec transmit path via knod_ipsec_xdo_offload_ok(). Concurrently, knod_dev_detach() for other devices modifies knod_dev_list using list_del() and kfree() without RCU protection or locking, meaning this traversal could read poisoned list pointers or freed memory. [ ... ] > knodev->ipsec_orig_xfrmdev_ops = knodev->netdev->xfrmdev_ops; > knodev->ipsec_added_hw_esp = > !(knodev->netdev->features & NETIF_F_HW_ESP); > knodev->netdev->xfrmdev_ops = &knod_ipsec_xfrmdev_ops; [Severity: Medium] Is there protection against double-attachment overwriting the original operations pointer? If knod_ipsec_attach() is called twice, the previously installed proxy operations will be saved as the original xfrmdev_ops. During detach, this proxy will be restored instead of the true hardware operations, breaking the offload and potentially creating recursion loops. > knodev->netdev->features |= NETIF_F_HW_ESP; > knodev->netdev->hw_enc_features |= NETIF_F_HW_ESP; [Severity: Medium] Does directly modifying netdev->features bypass core network synchronization? Dynamically modifying features should typically be done by updating hw_features or wanted_features and invoking netdev_update_features(). Modifying them directly misses the NETDEV_FEAT_CHANGE notification, leaving userspace tools like ethtool out of sync with the hardware state. [ ... ] > static bool hostmem_pp_release_netmem(struct page_pool *pool, netmem_ref > netmem) > { > struct page_pool_hostmem *hm = pool->mp_priv; > unsigned long addr = page_pool_get_dma_addr_netmem(netmem); > > page_pool_clear_pp_info(netmem); > gen_pool_free(hm->genpool, addr, PAGE_SIZE); [Severity: High] Can this lead to a use-after-free when delayed SKBs are released? In knod_pass_detach(), there is a timeout that forcefully frees the wpriv structure (which contains pass_hm) if the page pool hasn't drained within 5 seconds. When the network stack eventually frees those delayed SKBs, this callback will dereference the freed pool->mp_priv (hm) when calling gen_pool_free(). [ ... ] > struct page_pool_params pp = { > .order = 0, > .pool_size = KNOD_PASS_SLOTS, > .nid = NUMA_NO_NODE, > .dev = knodev->netdev->dev.parent, > .dma_dir = DMA_FROM_DEVICE, > .max_len = PAGE_SIZE, > .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | > PP_FLAG_CUSTOM_MEMORY_PROVIDER, [Severity: High] Is PP_FLAG_DMA_MAP appropriate for a custom memory provider returning pre-mapped device memory? Setting PP_FLAG_DMA_MAP instructs the page pool core to automatically unmap the page via dma_unmap_page_attrs() on release. Since hostmem_pp_init uses GTT device addresses instead of standard system RAM DMA mappings, this will result in invalid DMA API unmap operations. [ ... ] > if (desc->src) { > struct page_pool *src_pp = > netmem_get_pp(desc->src); > > page_pool_recycle_direct_netmem(src_pp, > desc->src); > } > page_pool_put_full_netmem(pool, desc->netmem, false); [Severity: High] Could recycling these DMA target pages after a timeout cause silent data corruption? If the polling loop in knod_pass_flush() times out, the pages are recycled back to the active pools immediately. Since the hardware operation was not definitively aborted, the accelerator might complete its delayed DMA write later, silently overwriting whatever new packet data has been placed in that page. [ ... ] > knodev->netdev = knetdev->dev; [Severity: High] Should dev_hold() be called here to take a reference on the net_device? If the device is hot-removed, the hardware driver invokes unregister_netdev() and frees the net_device. knod_netdev_unregister() only removes the object from the list without detaching it. This leaves a dangling pointer in knodev->netdev, which can cause a use-after-free when accessed by Netlink queries like knod_nl_dev_get_dumpit(). [ ... ] > if (knodev->accel_ops->alloc_mem) { > size_t total = pool_size * nqueues; > u64 base_gaddr; > void *base_pool; > void *pool_priv; > > base_pool = knodev->accel_ops->alloc_mem(knodev, total, > &base_gaddr, NULL, &pool_priv); > if (!base_pool) { > err = -ENOMEM; > pr_err("%s: alloc_mem failed\n", __func__); > goto free_spsc; > } [ ... ] > free_spsc: > for (i--; i >= 0; i--) > spsc_destroy(&knodev->wpriv[i].spsc_bds); [Severity: High] Will this loop use an uninitialized variable if alloc_mem() fails? If alloc_mem() fails, the code jumps to free_spsc before 'i' is initialized. The loop then operates on an uninitialized stack value for 'i', leading to an out-of-bounds array access on knodev->wpriv[i] and passing an invalid pointer to spsc_destroy(). > diff --git a/net/knod/knod_nl.c b/net/knod/knod_nl.c > index 77799202abcf4..[snip] > --- /dev/null > +++ b/net/knod/knod_nl.c [ ... ] > genlmsg_multicast_netns(&knod_nl_family, net, ntf, 0, > KNOD_NLGRP_MGMT, GFP_KERNEL); [Severity: Medium] Does this ignore the NLM_F_ECHO flag? Notifications emitted in response to requests should pass the request information to genl_notify() to honor the NLM_F_ECHO flag. Using genlmsg_multicast_netns() drops the request context. [ ... ] > err = knod_dev_attach(knetdev, accel); > if (!err) > knod_nl_notify_dev(knetdev->knodev, KNOD_CMD_DEV_ADD_NTF); > > unlock: > mutex_unlock(&knod_lock); > rtnl_unlock(); > return err; [Severity: Medium] Should this operation reply with a full message? According to Netlink uAPI design rules, NEW/ADD operations must reply with a full message carrying identifying information about the newly created object, rather than just returning zero and ACKing the request. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
