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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] include/uapi/linux/gtp.h defines `enum gtp_version { GTP_V0 = 0,
  GTP_V1, }` for GTPA_VERSION values, and drivers/net/gtp.c compares…
- [Low] The `family` attribute doc in
  Documentation/netlink/specs/gtp.yaml (line 96) reads "Address family…

--

netlink: specs: add genetlink-legacy spec for GTP

This commit adds a genetlink-legacy YAML spec for the existing GTP
generic netlink family. The spec covers the NEWPDP, DELPDP, GETPDP and
ECHOREQ commands, the GTPA_* attribute set and the "gtp" multicast group.
It also adds a CFLAGS_gtp entry to tools/net/ynl/Makefile.deps.

> diff --git a/Documentation/netlink/specs/gtp.yaml b/Documentation/netlink/specs/gtp.yaml
> new file mode 100644
> index 0000000000000..6a6b49ad2f067
> --- /dev/null
> +++ b/Documentation/netlink/specs/gtp.yaml

[ ... ]

> +attribute-sets:
> +  -
> +    name: gtp
> +    name-prefix: gtpa-
> +    attributes:
> +      -
> +        name: link
> +        type: u32
> +        doc: ifindex of the gtp netdevice the context is attached to.
> +      -
> +        name: version
> +        type: u32
> +        doc: GTP version of the context, 0 for GTPv0 or 1 for GTPv1-U.

[Severity: Low]
Should this attribute reference an enum? include/uapi/linux/gtp.h already
defines the values for GTPA_VERSION:

include/uapi/linux/gtp.h:
enum gtp_version {
	GTP_V0 = 0,
	GTP_V1,
};

drivers/net/gtp.c compares against these values, for example in
gtp_pdp_add() and gtp_genl_fill_info():

	switch (pctx->gtp_version) {
	case GTP_V0:
	...
	case GTP_V1:

The spec has no definitions section, so the valid values only appear in
the doc string. That means ynl clients see version as a raw integer and
cannot encode or decode it by name.

Other legacy specs reference their uapi enums like this, for example
fou.yaml uses "enum: encap-type". Would a definitions entry for
gtp-version plus an enum reference on this attribute work here?

[ ... ]

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

[Severity: Low]
Is this description accurate? In drivers/net/gtp.c, GTPA_FAMILY only
selects the family of the mobile subscriber address, and it defaults to
AF_INET when the attribute is absent.

In gtp_pdp_add(), family only picks between GTPA_MS_ADDRESS and
GTPA_MS_ADDR6. The peer address is checked against the socket family
instead:

drivers/net/gtp.c:gtp_pdp_add() {
    ...
	family = nla_get_u8_default(info->attrs[GTPA_FAMILY], AF_INET);
    ...
	if ((info->attrs[GTPA_PEER_ADDRESS] &&
	     sk->sk_family == AF_INET6) ||
	    (info->attrs[GTPA_PEER_ADDR6] &&
	     sk->sk_family == AF_INET))
		return ERR_PTR(-EAFNOSUPPORT);

	switch (family) {
	case AF_INET:
		if (!info->attrs[GTPA_MS_ADDRESS] ||
    ...
}

The reply side uses the same split. gtp_genl_fill_info() picks the MS
attribute by pctx->af and the peer attribute by pctx->sk->sk_family:

drivers/net/gtp.c:gtp_genl_fill_info() {
    ...
	    nla_put_u8(skb, GTPA_FAMILY, pctx->af))
    ...
	switch (pctx->af) {
	case AF_INET:
		if (nla_put_be32(skb, GTPA_MS_ADDRESS, pctx->ms.addr.s_addr))
    ...
	switch (pctx->sk->sk_family) {
	case AF_INET:
		if (nla_put_be32(skb, GTPA_PEER_ADDRESS, pctx->peer.addr.s_addr))
    ...
}

As a result, a valid context with an IPv4 subscriber over IPv6 transport
reports family=AF_INET together with peer-addr6. "of the context
addresses" does not describe that case.

Could the doc say that family applies to the ms-address / ms-addr6
attribute only, and that it defaults to AF_INET when absent?

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