| Message ID | 16be6307909b25852744a67b2caf570efbb83c7f.1684502478.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe79bd65c819cc520aa66de65caae8e4cea29c5a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/tcp: refactor tcp_inet6_sk() | expand |
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> ...
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!
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
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.
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
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.
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;
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(-)