[PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Wen Gu]

When copying smc settings to clcsock, avoid setting clcsock's sk_sndbuf
to sysctl_tcp_wmem[1], since this may overwrite the value set by
tcp_sndbuf_expand() in TCP connection establishment.

And the other setting sk_{snd|rcv}buf to sysctl value in
smc_adjust_sock_bufsizes() can also be omitted since the initialization
of smc sock and clcsock has set sk_{snd|rcv}buf to smc.sysctl_{w|r}mem
or ipv4_sysctl_tcp_{w|r}mem[1].

Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when switching between TCP and SMC")
Link: https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
FYI,
The detailed motivation and testing can be found in the link above.
---
 net/smc/af_smc.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 9389f0cfa374..a35281153067 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -459,29 +459,11 @@ static int smc_bind(struct socket *sock, struct sockaddr *uaddr,
 static void smc_adjust_sock_bufsizes(struct sock *nsk, struct sock *osk,
 				     unsigned long mask)
 {
-	struct net *nnet = sock_net(nsk);
-
 	nsk->sk_userlocks = osk->sk_userlocks;
-	if (osk->sk_userlocks & SOCK_SNDBUF_LOCK) {
+	if (osk->sk_userlocks & SOCK_SNDBUF_LOCK)
 		nsk->sk_sndbuf = osk->sk_sndbuf;
-	} else {
-		if (mask == SK_FLAGS_SMC_TO_CLC)
-			WRITE_ONCE(nsk->sk_sndbuf,
-				   READ_ONCE(nnet->ipv4.sysctl_tcp_wmem[1]));
-		else
-			WRITE_ONCE(nsk->sk_sndbuf,
-				   2 * READ_ONCE(nnet->smc.sysctl_wmem));
-	}
-	if (osk->sk_userlocks & SOCK_RCVBUF_LOCK) {
+	if (osk->sk_userlocks & SOCK_RCVBUF_LOCK)
 		nsk->sk_rcvbuf = osk->sk_rcvbuf;
-	} else {
-		if (mask == SK_FLAGS_SMC_TO_CLC)
-			WRITE_ONCE(nsk->sk_rcvbuf,
-				   READ_ONCE(nnet->ipv4.sysctl_tcp_rmem[1]));
-		else
-			WRITE_ONCE(nsk->sk_rcvbuf,
-				   2 * READ_ONCE(nnet->smc.sysctl_rmem));
-	}
 }
 
 static void smc_copy_sock_settings(struct sock *nsk, struct sock *osk,
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Paolo Abeni]

On Fri, 2024-05-31 at 16:54 +0800, Wen Gu wrote:
> When copying smc settings to clcsock, avoid setting clcsock's sk_sndbuf
> to sysctl_tcp_wmem[1], since this may overwrite the value set by
> tcp_sndbuf_expand() in TCP connection establishment.
> 
> And the other setting sk_{snd|rcv}buf to sysctl value in
> smc_adjust_sock_bufsizes() can also be omitted since the initialization
> of smc sock and clcsock has set sk_{snd|rcv}buf to smc.sysctl_{w|r}mem
> or ipv4_sysctl_tcp_{w|r}mem[1].
> 
> Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when switching between TCP and SMC")
> Link: https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
> FYI,
> The detailed motivation and testing can be found in the link above.

My understanding is that there is an open question here if this is the
expected and desired behavior.

@Wenjia, @Jan: could you please have a look?

Thanks!

Paolo

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Wenjia Zhang]

On 04.06.24 16:04, Paolo Abeni wrote:
> On Fri, 2024-05-31 at 16:54 +0800, Wen Gu wrote:
>> When copying smc settings to clcsock, avoid setting clcsock's sk_sndbuf
>> to sysctl_tcp_wmem[1], since this may overwrite the value set by
>> tcp_sndbuf_expand() in TCP connection establishment.
>>
>> And the other setting sk_{snd|rcv}buf to sysctl value in
>> smc_adjust_sock_bufsizes() can also be omitted since the initialization
>> of smc sock and clcsock has set sk_{snd|rcv}buf to smc.sysctl_{w|r}mem
>> or ipv4_sysctl_tcp_{w|r}mem[1].
>>
>> Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when switching between TCP and SMC")
>> Link: https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
>> ---
>> FYI,
>> The detailed motivation and testing can be found in the link above.
> 
> My understanding is that there is an open question here if this is the
> expected and desired behavior.
> 
> @Wenjia, @Jan: could you please have a look?
> 
> Thanks!
> 
> Paolo
> 
> @Paolo, thank you for reminding us!

@Wen, Gerd and I have looked into your patch and discussed on it. Gerd 
would send the concrete answer to your question soon.

Regarding to this patch, it looks good to me. Here you are:

Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Thanks,
Wenjia

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Gerd Bayer]

Hi Wen Gu,

sorry for the late reply, I'm just catching up after a bit of a
vacation.

On Fri, 2024-05-31 at 16:54 +0800, Wen Gu wrote:
> When copying smc settings to clcsock, avoid setting clcsock's
> sk_sndbuf to sysctl_tcp_wmem[1], since this may overwrite the value
> set by tcp_sndbuf_expand() in TCP connection establishment.
> 
> And the other setting sk_{snd|rcv}buf to sysctl value in
> smc_adjust_sock_bufsizes() can also be omitted since the
> initialization of smc sock and clcsock has set sk_{snd|rcv}buf to
> smc.sysctl_{w|r}mem or ipv4_sysctl_tcp_{w|r}mem[1].
> 
> Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when
> switching between TCP and SMC")
> Link:
> https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
> FYI,
> The detailed motivation and testing can be found in the link above.
> ---
>  net/smc/af_smc.c | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 9389f0cfa374..a35281153067 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -459,29 +459,11 @@ static int smc_bind(struct socket *sock, struct
> sockaddr *uaddr,
>  static void smc_adjust_sock_bufsizes(struct sock *nsk, struct sock
> *osk,
>  				     unsigned long mask)
>  {
> -	struct net *nnet = sock_net(nsk);
> -
>  	nsk->sk_userlocks = osk->sk_userlocks;
> -	if (osk->sk_userlocks & SOCK_SNDBUF_LOCK) {
> +	if (osk->sk_userlocks & SOCK_SNDBUF_LOCK)
>  		nsk->sk_sndbuf = osk->sk_sndbuf;
> -	} else {
> -		if (mask == SK_FLAGS_SMC_TO_CLC)
> -			WRITE_ONCE(nsk->sk_sndbuf,
> -				   READ_ONCE(nnet-
> >ipv4.sysctl_tcp_wmem[1]));
> -		else
> -			WRITE_ONCE(nsk->sk_sndbuf,
> -				   2 * READ_ONCE(nnet-
> >smc.sysctl_wmem));
> -	}
> -	if (osk->sk_userlocks & SOCK_RCVBUF_LOCK) {
> +	if (osk->sk_userlocks & SOCK_RCVBUF_LOCK)
>  		nsk->sk_rcvbuf = osk->sk_rcvbuf;
> -	} else {
> -		if (mask == SK_FLAGS_SMC_TO_CLC)
> -			WRITE_ONCE(nsk->sk_rcvbuf,
> -				   READ_ONCE(nnet-
> >ipv4.sysctl_tcp_rmem[1]));
> -		else
> -			WRITE_ONCE(nsk->sk_rcvbuf,
> -				   2 * READ_ONCE(nnet-
> >smc.sysctl_rmem));
> -	}
>  }
>  
>  static void smc_copy_sock_settings(struct sock *nsk, struct sock
> *osk,

As Wenjia already said, we've discussed this a bit.
As I remember, I've added the sections to copy over the sysctl values 
as a "safety measure" when moving between smc/clc sockets - but had the
wrong assumption in mind that e.g. in a fall-back a new TCP handshake
would be done. Apparently, we didn't test the buffer size behavior in
these scenarios enough to notice the "weird" behavior.

So we reviewed your initial report of the oddity per your message in
the link above, too.

We fully agree that if no connection at the SMC level could be
established, you should expect the socket buffersizes be used that had
been established for the TCP connection - regardless if the fallback is
due to the server or the client.

So feel free to add my
Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>, too.

Thanks,
Gerd

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Wen Gu]

On 2024/6/5 00:16, Gerd Bayer wrote:
> Hi Wen Gu,
> 
> sorry for the late reply, I'm just catching up after a bit of a
> vacation.

No worries at all, I hope you had a great vacation!

> 
> On Fri, 2024-05-31 at 16:54 +0800, Wen Gu wrote:
>> When copying smc settings to clcsock, avoid setting clcsock's
>> sk_sndbuf to sysctl_tcp_wmem[1], since this may overwrite the value
>> set by tcp_sndbuf_expand() in TCP connection establishment.
>>
>> And the other setting sk_{snd|rcv}buf to sysctl value in
>> smc_adjust_sock_bufsizes() can also be omitted since the
>> initialization of smc sock and clcsock has set sk_{snd|rcv}buf to
>> smc.sysctl_{w|r}mem or ipv4_sysctl_tcp_{w|r}mem[1].
>>
>> Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when
>> switching between TCP and SMC")
>> Link:
>> https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
>> ---
>> FYI,
>> The detailed motivation and testing can be found in the link above.
>> ---

<...>

> 
> As Wenjia already said, we've discussed this a bit.
> As I remember, I've added the sections to copy over the sysctl values
> as a "safety measure" when moving between smc/clc sockets - but had the
> wrong assumption in mind that e.g. in a fall-back a new TCP handshake
> would be done. Apparently, we didn't test the buffer size behavior in
> these scenarios enough to notice the "weird" behavior.
> 
> So we reviewed your initial report of the oddity per your message in
> the link above, too.
> 
> We fully agree that if no connection at the SMC level could be
> established, you should expect the socket buffersizes be used that had
> been established for the TCP connection - regardless if the fallback is
> due to the server or the client.
> 
> So feel free to add my
> Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>, too.
> 

Hi, Gerd and Wenjia. Thanks a lot for your confirmation.

And as for the last question in the initial report (link above), that
why the server does not call smc_copy_sock_settings_to_clc() like the
client when fallback happens, I guess it is because at the time that
server fallback, the new_smc sock has not been accepted, so there will
be no user's sock settings that needs to be copied to clcsock.

Thanks!

> Thanks,
> Gerd

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 31 May 2024 16:54:17 +0800 you wrote:
> When copying smc settings to clcsock, avoid setting clcsock's sk_sndbuf
> to sysctl_tcp_wmem[1], since this may overwrite the value set by
> tcp_sndbuf_expand() in TCP connection establishment.
> 
> And the other setting sk_{snd|rcv}buf to sysctl value in
> smc_adjust_sock_bufsizes() can also be omitted since the initialization
> of smc sock and clcsock has set sk_{snd|rcv}buf to smc.sysctl_{w|r}mem
> or ipv4_sysctl_tcp_{w|r}mem[1].
> 
> [...]

Here is the summary with links:
  - [net] net/smc: avoid overwriting when adjusting sock bufsizes
    https://git.kernel.org/netdev/net/c/fb0aa0781a5f

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

Re: [PATCH net] net/smc: avoid overwriting when adjusting sock bufsizes

[Wen Gu]

On 2024/5/31 16:54, Wen Gu wrote:
> When copying smc settings to clcsock, avoid setting clcsock's sk_sndbuf
> to sysctl_tcp_wmem[1], since this may overwrite the value set by
> tcp_sndbuf_expand() in TCP connection establishment.
> 
> And the other setting sk_{snd|rcv}buf to sysctl value in
> smc_adjust_sock_bufsizes() can also be omitted since the initialization
> of smc sock and clcsock has set sk_{snd|rcv}buf to smc.sysctl_{w|r}mem
> or ipv4_sysctl_tcp_{w|r}mem[1].
> 
> Fixes: 30c3c4a4497c ("net/smc: Use correct buffer sizes when switching between TCP and SMC")
> Link: https://lore.kernel.org/r/5eaf3858-e7fd-4db8-83e8-3d7a3e0e9ae2@linux.alibaba.com
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
> FYI,
> The detailed motivation and testing can be found in the link above.
> ---

Hi, Gerd and Wenjia, do you think this makes sense? Thanks!