Reaching official SMC maintainers

Reaching official SMC maintainers

[Dmitry Antipov]

Jakub,

could you please check whether an official maintainers of net/smc are
actually active? I'm interesting just because there was no feedback on
[1]. After all, it's still a kernel memory leak, and IMO should not be
silently ignored by the maintainers (if any).

Thanks,
Dmitry

[1] https://lore.kernel.org/netdev/20240221051608.43241-1-dmantipov@yandex.ru/

Re: Reaching official SMC maintainers

[Wen Gu]

On 2024/3/4 18:31, Dmitry Antipov wrote:
> Jakub,
> 
> could you please check whether an official maintainers of net/smc are
> actually active? I'm interesting just because there was no feedback on
> [1]. After all, it's still a kernel memory leak, and IMO should not be
> silently ignored by the maintainers (if any).
> 
> Thanks,
> Dmitry
> 
> [1] https://lore.kernel.org/netdev/20240221051608.43241-1-dmantipov@yandex.ru/

Hi, Dmitry, I think I have given my feedback about this, see:

https://lore.kernel.org/netdev/819353f3-f5f9-4a15-96a1-4f3a7fd6b33e@linux.alibaba.com/
https://lore.kernel.org/netdev/19d7d71b-c911-45cc-9671-235d98720be6@linux.alibaba.com/

IMHO, if we want to address the problem of fasync_struct entries being
incorrectly inserted to old socket, we may have to change the general code.

Thanks.

Re: Reaching official SMC maintainers

[Dmitry Antipov]

On 3/4/24 13:51, Wen Gu wrote:

> IMHO, if we want to address the problem of fasync_struct entries being
> incorrectly inserted to old socket, we may have to change the general code.

BTW what about using shared wait queue? Just to illustrate an idea:

diff --git a/include/linux/net.h b/include/linux/net.h
index c9b4a63791a4..02df64747db7 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -126,6 +126,7 @@ struct socket {
  	const struct proto_ops	*ops; /* Might change with IPV6_ADDRFORM or MPTCP. */

  	struct socket_wq	wq;
+	struct socket_wq	*shared_wq;
  };

  /*
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 0f53a5c6fd9d..f04d61e316b2 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3360,6 +3360,9 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
  		smc->clcsock = clcsock;
  	}

+	sock->shared_wq = &smc->shared_wq;
+	smc->clcsock->shared_wq = &smc->shared_wq;
+
  out:
  	return rc;
  }
diff --git a/net/smc/smc.h b/net/smc/smc.h
index df64efd2dee8..26e66c289d4f 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -287,6 +287,7 @@ struct smc_sock {				/* smc sock container */
  						/* protects clcsock of a listen
  						 * socket
  						 * */
+	struct socket_wq	shared_wq;
  };

  #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
diff --git a/net/socket.c b/net/socket.c
index ed3df2f749bf..9b9e6932906f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1437,7 +1437,8 @@ static int sock_fasync(int fd, struct file *filp, int on)
  {
  	struct socket *sock = filp->private_data;
  	struct sock *sk = sock->sk;
-	struct socket_wq *wq = &sock->wq;
+	struct socket_wq *wq = (unlikely(sock->shared_wq) ?
+				sock->shared_wq : &sock->wq);

  	if (sk == NULL)
  		return -EINVAL;

Dmitry

Re: Reaching official SMC maintainers

[Jan Karcher]

On 05/03/2024 17:39, Dmitry Antipov wrote:
> On 3/4/24 13:51, Wen Gu wrote:
> 
>> IMHO, if we want to address the problem of fasync_struct entries being
>> incorrectly inserted to old socket, we may have to change the general 
>> code.
> 
> BTW what about using shared wait queue? Just to illustrate an idea:

I'm sorry but could we please clean up the e-mail threads?
This one here is a question if we are still alive: Yes, we are.

The other one i currently treat as an RFC and gracefully ignore the 
PATCH tag. If you want to post it as an patch please come up with a 
solution, clean it up and re-post it.
See patchwork errors for example: 
https://patchwork.kernel.org/project/netdevbpf/patch/20240221051608.43241-1-dmantipov@yandex.ru/

For the general RFC discussion we are going to comment on it as soon as 
we have something to say about it.
Feel free to re-post your idea regarding a shared wait queue there.

Thank you for your interest in smc and the ideas!
- Jan

> 
> diff --git a/include/linux/net.h b/include/linux/net.h
> index c9b4a63791a4..02df64747db7 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -126,6 +126,7 @@ struct socket {
>       const struct proto_ops    *ops; /* Might change with IPV6_ADDRFORM 
> or MPTCP. */
> 
>       struct socket_wq    wq;
> +    struct socket_wq    *shared_wq;
>   };
> 
>   /*
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 0f53a5c6fd9d..f04d61e316b2 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -3360,6 +3360,9 @@ static int __smc_create(struct net *net, struct 
> socket *sock, int protocol,
>           smc->clcsock = clcsock;
>       }
> 
> +    sock->shared_wq = &smc->shared_wq;
> +    smc->clcsock->shared_wq = &smc->shared_wq;
> +
>   out:
>       return rc;
>   }
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index df64efd2dee8..26e66c289d4f 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -287,6 +287,7 @@ struct smc_sock {                /* smc sock 
> container */
>                           /* protects clcsock of a listen
>                            * socket
>                            * */
> +    struct socket_wq    shared_wq;
>   };
> 
>   #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> diff --git a/net/socket.c b/net/socket.c
> index ed3df2f749bf..9b9e6932906f 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1437,7 +1437,8 @@ static int sock_fasync(int fd, struct file *filp, 
> int on)
>   {
>       struct socket *sock = filp->private_data;
>       struct sock *sk = sock->sk;
> -    struct socket_wq *wq = &sock->wq;
> +    struct socket_wq *wq = (unlikely(sock->shared_wq) ?
> +                sock->shared_wq : &sock->wq);
> 
>       if (sk == NULL)
>           return -EINVAL;
> 
> Dmitry
> 

Re: Reaching official SMC maintainers

[Wenjia Zhang]

On 04.03.24 11:31, Dmitry Antipov wrote:
> Jakub,
> 
> could you please check whether an official maintainers of net/smc are
> actually active? I'm interesting just because there was no feedback on
> [1]. After all, it's still a kernel memory leak, and IMO should not be
> silently ignored by the maintainers (if any).
> 
> Thanks,
> Dmitry
> 
> [1] 
> https://lore.kernel.org/netdev/20240221051608.43241-1-dmantipov@yandex.ru/
> 

Hi Dmitry,

I'm on the way to answering you. I understand your worry and appreciate 
your sugguestion on the improvement. Since I'm not the original author, 
either, I also need to undestand what was the original intention. i.e. 
Why should the fasync_list of the smc socket be handed over to the clc 
socket? Is there a way to deal with the list prior to the fallback?

AIU, the syzbot's reports on whichever the original fixed or your last 
patch fixed are about the same issue. And both of the fixes seem not to 
solve the problem.  Instead of patches on patches, I'd prefer to find 
the root problem and solve it.

Thus, to the proposed patches from you guys (and back to the question at 
the beginning), if the fasyn_list should be handed over, I like the Wen 
Gu's patch more. Otherwise, I'd like yours more, but as you already 
underlied, it should be done in some other way

Thanks,
Wenjia