Re: [PATCH net-next v2 2/3] netlink: specs: add genetlink-legacy spec for GTP

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · 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 echoreq request attribute list in
  Documentation/netlink/specs/gtp.yaml (lines 159-165: link, version,…
- [Medium] The getpdp reply attribute list (&pdp-reply,
  Documentation/netlink/specs/gtp.yaml lines 138-149), which the dump…
- [Medium] The spec declares the 'gtp' multicast group
  (Documentation/netlink/specs/gtp.yaml lines 167-170) but describes no…
- [Medium] Adding Documentation/netlink/specs/gtp.yaml makes
  tools/net/ynl/generated/Makefile generate and compile gtp-user.c/gtp-…
- [Low] peer-addr6 and ms-addr6 are declared in
  Documentation/netlink/specs/gtp.yaml with `checks: exact-len: 16`…
- [Low] delpdp in Documentation/netlink/specs/gtp.yaml (line 120,
  `request: *pdp-attrs`) reuses the full newpdp request list, so it…

--

netlink: specs: add genetlink-legacy spec for GTP

This commit adds a genetlink-legacy YAML spec for the existing GTP generic
netlink family. It covers the NEWPDP, DELPDP, GETPDP and ECHOREQ commands,
the GTPA_* attribute set and the "gtp" multicast group. It also adds the
spec to the GTP entry in MAINTAINERS.

> diff --git a/Documentation/netlink/specs/gtp.yaml b/Documentation/netlink/specs/gtp.yaml
> new file mode 100644
> index 0000000000000..7193f5e53a257
> --- /dev/null
> +++ b/Documentation/netlink/specs/gtp.yaml
> @@ -0,0 +1,170 @@

[ ... ]

> +      -
> +        name: peer-addr6
> +        type: binary
> +        checks:
> +          exact-len: 16
> +        byte-order: big-endian
> +        display-hint: ipv6
> +        doc: IPv6 address of the remote GSN peer (GGSN or SGSN).
> +      -
> +        name: ms-addr6
> +        type: binary
> +        checks:
> +          exact-len: 16
> +        byte-order: big-endian
> +        display-hint: ipv6
> +        doc: IPv6 address of the mobile subscriber served by the context.

[Severity: Low]
Does exact-len: 16 match what the kernel accepts for these two attributes?
In gtp_genl_policy in drivers/net/gtp.c they have no type:

	[GTPA_PEER_ADDR6]	= { .len = sizeof(struct in6_addr), },
	[GTPA_MS_ADDR6]		= { .len = sizeof(struct in6_addr), },

That makes them NLA_UNSPEC. With the family's liberal validation, .len is
a minimum length, so the kernel accepts longer payloads and
nla_get_in6_addr() reads the first 16 bytes.

ynl_gen_c.py turns exact-len into NLA_POLICY_EXACT_LEN(). A policy
generated from this spec would therefore reject input the kernel accepts
today.

For the same .len = sizeof(struct in6_addr) pattern, mptcp_pm.yaml,
nfsd.yaml and wireguard.yaml use min-len: 16. Should these two use min-len
as well?

> +      -
> +        name: family
> +        type: u8
> +        doc: Address family (AF_INET or AF_INET6) of the context addresses.

[Severity: Medium]
With this spec in place, tools/net/ynl/generated/Makefile will start
generating and compiling gtp-user.c. That Makefile picks up every spec
via SPECS_PATHS=$(wildcard $(SPECS_DIR)/*.yaml), and GENS_UNSUP only
excludes conntrack and nftables.

The compile uses -idirafter $(UAPI_PATH), so #include <linux/gtp.h> finds
the build host's /usr/include/linux/gtp.h before the in-tree uAPI header.
The generated tables use GTPA_PEER_ADDR6, GTPA_MS_ADDR6, GTPA_FAMILY and
GTP_CMD_ECHOREQ.

Older installed kernel headers (for example 6.1-era distro headers) don't
define these identifiers. On such hosts, would make -C tools/net/ynl fail
to build gtp-user.o?

Other families with recent uAPI additions (ovpn, psp, wireguard, ovs_*,
mptcp_pm and others) force the in-tree header in
tools/net/ynl/Makefile.deps. Does gtp need a similar entry, like this:

CFLAGS_gtp:=$(call get_hdr_inc,_LINUX_GTP_H_,gtp.h)

> +
> +operations:
> +  list:

[ ... ]

> +    -
> +      name: delpdp
> +      doc: Delete a PDP context.
> +      attribute-set: gtp
> +      dont-validate: [strict, dump]
> +      flags: [admin-perm]
> +      do:
> +        request: *pdp-attrs

[Severity: Low]
Is reusing the full newpdp request list correct for delpdp?

gtp_genl_del_pdp() only checks GTPA_VERSION, then calls
gtp_find_pdp()->gtp_find_pdp_by_link(). That lookup reads only LINK,
NET_NS_FD (through gtp_find_dev()), FAMILY, MS_ADDRESS, MS_ADDR6, VERSION,
TID and I_TEI.

The kernel silently ignores peer-address, peer-addr6, flow and o-tei on
delete. Listing them here suggests in the spec, generated docs and
generated C API that they help pick which context gets deleted.

The getpdp request list below matches the lookup. Could delpdp use that
list instead?

> +    -
> +      name: getpdp
> +      doc: Get or dump one or more PDP contexts.

[ ... ]

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

[Severity: Medium]
Should link be in this reply list? gtp_genl_fill_info() always emits it:

	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))
		goto nla_put_failure;

That layout is used for GETPDP replies, for dump entries and for the
NEWPDP/DELPDP notifications. ynl-gen-c builds reply structs and parsers
only from the listed attributes, so generated C clients will drop the
ifindex.

gtp_genl_dump_pdp() walks every gtp netdevice in the namespace. The
per-message GTPA_LINK is the only way to tell which device a dumped
context belongs to.

> +    -
> +      name: echoreq
> +      doc: Send a GTP-U echo request to a peer.
> +      attribute-set: gtp
> +      dont-validate: [strict, dump]
> +      flags: [admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - link
> +            - version
> +            - peer-address
> +            - peer-addr6
> +            - family

[Severity: Medium]
This list doesn't seem to match gtp_genl_send_echo_req(), which requires
ms-address:

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

ynl-gen-c only generates setters for the attributes listed for the op, so
a generated C client has no way to send ms-address. Would every echoreq
from such a client fail with -EINVAL?

The handler is also IPv4-only. It uses ip4_route_output_gtp() and an IPv4
udp_tunnel_xmit_skb(), and it never reads GTPA_PEER_ADDR6 or GTPA_FAMILY.
Should peer-addr6 and family be dropped from this list?

net-ns-fd is not listed either, but gtp_find_dev() honours it when
resolving GTPA_LINK for this command:

	if (nla[GTPA_NET_NS_FD])
		net = get_net_ns_by_fd(nla_get_u32(nla[GTPA_NET_NS_FD]));

Finally, the ms-address doc reads "IPv4 address of the mobile subscriber
served by the context". For echoreq the value is used as the local source
address of the echo request instead.

> +
> +mcast-groups:
> +  list:
> +    -
> +      name: gtp

[Severity: Medium]
The gtp multicast group is declared, but no operation has notify:, event:
or mcgrp:. Where are the messages the kernel multicasts on this group
described?

The kernel sends three kinds of message on GTP_GENL_MCGRP:

  gtp_genl_new_pdp()->gtp_tunnel_notify(pctx, GTP_CMD_NEWPDP, ...)
  gtp_genl_del_pdp()->gtp_tunnel_notify(pctx, GTP_CMD_DELPDP, ...)
  gtp0_handle_echo_resp() / gtp1u_handle_echo_resp()
      ->gtp_genl_fill_echo(msg, 0, 0, 0, GTP_CMD_ECHOREQ, echo)

The first two use the gtp_genl_fill_info() layout. The echo responses
carry version, peer-address and ms-address.

In tools/net/ynl/pyynl/lib/ynl.py, pyynl only accepts notifications whose
IDs are in async_msg_ids. YnlFamily.__init__() fills that set only from
async ops.

A ynl client that subscribes to gtp would therefore drop each of these
messages with "Unexpected msg id while checking for ntf". C codegen also
won't produce any notification parsers.

Echo responses only arrive through this multicast. Can a spec-based client
send echoreq but never see the result?

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