Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] selftests: net: global dmabuf debugfs parsing causes spurious test failures - [Low] selftests: net: resource leak on exception in devmem unregister tests --
commit 920bd7824b195d91554d5afd53f2bc33d3bbf933 Author: Mina Almasry <[email protected]> Subject: selftests: net: add devmem RX and TX netdev unregister tests This commit adds devmem_bind_rx_unregister_check and devmem_bind_tx_unregister_check to verify that unregistering a netdevsim device synchronously detaches dma_buf attachments. It ensures netlink sockets close cleanly without page faults or use-after-free bugs. > diff --git a/tools/testing/selftests/net/nl_netdev.py > b/tools/testing/selftests/net/nl_netdev.py > index ceb44c8e1fec5..bad230ee9dcd2 100755 > --- a/tools/testing/selftests/net/nl_netdev.py > +++ b/tools/testing/selftests/net/nl_netdev.py [ ... ] > +def _dmabuf_attached_devs(): > + """Return attached device names from debugfs dma_buf/bufinfo if > available.""" > + path = "/sys/kernel/debug/dma_buf/bufinfo" > + if not os.path.exists(path): > + return None > + with open(path, "r", encoding="utf-8") as f: > + text = f.read() > + attached = [] > + in_attached = False > + for line in text.splitlines(): > + if line.strip() == "Attached Devices:": > + in_attached = True > + continue > + if in_attached: > + if line.startswith("\t") and line.strip(): > + attached.append(line.strip()) > + else: > + in_attached = False [Severity: Medium] Will this loop blindly accumulate attached devices for all dma_bufs system-wide without filtering by inode or specific fd? If there are other active dma_bufs on the system, such as from DRM or V4L2 workloads, it looks like their attachments would all be unconditionally appended to this global list. > + return attached > + > + > +def devmem_bind_rx_unregister_check(_nf) -> None: > + """Verify RX devmem bindings detach dma_buf synchronously on netdev > unregister.""" > + dmabuf_fd = _create_udmabuf() > + nf_priv = NetdevFamily() > + ef = EthtoolFamily() > + nsimdev = NetdevSimDev(queue_count=2) > + nsim = nsimdev.nsims[0] > + > + ip(f"link set dev {nsim.ifname} up") [Severity: Low] If a test helper like ip() or nf_priv.bind_rx() raises an exception, will these resources be permanently leaked? The file descriptor from _create_udmabuf() and the NetdevSimDev are created but do not appear to be protected by defer(). If an exception occurs, it could skip the manual cleanup calls at the end of the function. > + ef.rings_set({"header": {"dev-index": nsim.ifindex}, > + "tcp-data-split": "enabled"}) > + nf_priv.bind_rx({ > + "ifindex": nsim.ifindex, > + "fd": dmabuf_fd, > + "queues": [{"id": 1, "type": "rx"}], > + }) > + > + attached = _dmabuf_attached_devs() > + if attached is not None: > + ksft_ge(len(attached), 1) > + > + nsimdev.remove() > + > + attached = _dmabuf_attached_devs() > + if attached is not None: > + ksft_eq(len(attached), 0) [Severity: Medium] Because _dmabuf_attached_devs() aggregates attachments system-wide, does this make the test exceedingly brittle? If any unrelated dma_buf has attachments, this assertion could incorrectly fail and cause spurious test failures in CI environments. > + > + del nf_priv > + os.close(dmabuf_fd) > + > + > +def devmem_bind_tx_unregister_check(_nf) -> None: > + """Verify TX devmem bindings detach cleanly on netdev unregister without > UAF.""" > + dmabuf_fd = _create_udmabuf() > + nf_priv = NetdevFamily() > + nsimdev = NetdevSimDev(queue_count=2) > + nsim = nsimdev.nsims[0] > + > + ip(f"link set dev {nsim.ifname} up") [Severity: Low] Similar to the RX test above, does this code leak the simulated netdev and file descriptor if an exception is raised before the manual cleanup? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
