[PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Pavel Begunkov]

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@gmail.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

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Simon Horman]

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@gmail.com>

Thanks,

this looks good to me.

For the record: looking at the x86_64 assembly (objdump -d) before and
after it seems that the offset is consistently 3064 (0xbf8). Which is
consistent with the layout of tcp6_sock reported by pahole.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 19 May 2023 14:30:36 +0100 you 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().
> 
> [...]

Here is the summary with links:
  - [net-next] net/tcp: refactor tcp_inet6_sk()
    https://git.kernel.org/netdev/net-next/c/fe79bd65c819

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Jason A. Donenfeld]

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@gmail.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

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Eric Dumazet]

On Fri, Aug 4, 2023 at 4:51 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> 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@gmail.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

I think my patch fixed this issue, can you double check ?

f5f80e32de12fad2813d37270e8364a03e6d3ef0 ipv6: remove hard coded
limitation on ipv6_pinfo

I was not sure if Pavel was problematic or not, I only guessed.

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Jason A. Donenfeld]

Hi Eric,

On Fri, Aug 04, 2023 at 04:57:04PM +0200, Eric Dumazet wrote:
> I think my patch fixed this issue, can you double check ?
> 
> f5f80e32de12fad2813d37270e8364a03e6d3ef0 ipv6: remove hard coded
> limitation on ipv6_pinfo
> 
> I was not sure if Pavel was problematic or not, I only guessed.

That appears to fix the issue indeed, thanks. As this is only in
net-next, you may want to pick it into net for 6.5.

Jason

Re: [PATCH net-next] net/tcp: refactor tcp_inet6_sk()

[Eric Dumazet]

On Fri, Aug 4, 2023 at 5:02 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Eric,
>
> On Fri, Aug 04, 2023 at 04:57:04PM +0200, Eric Dumazet wrote:
> > I think my patch fixed this issue, can you double check ?
> >
> > f5f80e32de12fad2813d37270e8364a03e6d3ef0 ipv6: remove hard coded
> > limitation on ipv6_pinfo
> >
> > I was not sure if Pavel was problematic or not, I only guessed.
>
> That appears to fix the issue indeed, thanks. As this is only in
> net-next, you may want to pick it into net for 6.5.

Sure, we will mark this patch as a stable candidate for 6.5

Thanks.