Re: [PATCH net v1 4/4] selftests: net: add devmem RX and TX netdev unregister tests

[sashiko-bot@kernel.org]

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 <almasrymina@google.com>
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/20261010025532.839559-1-almasrymina@google.com?part=4