Re: [PATCH net-next v2 6/6] selftests: net: add leased devmem RX and TX netdev unregister tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new leased tests bind through `_devmem_bind()`, which
  turns every NlError with EOPNOTSUPP into KsftSkipEx.
- [Low] devmem_lease_bind_rx_unregister_check()
  (tools/testing/selftests/net/nl_netdev.py), remove_virt_first=True…
- [Low] devmem_lease_bind_rx_unregister_check() and
  devmem_lease_bind_tx_unregister_check() each run a two-pass loop…

--

selftests: net: add leased devmem RX and TX netdev unregister tests

This commit adds two tests to nl_netdev.py. Each one binds an RX or TX
dmabuf to a single-mode netkit device that leases RX queue 1 from
netdevsim. The tests then check for clean teardown both when the netkit
device is deleted first and when the netdevsim device is removed
directly.

> diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
> index dd0321c0b463a..fa4b5be093257 100755
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py

[ ... ]

> +def devmem_lease_bind_rx_unregister_check(nf) -> None:
> +    """Verify leased RX devmem bindings detach cleanly on virt/phys unregister."""
> +    for remove_virt_first in (False, True):

[ ... ]

> +        _devmem_bind(nf_priv.bind_rx, {
> +            "ifindex": nk_ifindex,
> +            "fd": dmabuf_fd,
> +            "queues": [{"id": res["id"], "type": "rx"}],
> +        })
> +        ksft_ge(_dmabuf_attach_count(dmabuf_fd), 1)
> +
> +        if remove_virt_first:
> +            ip(f"link del dev {nk_ifname}")
> +            phys_q = nf.queue_get({"ifindex": nsim.ifindex, "id": 1,
> +                                   "type": "rx"})
> +            ksft_not_in("dmabuf", phys_q)
> +            ksft_not_in("lease", phys_q)

[Severity: Low]
Do these two absence checks show that unleasing removed anything?

Nothing earlier checks that the physical queue reported dmabuf or lease
while the binding was active. The only check after the bind is:

    ksft_ge(_dmabuf_attach_count(dmabuf_fd), 1)

That count goes up when net_devmem_bind_dmabuf() maps the dma-buf, which
happens before any queue gets the memory provider. So it says nothing
about the physical rxq.

Suppose netdev_nl_queue_fill_mp() stopped reporting dmabuf on the leased
physical queue, or the lease redirect in netif_mp_open_rxq() never
installed the memory provider. These ksft_not_in() checks would still
pass.

nk_qlease.py checks lease reporting, but no selftest checks dmabuf on a
leased physical queue.

Could a ksft_in() check on the physical queue be added right after the
bind?

> +            ksft_ge(_dmabuf_attach_count(dmabuf_fd), 1)
> +            nsimdev.remove()
> +        else:
> +            nsimdev.remove()
> +            ksft_ne(cmd(f"ip link show dev {nk_ifname}", fail=False).ret, 0)

[Severity: Low]
What happens on the second loop pass if this check fails?

ksft_ne() only records the failure and does not raise. If removing
netdevsim no longer cascades to the netkit device, the loop moves on to
the remove_virt_first=True pass with the netkit device still there.

That pass calls _create_netkit_single() again with the same name and
NLM_F_EXCL:

    ifname = f"nk{os.getpid()}"
    ...
                flags=[Netlink.NLM_F_CREATE, Netlink.NLM_F_EXCL],
    ...
    defer(cmd, f"ip link del dev {ifname}", fail=False)

The deferred ip link del only runs when the whole test case ends, not
after each iteration.

Would RTM_NEWLINK then fail with EEXIST? _create_netkit_single() only
turns EOPNOTSUPP and ENOENT into a skip, so it would re-raise that error.

The real cascade failure would then be hidden under an unrelated NlError
traceback, and the remove_virt_first=True case would not run.
devmem_lease_bind_tx_unregister_check() has the same pattern.

> +        ksft_eq(_dmabuf_attach_count(dmabuf_fd), 0)
> +
> +        nf_priv.close()
> +
> +
> +def devmem_lease_bind_tx_unregister_check(nf) -> None:
> +    """Verify leased TX devmem bindings detach cleanly on virt/phys unregister."""
> +    for remove_virt_first in (False, True):

[ ... ]

> +        _devmem_bind(nf_priv.bind_tx, {
> +            "ifindex": nk_ifindex,
> +            "fd": dmabuf_fd,
> +        })

[Severity: Medium]
Can a broken lease lookup be reported as SKIP here rather than FAIL?

_devmem_bind() turns every EOPNOTSUPP into a skip:

    def _devmem_bind(bind, attrs) -> None:
        try:
            bind(attrs)
        except NlError as e:
            if e.error == errno.EOPNOTSUPP:
                raise KsftSkipEx(f"devmem binding not supported: {e}") from e

netkit sets dev->netmem_tx = NETMEM_TX_NO_DMA. So this bind only succeeds
if netdev_find_netmem_tx_dev() walks the netkit rx queues, finds
rxq->lease, and returns the physical device. If that walk fails,
netdev_nl_bind_tx_doit() returns the same errno:

    map_dev = netdev_find_netmem_tx_dev(netdev);
    if (!map_dev) {
        err = -EOPNOTSUPP;
        NL_SET_ERR_MSG(info->extack,
                       "No DMA-capable device found for netmem TX");

The leased RX bind in devmem_lease_bind_rx_unregister_check() has the
same problem. netif_mp_open_rxq() returns -EOPNOTSUPP when the lease
redirect resolves to a device without a parent ("rx queue belongs to a
virtual netdev"), and __netif_mp_open_rxq() does the same when there are
no qops.

The skip is needed when netdevsim has no DMA mask and so does not set
NETMEM_TX_DMA. But the wrapper cannot tell that case apart from a lease
resolution failure.

Would it work to skip only if a direct bind on the nsim device also
returns EOPNOTSUPP, or to check the nsim DMA capability up front?

> +        ksft_ge(_dmabuf_attach_count(dmabuf_fd), 1)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108055809.2122934-1-almasrymina%40google.com