| Message ID | 20190417175119.27977-1-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net/tls: fix refcount adjustment in fallback | expand |
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? :-)
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
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(-)