Re: [PATCH mptcp-next] sock: add sock_krfree_s helper

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Wed, 2025-04-30 at 11:46 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 30/04/2025 11:09, Geliang Tang wrote:
> > This patch adds the rcu version of sock_kfree_s() helper, named
> > sock_krfree_s(), to free the memory using kfree_rcu_mightsleep()
> > and adjust the socket's option memory buffer.
> > 
> > This new helper is first used in MPTCP.
> > 
> > Signed-off-by: Geliang Tang <geliang@kernel.org>
> > ---
> >  include/net/sock.h       |  1 +
> >  net/core/sock.c          | 14 +++++++++++---
> >  net/mptcp/pm_userspace.c |  3 +--
> >  3 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index e223102337c7..94f72f5c8754 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1836,6 +1836,7 @@ void *sock_kmemdup(struct sock *sk, const
> > void *src,
> >  		   int size, gfp_t priority);
> >  void sock_kfree_s(struct sock *sk, void *mem, int size);
> >  void sock_kzfree_s(struct sock *sk, void *mem, int size);
> > +void sock_krfree_s(struct sock *sk, void *mem, int size);
> >  void sk_send_sigurg(struct sock *sk);
> >  
> >  static inline void sock_replace_proto(struct sock *sk, struct
> > proto *proto)
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index b64df2463300..f9ee0b44ab2a 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2862,12 +2862,14 @@ EXPORT_SYMBOL(sock_kmemdup);
> >   * condition entirely.
> >   */
> >  static inline void __sock_kfree_s(struct sock *sk, void *mem, int
> > size,
> > -				  const bool nullify)
> > +				  const bool nullify, const bool
> > rcu)
> >  {
> >  	if (WARN_ON_ONCE(!mem))
> >  		return;
> >  	if (nullify)
> >  		kfree_sensitive(mem);
> > +	else if (rcu)
> > +		kfree_rcu_mightsleep(mem);
> 
> I'm not sure about that: in some cases, kfree_rcu() might be called.
> kfree_rcu_mightsleep() is more an exception to me, I think. Also, it
> looks like there are many other places where the free and
> atomic_sub()
> is done "manually", that's not specific to MPTCP.
> 
> I don't know what Mat thinks about that, but I don't think this is
> needed (or it should replace some code in different places in the
> kernel).

Indeed, thanks for your reminder. I didn't notice that there are many
other places that use free() and atomic_sub() too. I will reconsider
whether to drop this patch or replace some code in different places.

Thanks,
-Geliang

> 
> Cheers,
> Matt