JIRA: https://issues.redhat.com/browse/RHEL-29306
commit 0844370f8945086eb9335739d10205dcea8d707b
Author: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed Apr 24 12:25:47 2024 +0200
tls: fix lockless read of strp->msg_ready in ->poll
tls_sk_poll is called without locking the socket, and needs to read
strp->msg_ready (via tls_strp_msg_ready). Convert msg_ready to a bool
and use READ_ONCE/WRITE_ONCE where needed. The remaining reads are
only performed when the socket is locked.
Fixes: 121dca784fc0 ("tls: suppress wakeups unless we have a full record")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Link: https://lore.kernel.org/r/0b7ee062319037cf86af6b317b3d72f7bfcd2e97.1713797701.git.sd@queasysnail.net
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-7936
commit 11863c6d440d34c4b967e517739b38a7e68ed092
Author: Hannes Reinecke <hare@suse.de>
Date: Wed Jul 26 21:15:54 2023 +0200
net/tls: Use tcp_read_sock() instead of ops->read_sock()
TLS resets the protocol operations, so the read_sock() callback might
be changed, too.
In this case using sock->ops->readsock() in tls_strp_read_copyin() will
enter an infinite recursion if the read_sock() callback is calling
tls_rx_rec_wait() which will call into sock->ops->readsock() via
tls_strp_read_copyin().
But as tls_strp_read_copyin() is supposed to produce data from the
consumed socket and that socket is always a TCP socket we can call
tcp_read_sock() directly without having to deal with callbacks.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/20230726191556.41714-5-hare@suse.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Jeffrey Layton <jlayton@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit 8a0d57df8938e9fd2e99d47a85b7f37d86f91097
Author: Jakub Kicinski <kuba@kernel.org>
Date: Wed May 24 22:17:41 2023 -0700
tls: improve lockless access safety of tls_err_abort()
Most protos' poll() methods insert a memory barrier between
writes to sk_err and sk_error_report(). This dates back to
commit a4d258036e ("tcp: Fix race in tcp_poll").
I guess we should do the same thing in TLS, tcp_poll() does
not hold the socket lock.
Fixes: 3c4d755915 ("tls: kernel TLS support")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit eca9bfafee3a0487e59c59201ae14c7594ba940a
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue May 16 18:50:41 2023 -0700
tls: rx: strp: preserve decryption status of skbs when needed
When receive buffer is small we try to copy out the data from
TCP into a skb maintained by TLS to prevent connection from
stalling. Unfortunately if a single record is made up of a mix
of decrypted and non-decrypted skbs combining them into a single
skb leads to loss of decryption status, resulting in decryption
errors or data corruption.
Similarly when trying to use TCP receive queue directly we need
to make sure that all the skbs within the record have the same
status. If we don't the mixed status will be detected correctly
but we'll CoW the anchor, again collapsing it into a single paged
skb without decrypted status preserved. So the "fixup" code will
not know which parts of skb to re-encrypt.
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Shai Amiram <samiram@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit c1c607b1e5d5477d82ca6a86a05a4f10907b33ee
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue May 16 18:50:40 2023 -0700
tls: rx: strp: factor out copying skb data
We'll need to copy input skbs individually in the next patch.
Factor that code out (without assuming we're copying a full record).
Tested-by: Shai Amiram <samiram@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit 8b0c0dc9fbbd01e58a573a41c38885f9e4c17696
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue May 16 18:50:39 2023 -0700
tls: rx: strp: fix determining record length in copy mode
We call tls_rx_msg_size(skb) before doing skb->len += chunk.
So the tls_rx_msg_size() code will see old skb->len, most
likely leading to an over-read.
Worst case we will over read an entire record, next iteration
will try to trim the skb but may end up turning frag len negative
or discarding the subsequent record (since we already told TCP
we've read it during previous read but now we'll trim it out of
the skb).
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Shai Amiram <samiram@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit 14c4be92ebb3e36e392aa9dd8f314038a9f96f3c
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue May 16 18:50:38 2023 -0700
tls: rx: strp: force mixed decrypted records into copy mode
If a record is partially decrypted we'll have to CoW it, anyway,
so go into copy mode and allocate a writable skb right away.
This will make subsequent fix simpler because we won't have to
teach tls_strp_msg_make_copy() how to copy skbs while preserving
decrypt status.
Tested-by: Shai Amiram <samiram@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2219775
Tested: tls selftests
commit 210620ae44a83f25220450bbfcc22e6fe986b25f
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue May 16 18:50:37 2023 -0700
tls: rx: strp: set the skb->len of detached / CoW'ed skbs
alloc_skb_with_frags() fills in page frag sizes but does not
set skb->len and skb->data_len. Set those correctly otherwise
device offload will most likely generate an empty skb and
hit the BUG() at the end of __skb_nsg().
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Shai Amiram <samiram@nvidia.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit 0d87bbd39d7fd1135ab9eca672d760470f6508e8
Author: Jakub Kicinski <kuba@kernel.org>
Date: Wed Oct 12 15:55:20 2022 -0700
tls: strp: make sure the TCP skbs do not have overlapping data
TLS tries to get away with using the TCP input queue directly.
This does not work if there is duplicated data (multiple skbs
holding bytes for the same seq number range due to retransmits).
Check for this condition and fall back to copy mode, it should
be rare.
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit d800a7b3577bfb783481b02865d8775a760212a7
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue Aug 9 10:55:44 2022 -0700
tls: rx: device: don't try to copy too much on detach
Another device offload bug, we use the length of the output
skb as an indication of how much data to copy. But that skb
is sized to offset + record length, and we start from offset.
So we end up double-counting the offset which leads to
skb_copy_bits() returning -EFAULT.
Reported-by: Tariq Toukan <tariqt@nvidia.com>
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Tested-by: Ran Rozenstein <ranro@nvidia.com>
Link: https://lore.kernel.org/r/20220809175544.354343-2-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit d11ef9cc5a6792c8508cb00308b604836f9a9053
Author: Jakub Kicinski <kuba@kernel.org>
Date: Tue Jul 26 20:15:23 2022 -0700
tls: strp: rename and multithread the workqueue
Paolo points out that there seems to be no strong reason strparser
users a single threaded workqueue. Perhaps there were some performance
or pinning considerations? Since we don't know (and it's the slow path)
let's default to the most natural, multi-threaded choice.
Also rename the workqueue to "tls-".
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit 8fd1e151779285b211e7184e9237bba69bd74386
Author: Yang Li <yang.lee@linux.alibaba.com>
Date: Thu Jul 28 11:10:19 2022 +0800
tls: rx: Fix unsigned comparison with less than zero
The return from the call to tls_rx_msg_size() is int, it can be
a negative error code, however this is being assigned to an
unsigned long variable 'sz', so making 'sz' an int.
Eliminate the following coccicheck warning:
./net/tls/tls_strp.c:211:6-8: WARNING: Unsigned expression compared with zero: sz < 0
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Link: https://lore.kernel.org/r/20220728031019.32838-1-yang.lee@linux.alibaba.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit 84c61fe1a75b4255df1e1e7c054c9e6d048da417
Author: Jakub Kicinski <kuba@kernel.org>
Date: Fri Jul 22 16:50:33 2022 -0700
tls: rx: do not use the standard strparser
TLS is a relatively poor fit for strparser. We pause the input
every time a message is received, wait for a read which will
decrypt the message, start the parser, repeat. strparser is
built to delineate the messages, wrap them in individual skbs
and let them float off into the stack or a different socket.
TLS wants the data pages and nothing else. There's no need
for TLS to keep cloning (and occasionally skb_unclone()'ing)
the TCP rx queue.
This patch uses a pre-allocated skb and attaches the skbs
from the TCP rx queue to it as frags. TLS is careful never
to modify the input skb without CoW'ing / detaching it first.
Since we call TCP rx queue cleanup directly we also get back
the benefit of skb deferred free.
Overall this results in a 6% gain in my benchmarks.
Acked-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit 8b3c59a7a0bed6fe365755ac211dcf94fdac81b4
Author: Jakub Kicinski <kuba@kernel.org>
Date: Fri Jul 22 16:50:32 2022 -0700
tls: rx: device: add input CoW helper
Wrap the remaining skb_cow_data() into a helper, so it's easier
to replace down the lane. The new version will change the skb
so make sure relevant pointers get reloaded after the call.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit d4e5db6452211467f668521f5a3bd3c3928918e1
Author: Jakub Kicinski <kuba@kernel.org>
Date: Fri Jul 22 16:50:30 2022 -0700
tls: rx: device: keep the zero copy status with offload
The non-zero-copy path assumes a full skb with decrypted contents.
This means the device offload would have to CoW the data. Try
to keep the zero-copy status instead, copy the data to user space.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Tested: selftests
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2143700
commit c618db2afe7c31d13ca8cf05b60f17165fbdc282
Author: Jakub Kicinski <kuba@kernel.org>
Date: Thu Jul 14 22:22:33 2022 -0700
tls: rx: async: hold onto the input skb
Async crypto currently benefits from the fact that we decrypt
in place. When we allow input and output to be different skbs
we will have to hang onto the input while we move to the next
record. Clone the inputs and keep them on a list.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sabrina Dubroca <sdubroca@redhat.com>
Sabrina Dubroca
Jeffrey Layton