Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 4
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 spec in Documentation/netlink/specs/gtp.yaml lists the
"gtp" multicast group (lines 168-171), but it never says what the…
- [Medium] Adding gtp.yaml pulls a new compile unit, gtp-user.o, into the
tools/net/ynl library build.
- [Low] drivers/net/gtp.c registers gtp_genl_family with `.version = 0`
(line 2483).
- [Low] gtp.yaml declares `checks: exact-len: 16` for peer-addr6 and ms-
addr6 (lines 71-72 and 79-80).
- [Low] gtp.yaml documents ms-address only as "IPv4 address of the mobile
subscriber served by the context." (line 48).
- [Low] gtp.yaml documents ms-addr6 as "IPv6 address of the mobile
subscriber served by the context." (line 83), and its exact-len: 16…
--
netlink: specs: add genetlink-legacy spec for GTP
This 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. The
spec is also added 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..2c61c4353c89a
> --- /dev/null
> +++ b/Documentation/netlink/specs/gtp.yaml
> @@ -0,0 +1,171 @@
> +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR
> BSD-3-Clause)
> +---
> +name: gtp
> +
> +protocol: genetlink-legacy
[Severity: Low]
Should the spec state the family's real version? With no version key, the
genetlink-legacy schema default of 1 applies. The kernel registers the
family as version 0:
drivers/net/gtp.c:
static struct genl_family gtp_genl_family __ro_after_init = {
.name = "gtp",
.version = 0,
Documentation/netlink/genetlink-legacy.yaml documents "Default is 1" and
sets minimum: 1, so the spec can't say version: 0 unless the schema
changes.
Nothing fails at runtime. ynl hard-codes version 1 in requests and the
genetlink core doesn't check it. Still, the spec gives a different version
from the one the kernel reports in CTRL_ATTR_VERSION and in message
headers.
Could the schema minimum be relaxed so version: 0 can be added? If not,
could the mismatch be documented?
[ ... ]
> + -
> + name: ms-address
> + type: u32
> + byte-order: big-endian
> + display-hint: ipv4
> + doc: IPv4 address of the mobile subscriber served by the context.
[Severity: Low]
Is this description right for echoreq, which also lists ms-address in its
request? gtp_genl_send_echo_req() uses the attribute as the outer IPv4
source address:
dst_ip = nla_get_be32(info->attrs[GTPA_PEER_ADDRESS]);
src_ip = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);
...
rt = ip4_route_output_gtp(&fl4, sk, dst_ip, src_ip);
if (IS_ERR(rt)) {
...
return -ENODEV;
}
So the value is the local GSN tunnel endpoint, not a subscriber address.
The echo response notification works the same way: gtp0_handle_echo_resp_ip()
fills ms-address from the local address (iph->daddr).
A user who follows the spec and passes the subscriber's IPv4 address gets
a route lookup with a non-local source, and the command returns -ENODEV.
Could the attribute doc, or the echoreq operation doc, mention this legacy
overloading?
[ ... ]
> + -
> + name: peer-addr6
> + type: binary
> + checks:
> + exact-len: 16
[Severity: Low]
Does the kernel actually enforce an exact length here? gtp_genl_policy
has:
[GTPA_PEER_ADDR6] = { .len = sizeof(struct in6_addr), },
[GTPA_MS_ADDR6] = { .len = sizeof(struct in6_addr), },
Neither entry sets .type, so both are NLA_UNSPEC. For NLA_UNSPEC,
validate_nla() only checks attrlen < pt->len, which is a minimum length.
Every gtp op also sets GENL_DONT_VALIDATE_STRICT. As a result the kernel
accepts longer attributes, and nla_get_in6_addr() reads the first 16
bytes.
Should the spec say min-len: 16 instead? The other option is to tighten
the kernel policy to NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)). The
same question applies to ms-addr6 below.
[ ... ]
> + -
> + 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]
The driver treats ms-addr6 as a /64 prefix whose lower 64 bits must be
zero. ipv6_pdp_fill() rejects NEWPDP otherwise:
pctx->ms.addr6 = nla_get_in6_addr(info->attrs[GTPA_MS_ADDR6]);
if (pctx->ms.addr6.s6_addr32[2] ||
pctx->ms.addr6.s6_addr32[3])
return false;
gtp_find_pdp_by_link() applies the same check and returns -EADDRNOTAVAIL
for GETPDP and DELPDP lookups:
if (addr.s6_addr32[2] ||
addr.s6_addr32[3])
return ERR_PTR(-EADDRNOTAVAIL);
The current text suggests a full subscriber IPv6 address. A context can't
be created, queried or deleted with one.
Could the doc say this attribute carries the subscriber's /64 prefix with
the low 64 bits zeroed?
> + -
> + name: family
> + type: u8
> + doc: Address family (AF_INET or AF_INET6) of the context addresses.
[Severity: Medium]
Will this spec break the tools/net/ynl build on hosts with older uapi
headers?
tools/net/ynl/generated/Makefile builds every spec except conntrack and
nftables, so gtp-user.c is now generated and compiled. It includes
<linux/gtp.h> and is built with:
-I../lib/ -idirafter $(UAPI_PATH)
Because of -idirafter, the installed /usr/include/linux/gtp.h takes
precedence over include/uapi/linux/gtp.h. The only way around that is a
CFLAGS_gtp entry in tools/net/ynl/Makefile.deps, and there isn't one.
Later patches in the series don't seem to add one either.
GTPA_PEER_ADDR6, GTPA_MS_ADDR6, GTPA_FAMILY and GTP_CMD_ECHOREQ are
fairly recent. On hosts with 5.x or 6.1-era linux-libc-dev, the generated
code would refer to undefined enumerators. The policy array sized by
GTPA_MAX would also be too small, and make -C tools/net/ynl would fail.
Would adding something like this to Makefile.deps fix it?
CFLAGS_gtp:=$(call get_hdr_inc,_LINUX_GTP_H_,gtp.h)
The guard name assumes headers_install strips _UAPI from
_UAPI_LINUX_GTP_H_.
[ ... ]
> + -
> + 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
> + - ms-address
> + - net-ns-fd
> +
> +mcast-groups:
> + list:
> + -
> + name: gtp
[Severity: Medium]
The gtp multicast group is declared, but which messages go out on it? No
operation has notify, event or mcgrp. The driver multicasts three kinds of
message on GTP_GENL_MCGRP. The first two come from these paths:
gtp_genl_new_pdp()->gtp_tunnel_notify(pctx, GTP_CMD_NEWPDP, GFP_KERNEL)
gtp_genl_del_pdp()->gtp_tunnel_notify(pctx, GTP_CMD_DELPDP, GFP_ATOMIC)
gtp_genl_fill_info() builds both, so they carry the same content as the
getpdp reply.
The third is the echo response. gtp0_handle_echo_resp() and the GTPv1-U
handler both send it:
ret = gtp_genl_fill_echo(msg, 0, 0, 0, GTP_CMD_ECHOREQ, echo);
...
return genlmsg_multicast_netns(>p_genl_family, dev_net(gtp->dev),
msg, 0, GTP_GENL_MCGRP, GFP_ATOMIC);
It carries version, peer-address and ms-address. The echoreq doit,
gtp_genl_send_echo_req(), only ACKs, so this multicast is the only way
user space learns the echo result.
nlspec only treats ops with notify or event as asynchronous, so
async_msg_ids is empty for gtp. After ntf_subscribe('gtp'), check_ntf() in
ynl.py prints "Unexpected msg id while checking for ntf". It then drops
every PDP add, PDP delete and echo response.
ynl_gen_c doesn't generate notification parsers for these messages
either, and the rendered docs don't mention them. The commit message says
the spec describes "the existing family", but this part of the uAPI is
missing.
Could newpdp and delpdp notifications be described by reusing the getpdp
reply (notify: getpdp, mcgrp: gtp)? The echo response could be an event
with version, peer-address and ms-address that keeps the legacy command
value. Otherwise, could the spec say explicitly that notifications are not
covered?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010105740.223992-1-anilkaushikwireless%40gmail.com
