netdev - Re: [PATCH net-next] net/tcp: refactor tcp_inet6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZM0QHZNKLQ9kVlJ8@zx2c4.com>
Date: Fri, 4 Aug 2023 16:50:05 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, davem@...emloft.net,
	dsahern@...nel.org, pabeni@...hat.com, kuba@...nel.org
Subject: Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

Hi Pavel,

On Fri, May 19, 2023 at 02:30:36PM +0100, Pavel Begunkov wrote:
> Don't keep hand coded offset caluclations and replace it with
> container_of(). It should be type safer and a bit less confusing.
> 
> It also makes it with a macro instead of inline function to preserve
> constness, which was previously casted out like in case of
> tcp_v6_send_synack().
> 
> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
> ---
>  net/ipv6/tcp_ipv6.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 7132eb213a7a..d657713d1c71 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -93,12 +93,8 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(const struct sock *sk,
>   * This avoids a dereference and allow compiler optimizations.
>   * It is a specialized version of inet6_sk_generic().
>   */
> -static struct ipv6_pinfo *tcp_inet6_sk(const struct sock *sk)
> -{
> -	unsigned int offset = sizeof(struct tcp6_sock) - sizeof(struct ipv6_pinfo);
> -
> -	return (struct ipv6_pinfo *)(((u8 *)sk) + offset);
> -}
> +#define tcp_inet6_sk(sk) (&container_of_const(tcp_sk(sk), \
> +					      struct tcp6_sock, tcp)->inet6)
>  
>  static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
>  {
> @@ -533,7 +529,7 @@ static int tcp_v6_send_synack(const struct sock *sk, struct dst_entry *dst,
>  			      struct sk_buff *syn_skb)
>  {
>  	struct inet_request_sock *ireq = inet_rsk(req);
> -	struct ipv6_pinfo *np = tcp_inet6_sk(sk);
> +	const struct ipv6_pinfo *np = tcp_inet6_sk(sk);
>  	struct ipv6_txoptions *opt;
>  	struct flowi6 *fl6 = &fl->u.ip6;
>  	struct sk_buff *skb;
> -- 
> 2.40.0

This patch broke the WireGuard test suite on 32-bit platforms:

https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/i686.log
https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/arm.log
https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/armeb.log
https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/powerpc.log
https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/mips.log
https://build.wireguard.com/wireguard-linux-stable/bf400e83708d055bdf442577ed2f2a8eb87a06f2/mipsel.log

The common point of failure in each of these is something like:

[+] NS1: iperf3 -s -1 -B fd00::1
[+] NS1: wait for iperf:5201 pid 115
-----------------------------------------------------------
Server listening on 5201 (test #1)
-----------------------------------------------------------
[+] NS2: iperf3 -Z -t 3 -c fd00::1
[    8.908396] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[    9.955882] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   10.994917] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   12.034269] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   13.073905] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   14.114022] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   16.194810] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   19.074925] wireguard: wg0: Sending keepalive packet to peer 1 (127.0.0.1:2)
[   19.075934] wireguard: wg0: Receiving keepalive packet from peer 2 (127.0.0.1:1)
[   20.273212] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   28.682020] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   30.593430] wireguard: wg0: Sending keepalive packet to peer 1 (127.0.0.1:2)
[   30.595999] wireguard: wg0: Receiving keepalive packet from peer 2 (127.0.0.1:1)
[   45.315640] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   55.560359] wireguard: wg0: Sending keepalive packet to peer 1 (127.0.0.1:2)
[   55.561675] wireguard: wg0: Receiving keepalive packet from peer 2 (127.0.0.1:1)
[   77.961218] wireguard: wg0: Packet has unallowed src IP (::2:0:0) from peer 1 (127.0.0.1:2)
[   88.200150] wireguard: wg0: Sending keepalive packet to peer 1 (127.0.0.1:2)
[   88.201031] wireguard: wg0: Receiving keepalive packet from peer 2 (127.0.0.1:1)
iperf3: error - unable to connect to server: Operation timed out

For some strange reason, the packets appear to have a src IP of
"::2:0:0" instead of fd00::2. It looks like some kind of offset issue, I
suppose. So you may want to revert this or reevaluate the calculation of
`offset` here, as there's something screwy happening on 32-bit systems.

Jason