* [PATCH net] net/tls: fix refcount adjustment in fallback
@ 2019-04-17 17:51 Jakub Kicinski
2019-04-18 23:54 ` David Miller
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Kicinski @ 2019-04-17 17:51 UTC (permalink / raw)
To: davem
Cc: edumazet, netdev, oss-drivers, alexei.starovoitov,
Jakub Kicinski, John Hurley
Unlike atomic_add(), refcount_add() does not deal well
with a negative argument. TLS fallback code reallocates
the skb and is very likely to shrink the truesize, leading to:
[ 189.513254] WARNING: CPU: 5 PID: 0 at lib/refcount.c:81 refcount_add_not_zero_checked+0x15c/0x180
Call Trace:
refcount_add_checked+0x6/0x40
tls_enc_skb+0xb93/0x13e0 [tls]
Once wmem_allocated count saturates the application can no longer
send data on the socket. This is similar to Eric's fixes for GSO,
TCP:
commit 7ec318feeed1 ("tcp: gso: avoid refcount_t warning from tcp_gso_segment()")
and UDP:
commit 575b65bc5bff ("udp: avoid refcount_t saturation in __udp_gso_segment()").
Unlike the GSO case, for TLS fallback it's likely that the skb has
shrunk, so the "likely" annotation is the other way around (likely
branch being "sub").
Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
Should we add a helper for this in -next? Because with
CONFIG_REFCOUNT_FULL=n we don't need the branch, we can just
do an atomic_add() directly..
I think the fact that different branch is likely in TLS is not
that important as in TLS fallback case we do the adjustment after
re-encrypting the SKB which is going to be orders of magnitude
more expensive.
---
net/tls/tls_device_fallback.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 87dfb6b0fe14..a4f41b66fae4 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -194,6 +194,9 @@ static void update_chksum(struct sk_buff *skb, int headln)
static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
{
+ struct sock *sk = skb->sk;
+ int delta;
+
skb_copy_header(nskb, skb);
skb_put(nskb, skb->len);
@@ -201,11 +204,15 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
update_chksum(nskb, headln);
nskb->destructor = skb->destructor;
- nskb->sk = skb->sk;
+ nskb->sk = sk;
skb->destructor = NULL;
skb->sk = NULL;
- refcount_add(nskb->truesize - skb->truesize,
- &nskb->sk->sk_wmem_alloc);
+
+ delta = nskb->truesize - skb->truesize;
+ if (likely(delta < 0))
+ WARN_ON_ONCE(refcount_sub_and_test(-delta, &sk->sk_wmem_alloc));
+ else if (delta)
+ refcount_add(delta, &sk->sk_wmem_alloc);
}
/* This function may be called after the user socket is already
--
2.21.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH net] net/tls: fix refcount adjustment in fallback
2019-04-17 17:51 [PATCH net] net/tls: fix refcount adjustment in fallback Jakub Kicinski
@ 2019-04-18 23:54 ` David Miller
0 siblings, 0 replies; 2+ messages in thread
From: David Miller @ 2019-04-18 23:54 UTC (permalink / raw)
To: jakub.kicinski
Cc: edumazet, netdev, oss-drivers, alexei.starovoitov, john.hurley
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Wed, 17 Apr 2019 10:51:19 -0700
> Unlike atomic_add(), refcount_add() does not deal well
> with a negative argument. TLS fallback code reallocates
> the skb and is very likely to shrink the truesize, leading to:
>
> [ 189.513254] WARNING: CPU: 5 PID: 0 at lib/refcount.c:81 refcount_add_not_zero_checked+0x15c/0x180
> Call Trace:
> refcount_add_checked+0x6/0x40
> tls_enc_skb+0xb93/0x13e0 [tls]
>
> Once wmem_allocated count saturates the application can no longer
> send data on the socket. This is similar to Eric's fixes for GSO,
> TCP:
> commit 7ec318feeed1 ("tcp: gso: avoid refcount_t warning from tcp_gso_segment()")
> and UDP:
> commit 575b65bc5bff ("udp: avoid refcount_t saturation in __udp_gso_segment()").
>
> Unlike the GSO case, for TLS fallback it's likely that the skb has
> shrunk, so the "likely" annotation is the other way around (likely
> branch being "sub").
>
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: John Hurley <john.hurley@netronome.com>
Applied and queued up for -stable, thanks Jakub.
> Should we add a helper for this in -next? Because with
> CONFIG_REFCOUNT_FULL=n we don't need the branch, we can just
> do an atomic_add() directly..
I think the value of that isn't so high. Are we counting branch miss
penalities in code pathes where we are copying SKB data or doing SW
crypto? :-)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-04-18 23:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-17 17:51 [PATCH net] net/tls: fix refcount adjustment in fallback Jakub Kicinski
2019-04-18 23:54 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.