Re: [PATCH mptcp-next v1 5/6] sock: add sock_kmemdup helper

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

Thanks for the review.

On Mon, 2025-02-24 at 09:54 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 09:13, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds the sock version of kmemdup() helper, named
> > sock_kmemdup(),
> > to duplicate a memory block using the socket's option memory
> > buffer.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  include/net/sock.h |  1 +
> >  net/core/sock.c    | 23 +++++++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index edbb870e3f86..ffd757e7e329 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1793,6 +1793,7 @@ static inline struct sk_buff
> > *sock_alloc_send_skb(struct sock *sk,
> >  }
> >  
> >  void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
> > +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 sk_send_sigurg(struct sock *sk);
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 0d385bf27b38..d09bd697c120 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2805,6 +2805,29 @@ void *sock_kmalloc(struct sock *sk, int
> > size, gfp_t priority)
> >  }
> >  EXPORT_SYMBOL(sock_kmalloc);
> >  
> > +/*
> > + * Duplicate a memory block using the socket's option memory
> > buffer.
> > + */
> > +void *sock_kmemdup(struct sock *sk, const void *src, int size,
> > gfp_t priority)
> > +{
> > + int optmem_max = READ_ONCE(sock_net(sk)->core.sysctl_optmem_max);
> > +
> > + if ((unsigned int)size <= optmem_max &&
> > +     atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {
> > + void *mem;
> > + /* First do the add, to avoid the race if kmalloc
> > + * might sleep.
> > + */
> > + atomic_add(size, &sk->sk_omem_alloc);
> > + mem = kmemdup(src, size, priority);

I made some updates to this patch here.

      mem = src ? kmemdup(src, size, priority) :
                  kmalloc(size, priority);

> > + if (mem)
> > + return mem;
> > + atomic_sub(size, &sk->sk_omem_alloc);

Then sock_kmalloc() can be implemented through sock_kmemdup(), which
can reduce the duplication of code between them.

void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
{
      return sock_kmemdup(sk, NULL, size, priority);
}

> 
> I'm not really convinced by this helper: it is a duplication of
> sock_kmalloc(), and it is only used once in MPTCP code.

I found that this new helper can also be used in
mptcp_copy_ip_options() too.

> 
> Calling sock_kmalloc() + memset, and using this new helper in
> different
> places in the net code might help. But still, I don't know if this

And there are three other places in the net code where it can be used:

	ipv6_dup_options()
	sctp_v4_copy_ip_options()
	tcp_ao_copy_key()

That way, this helper can be used in five places.

> would
> be accepted, it is only saving one line (plus memcpy() will be used
> when
> it is not needed, same for the previous patch at the end).

What do you think of this new version?

> 
> If you still want to propose that, I suggest sending a dedicated
> series
> to netdev, not to block MPTCP (only) patches. WDYT?

Please remove the last two patches from this set if others are ready to
apply.

Thanks,
-Geliang

> 
> Cheers,
> Matt