Re: [PATCH net-next v15 04/15] quic: provide family ops for address and protocol

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The commit message describes this patch only as the introduction
  of family helper code ("provide helpers for routing, skb transmit…
- [Low] Both new transmit debug messages format the unsigned skb->len
  field with the signed %d conversion instead of %u:…

--

quic: provide family ops for address and protocol

This adds net/quic/family.{c,h} with IPv4/IPv6 helpers for routing, lower
transmit, address parsing and comparison, MTU info extraction and UDP
tunnel socket configuration. It also wires quic_inet_getname() and the
non-SOL_QUIC setsockopt/getsockopt paths into the new helpers.

> diff --git a/net/quic/Makefile b/net/quic/Makefile

[ ... ]

> diff --git a/net/quic/family.c b/net/quic/family.c
> new file mode 100644
> index 0000000000000..f61a8203b2e17
> --- /dev/null
> +++ b/net/quic/family.c
> @@ -0,0 +1,446 @@

[ ... ]

> +static void quic_v4_lower_xmit(struct sock *sk, struct sk_buff *skb,
> +			       struct flowi *fl)
> +{
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	struct flowi4 *fl4 = &fl->u.ip4;
> +	u8 tos = inet_sk(sk)->tos, ttl;
> +	struct dst_entry *dst;
> +	__be16 df = 0;
> +
> +	pr_debug("%s: skb: %p, len: %d, num: %lld, %pI4:%d -> %pI4:%d\n",
> +		 __func__, skb, skb->len, cb->number, &fl4->saddr,
> +		 ntohs(fl4->fl4_sport), &fl4->daddr, ntohs(fl4->fl4_dport));

[Severity: Low]
This isn't a bug, but should the length conversion be %u here?  skb->len is
declared unsigned in struct sk_buff:

include/linux/skbuff.h:
	unsigned int		len,
				data_len;

The same mismatch appears in quic_v6_lower_xmit() below.

[ ... ]

> +static void quic_v6_lower_xmit(struct sock *sk, struct sk_buff *skb,
> +			       struct flowi *fl)
> +{
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	u8 tc = inet6_sk(sk)->tclass, ttl;
> +	struct flowi6 *fl6 = &fl->u.ip6;
> +	struct dst_entry *dst;
> +	__be32 label;
> +
> +	pr_debug("%s: skb: %p, len: %d, num: %lld, %pI6c:%d -> %pI6c:%d\n",
> +		 __func__, skb, skb->len, cb->number, &fl6->saddr,
> +		 ntohs(fl6->fl6_sport), &fl6->daddr, ntohs(fl6->fl6_dport));

[ ... ]

> diff --git a/net/quic/protocol.c b/net/quic/protocol.c
> index a53a2b1218a63..c5cd6d609d06f 100644
> --- a/net/quic/protocol.c
> +++ b/net/quic/protocol.c
> @@ -47,7 +47,7 @@ static int quic_inet_listen(struct socket *sock, int backlog)
>  static int quic_inet_getname(struct socket *sock, struct sockaddr *uaddr,
>  			     int peer)
>  {
> -	return -EOPNOTSUPP;
> +	return quic_get_sk_addr(sock, uaddr, peer);
>  }
>  
>  static __poll_t quic_inet_poll(struct file *file, struct socket *sock,
> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index c2841caca5dcc..1c97d95c61b2c 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c
> @@ -121,7 +121,8 @@ static int quic_setsockopt(struct sock *sk, int level, int optname,
>  			   sockptr_t optval, unsigned int optlen)
>  {
>  	if (level != SOL_QUIC)
> -		return -EOPNOTSUPP;
> +		return quic_common_setsockopt(sk, level, optname, optval,
> +					      optlen);
>  
>  	return quic_do_setsockopt(sk, optname, optval, optlen);
>  }
> @@ -136,7 +137,8 @@ static int quic_getsockopt(struct sock *sk, int level, int optname,
>  			   char __user *optval, int __user *optlen)
>  {
>  	if (level != SOL_QUIC)
> -		return -EOPNOTSUPP;
> +		return quic_common_getsockopt(sk, level, optname, optval,
> +					      optlen);
>  
>  	return quic_do_getsockopt(sk, optname, USER_SOCKPTR(optval),
>  				  USER_SOCKPTR(optlen));

[Severity: Low]
Could the changelog mention these three behaviour changes?  The commit
message describes the patch as

    "The new quic_family.{c,h} provide helpers for routing, skb transmit
    handling, address parsing and comparison and UDP socket config
    initializing etc."

but beyond adding helpers, three entry points that previously returned
-EOPNOTSUPP now work:

  quic_inet_getname() -> quic_get_sk_addr()
  quic_setsockopt()   -> quic_common_setsockopt() -> ip_setsockopt()/ipv6_setsockopt()
  quic_getsockopt()   -> quic_common_getsockopt() -> ip_getsockopt()/ipv6_getsockopt()

So getsockname()/getpeername() start returning addresses, and every
level != SOL_QUIC option is now passed through to the IP/IPv6 option
handlers.  Some of those values are consumed by code added in this same
patch: inet_sk(sk)->tos in quic_v4_lower_xmit(), inet6_sk(sk)->tclass in
quic_v6_lower_xmit(), ipv6_only_sock() in quic_v6_udp_conf_init(), and
pmtudisc through ip_dont_fragment() and quic_sk_accept_pmtu().

Also, the message names the new files quic_family.{c,h} while the patch
adds net/quic/family.{c,h}.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789393775.git.lucien.xin%40gmail.com