Re: [PATCH net-next v3 3/3] selftests: net: add a test for the gtp netlink family

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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 test aborts with a traceback instead of running or
  skipping whenever the 'gtp' genl family is not already registered.
- [Medium] The `echoreq` operation in
  Documentation/netlink/specs/gtp.yaml (lines 159-165) declares its…
- [Low] del_unknown_pdp() in tools/testing/selftests/net/gtp.py uses
  `with ksft_raises(NlError)` without checking the errno, so any netlink…
- [Low] The getpdp `&pdp-reply` attribute set in
  Documentation/netlink/specs/gtp.yaml (lines 138-149, used by getpdp do…

--

selftests: net: add a test for the gtp netlink family

This adds a Python selftest that drives the gtp generic netlink family
through its ynl spec. It covers newpdp, getpdp and delpdp, a two-context
dump, and a delete of an unknown context. It also adds a GtpFamily ynl
wrapper, CONFIG_GTP=m in the net selftest config, and Makefile and
MAINTAINERS entries.

> diff --git a/tools/testing/selftests/net/gtp.py b/tools/testing/selftests/net/gtp.py
> new file mode 100755
> index 0000000000000..fcc9bf57a663f
> --- /dev/null
> +++ b/tools/testing/selftests/net/gtp.py
> @@ -0,0 +1,85 @@

[ ... ]

> +def add_del_pdp(gtp, ns) -> None:
> +    """Add a PDP context, query it, then delete it.
> +
> +    The query must return the context with the TEIDs and addresses
> +    it was created with, and it must be gone after the delete.
> +    """
> +    link = _add_gtp_dev(ns)
> +
> +    gtp.newpdp({"link": link, "version": 1,
> +                "i-tei": 0x100, "o-tei": 0x200,
> +                "ms-address": "10.0.0.1", "peer-address": "192.0.2.1"})
> +
> +    pdp = [c for c in gtp.getpdp({}, dump=True) if c.get("i-tei") == 0x100]

[Severity: Low]
This dump is decoded with Documentation/netlink/specs/gtp.yaml. That spec
was added earlier in this series by "netlink: specs: add genetlink-legacy
spec for GTP", and its getpdp reply leaves out link:

        reply: &pdp-reply
          attributes:
            - version
            - tid
            - peer-address
            - peer-addr6
            - ms-address
            - ms-addr6
            - flow
            - i-tei
            - o-tei
            - family

But gtp_genl_fill_info() always emits it, in every getpdp reply, dump entry
and notification:

	if (nla_put_u32(skb, GTPA_VERSION, pctx->gtp_version) ||
	    nla_put_u32(skb, GTPA_LINK, pctx->dev->ifindex) ||
	    nla_put_u8(skb, GTPA_FAMILY, pctx->af))

Should link be added to the reply attributes? Without it, consumers
generated from the spec can't tell which gtp device a dumped context
belongs to.

On a related note, this filter matches only on i-tei, not on the link the
test just created.

> +    ksft_eq(len(pdp), 1)
> +    ksft_eq(pdp[0]["o-tei"], 0x200)
> +    ksft_eq(pdp[0]["ms-address"], "10.0.0.1")
> +    ksft_eq(pdp[0]["peer-address"], "192.0.2.1")
> +
> +    gtp.delpdp({"link": link, "version": 1,
> +                "i-tei": 0x100, "ms-address": "10.0.0.1"})
> +    ksft_not_in(0x100, _iteis(gtp.getpdp({}, dump=True)))

[ ... ]

> +def del_unknown_pdp(gtp, ns) -> None:
> +    """Deleting a context that was never added must fail."""
> +    link = _add_gtp_dev(ns)
> +    with ksft_raises(NlError):
> +        gtp.delpdp({"link": link, "version": 1,
> +                    "i-tei": 0x999, "ms-address": "10.9.9.9"})

[Severity: Low]
Should this check the errno too? ksft_raises(NlError) accepts any netlink
error. For example, -EINVAL or -ENODEV from gtp_find_pdp_by_link() or
gtp_genl_del_pdp() would also pass. The real lookup miss in gtp_find_pdp()
is -ENOENT.

Also, both delpdp requests in this test send ms-address, and
gtp_find_pdp_by_link() checks that before the TEI branch:

drivers/net/gtp.c:gtp_find_pdp_by_link() {
    ...
	if (nla[GTPA_MS_ADDRESS]) {
		__be32 ip = nla_get_be32(nla[GTPA_MS_ADDRESS]);
		if (family != AF_INET)
			return ERR_PTR(-EINVAL);
		return ipv4_pdp_find(gtp, ip);
    ...
}

That means the i-tei values passed to delpdp are ignored, and delete by
TEI through gtp1_pdp_find() is never tested.

With the current kernel this request does return -ENOENT, so the test
passes for the right reason today. It just checks less than the docstring
suggests.

> +
> +
> +def main() -> None:
> +    """Run the PDP context tests in a temporary netns."""
> +    with NetNS() as ns:
> +        with NetNSEnter(str(ns)):
> +            gtp = GtpFamily()
> +        ksft_run([add_del_pdp, add_two_pdp, del_unknown_pdp], args=(gtp, ns))

[Severity: Medium]
Can GtpFamily() succeed here if gtp.ko isn't loaded yet?

GtpFamily() -> YnlFamily.__init__() -> GenlProtocol('gtp') looks up the
family id with _genl_load_families(). That sends CTRL_CMD_GETFAMILY with
NLM_F_DUMP, which the kernel answers from ctrl_dumpfamily(). Only the
non-dump ctrl_getfamily() path autoloads the module:

net/netlink/genetlink.c:ctrl_getfamily() {
    ...
	request_module("net-pf-%d-proto-%d-family-%s",
		       PF_NETLINK, NETLINK_GENERIC, name);
    ...
}

This patch adds CONFIG_GTP=m to the selftest config. On a fresh boot with
that config, the "gtp" family isn't registered yet, so
genl_family_name_to_id['gtp'] raises KeyError. YnlFamily.__init__()
turns that into:

    YnlException(f"Family '{self.yaml['name']}' not supported by the kernel")

That exception isn't a KsftSkipEx and is raised outside ksft_run(). Won't
the whole script abort with a traceback before any case runs?

gtp.ko would only be autoloaded later, through the rtnl link alias, when
_add_gtp_dev() runs "ip link add gtp0 type gtp". For the same reason, with
CONFIG_GTP unset the KsftSkipEx in _add_gtp_dev() is never reached, and the
test fails instead of skipping.

Would it work to create the gtp device, or otherwise load the module,
before constructing GtpFamily(), and to treat a missing family as a skip?

> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()

[ ... ]

> diff --git a/tools/testing/selftests/net/lib/py/ynl.py b/tools/testing/selftests/net/lib/py/ynl.py
> index 08deff756f29b..7edb749710231 100644
> --- a/tools/testing/selftests/net/lib/py/ynl.py
> +++ b/tools/testing/selftests/net/lib/py/ynl.py

[ ... ]

> @@ -85,3 +85,9 @@ class PSPFamily(YnlFamily):
>      def __init__(self, recv_size=0):
>          super().__init__((SPEC_PATH / Path('psp.yaml')).as_posix(),
>                           schema='', recv_size=recv_size)
> +
> +
> +class GtpFamily(YnlFamily):
> +    def __init__(self, recv_size=0):
> +        super().__init__((SPEC_PATH / Path('gtp.yaml')).as_posix(),
> +                         schema='', recv_size=recv_size)

[Severity: Medium]
This wrapper loads the gtp.yaml added earlier in this series by
"netlink: specs: add genetlink-legacy spec for GTP". Does that spec's
echoreq request match the driver? The spec lists:

        request:
          attributes:
            - link
            - version
            - peer-address
            - peer-addr6
            - family

gtp_genl_send_echo_req() requires GTPA_MS_ADDRESS, which isn't listed:

	if (!info->attrs[GTPA_VERSION] ||
	    !info->attrs[GTPA_LINK] ||
	    !info->attrs[GTPA_PEER_ADDRESS] ||
	    !info->attrs[GTPA_MS_ADDRESS])
		return -EINVAL;

The handler is also IPv4-only and never reads GTPA_PEER_ADDR6 or
GTPA_FAMILY.

A client generated from the spec would never send ms-address, so its
echoreq would always fail with -EINVAL.

That spec commit says it is "derived directly from include/uapi/linux/gtp.h
and the gtp_genl_policy / gtp_genl_ops tables in drivers/net/gtp.c". That
doesn't seem to hold for echoreq.

Should ms-address be added to the echoreq request, and peer-addr6 and
family removed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009060615.4177402-1-anilkaushikwireless%40gmail.com