Thanks, all four are valid and fixed in v4 The echoreq request now matches gtp_genl_send_echo_req() and link is in the getpdp reply The test creates the gtp device before the family handle (and skips without GTP). And del_unknown_pdp checks ENOENT for both the TEI and MS-address lookups.
On Sat, Oct 10, 2026 at 11:51 AM <[email protected]> wrote: > > 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
