| Message ID | 532014d8a99966da5fbcee528abe56356899f04a.1617121851.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9adc89af724f12a03b47099cd943ed54e877cd59 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: let skb_orphan_partial wake-up waiters. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: daniel@iogearbox.net linmiaohe@huawei.com xiangxia.m.yue@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4254 this patch: 4254 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4491 this patch: 4491 |
| netdev/header_inline | success | Link |
On 3/30/21 6:43 PM, Paolo Abeni wrote: > Currently the mentioned helper can end-up freeing the socket wmem > without waking-up any processes waiting for more write memory. > > If the partially orphaned skb is attached to an UDP (or raw) socket, > the lack of wake-up can hang the user-space. > > Even for TCP sockets not calling the sk destructor could have bad > effects on TSQ. > > Address the issue using skb_orphan to release the sk wmem before > setting the new sock_efree destructor. Additionally bundle the > whole ownership update in a new helper, so that later other > potential users could avoid duplicate code. > > v1 -> v2: > - use skb_orphan() instead of sort of open coding it (Eric) > - provide an helper for the ownership change (Eric) > > Fixes: f6ba8d33cfbb ("netem: fix skb_orphan_partial()") > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks !
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 30 Mar 2021 18:43:54 +0200 you wrote: > Currently the mentioned helper can end-up freeing the socket wmem > without waking-up any processes waiting for more write memory. > > If the partially orphaned skb is attached to an UDP (or raw) socket, > the lack of wake-up can hang the user-space. > > Even for TCP sockets not calling the sk destructor could have bad > effects on TSQ. > > [...] Here is the summary with links: - [net,v2] net: let skb_orphan_partial wake-up waiters. https://git.kernel.org/netdev/net/c/9adc89af724f You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/include/net/sock.h b/include/net/sock.h index 636810ddcd9ba..4a1c3f53630b3 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2221,6 +2221,15 @@ static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk) sk_mem_charge(sk, skb->truesize); } +static inline void skb_set_owner_sk_safe(struct sk_buff *skb, struct sock *sk) +{ + if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) { + skb_orphan(skb); + skb->destructor = sock_efree; + skb->sk = sk; + } +} + void sk_reset_timer(struct sock *sk, struct timer_list *timer, unsigned long expires); diff --git a/net/core/sock.c b/net/core/sock.c index 0ed98f20448a2..7092e3b3076ee 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -2132,16 +2132,10 @@ void skb_orphan_partial(struct sk_buff *skb) if (skb_is_tcp_pure_ack(skb)) return; - if (can_skb_orphan_partial(skb)) { - struct sock *sk = skb->sk; - - if (refcount_inc_not_zero(&sk->sk_refcnt)) { - WARN_ON(refcount_sub_and_test(skb->truesize, &sk->sk_wmem_alloc)); - skb->destructor = sock_efree; - } - } else { + if (can_skb_orphan_partial(skb)) + skb_set_owner_sk_safe(skb, skb->sk); + else skb_orphan(skb); - } } EXPORT_SYMBOL(skb_orphan_partial);
Currently the mentioned helper can end-up freeing the socket wmem without waking-up any processes waiting for more write memory. If the partially orphaned skb is attached to an UDP (or raw) socket, the lack of wake-up can hang the user-space. Even for TCP sockets not calling the sk destructor could have bad effects on TSQ. Address the issue using skb_orphan to release the sk wmem before setting the new sock_efree destructor. Additionally bundle the whole ownership update in a new helper, so that later other potential users could avoid duplicate code. v1 -> v2: - use skb_orphan() instead of sort of open coding it (Eric) - provide an helper for the ownership change (Eric) Fixes: f6ba8d33cfbb ("netem: fix skb_orphan_partial()") Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- include/net/sock.h | 9 +++++++++ net/core/sock.c | 12 +++--------- 2 files changed, 12 insertions(+), 9 deletions(-)