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);
+ if (mem)
+ return mem;
+ atomic_sub(size, &sk->sk_omem_alloc);
+ }
+ return NULL;
+}
+EXPORT_SYMBOL(sock_kmemdup);
+
/* Free an option memory block. Note, we actually want the inline
* here as this allows gcc to detect the nullify and fold away the
* condition entirely.
--
2.43.0Hi 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);
> + if (mem)
> + return mem;
> + atomic_sub(size, &sk->sk_omem_alloc);
I'm not really convinced by this helper: it is a duplication of
sock_kmalloc(), and it is only used once in MPTCP code.
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 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).
If you still want to propose that, I suggest sending a dedicated series
to netdev, not to block MPTCP (only) patches. WDYT?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
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
Hi Geliang,
On 24/02/2025 11:42, Geliang Tang wrote:
> 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);
> }
Yes, looks good.
I was thinking about not modifying sock_kmalloc() by calling it from
sock_kmemdup(), then calling memcpy(). So similar to what kmemdup() is
doing. But your version looks good.
>> 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.
Indeed.
>> 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.
Indeed, that's what I saw when I quickly looked.
>> 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?
Yes, it might be OK, but I don't know if that kind of cleanup would be
accepted by netdev maintainers. Then, do you mind sending a dedicated
patch (or series) introducing this new helper and using it in the
different places to the netdev ML with the appropriated reviewers added
in cc, please?
>> 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.
Yes, I can do that.
I will not apply patch 4/6 as well, because the modification is similar.
I will first wait to see what kind of feedback the netdev maintainers
will give.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2025 Red Hat, Inc.