| Message ID | cover.1629840814.git.cdleonard@gmail.com |
|---|---|
| Headers | show |
| Series | tcp: Initial support for RFC5925 auth option | expand |
On 8/24/21 2:34 PM, Leonard Crestez wrote: > The crypto_shash API is used in order to compute packet signatures. The > API comes with several unfortunate limitations: > > 1) Allocating a crypto_shash can sleep and must be done in user context. > 2) Packet signatures must be computed in softirq context > 3) Packet signatures use dynamic "traffic keys" which require exclusive > access to crypto_shash for crypto_setkey. > > The solution is to allocate one crypto_shash for each possible cpu for > each algorithm at setsockopt time. The per-cpu tfm is then borrowed from > softirq context, signatures are computed and the tfm is returned. > > The pool for each algorithm is reference counted, initialized at > setsockopt time and released in tcp_authopt_key_info's rcu callback > > I don't know, why should we really care and try so hard to release the tfm per cpu ? I would simply allocate them at boot time. This would avoid the expensive refcounting (potential false sharing)
On Tue, Aug 24, 2021 at 04:34:58PM -0700, Eric Dumazet wrote: > > On 8/24/21 2:34 PM, Leonard Crestez wrote: > > The crypto_shash API is used in order to compute packet signatures. The > > API comes with several unfortunate limitations: > > > > 1) Allocating a crypto_shash can sleep and must be done in user context. > > 2) Packet signatures must be computed in softirq context > > 3) Packet signatures use dynamic "traffic keys" which require exclusive > > access to crypto_shash for crypto_setkey. > > > > The solution is to allocate one crypto_shash for each possible cpu for > > each algorithm at setsockopt time. The per-cpu tfm is then borrowed from > > softirq context, signatures are computed and the tfm is returned. > > > > I could not see the per-cpu stuff that you mention in the changelog. Perhaps it's time we moved the key information from the tfm into the request structure for hashes? Or at least provide a way for the key to be in the request structure in addition to the tfm as the tfm model still works for IPsec. Ard/Eric, what do you think about that? Thanks,
On Wed, 25 Aug 2021 at 10:08, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, Aug 24, 2021 at 04:34:58PM -0700, Eric Dumazet wrote: > > > > On 8/24/21 2:34 PM, Leonard Crestez wrote: > > > The crypto_shash API is used in order to compute packet signatures. The > > > API comes with several unfortunate limitations: > > > > > > 1) Allocating a crypto_shash can sleep and must be done in user context. > > > 2) Packet signatures must be computed in softirq context > > > 3) Packet signatures use dynamic "traffic keys" which require exclusive > > > access to crypto_shash for crypto_setkey. > > > > > > The solution is to allocate one crypto_shash for each possible cpu for > > > each algorithm at setsockopt time. The per-cpu tfm is then borrowed from > > > softirq context, signatures are computed and the tfm is returned. > > > > > > > I could not see the per-cpu stuff that you mention in the changelog. > > Perhaps it's time we moved the key information from the tfm into > the request structure for hashes? Or at least provide a way for > the key to be in the request structure in addition to the tfm as > the tfm model still works for IPsec. Ard/Eric, what do you think > about that? > I think it makes sense for a shash desc to have the ability to carry a key, which will be used instead of the TFM key, but this seems like quite a lot of work, given that all implementations will need to be updated. Also, setkey() can currently sleep, so we need to check whether the existing key manipulation code can actually execute during init/update/final if sleeping is not permitted.
On 25.08.2021 08:18, David Ahern wrote: > On 8/24/21 2:34 PM, Leonard Crestez wrote: >> By default TCP-AO keys apply to all possible peers but it's possible to >> have different keys for different remote hosts. >> >> This patch adds initial tests for the behavior behind the >> TCP_AUTHOPT_KEY_BIND_ADDR flag. Server rejection is tested via client >> timeout so this can be slightly slow. >> >> Signed-off-by: Leonard Crestez <cdleonard@gmail.com> >> --- >> .../tcp_authopt_test/netns_fixture.py | 63 +++++++ >> .../tcp_authopt/tcp_authopt_test/server.py | 82 ++++++++++ >> .../tcp_authopt/tcp_authopt_test/test_bind.py | 143 ++++++++++++++++ >> .../tcp_authopt/tcp_authopt_test/utils.py | 154 ++++++++++++++++++ >> 4 files changed, 442 insertions(+) >> create mode 100644 tools/testing/selftests/tcp_authopt/tcp_authopt_test/netns_fixture.py >> create mode 100644 tools/testing/selftests/tcp_authopt/tcp_authopt_test/server.py >> create mode 100644 tools/testing/selftests/tcp_authopt/tcp_authopt_test/test_bind.py >> create mode 100644 tools/testing/selftests/tcp_authopt/tcp_authopt_test/utils.py >> > > This should be under selftests/net as a single "tcp_authopt" directory > from what I can tell. Maybe? I found no clear guidelines for organizing tests by subsystem. I just did a grep for .py in selftests and placed mine next to tc-testing. Having a tcp_authopt_test code directory under tcp_authopt is the standard pattern for python packages, otherwise all submodules with utilities of dubious generality are dumped at the global level. Removing the tcp_authopt/tcp_authopt_test structure is awkward in python. One way to deal with this is to add my test code in tools/testing/selftests/net/tcp_authopt and my setup.cfg and similar directly in tools/testing/selftests/net. This would make "net" the root of the package and make it easy to add other networking pytests. This seems close to what you mean. kselftest itself does not seem to offer any special support for python code, only some for C and shell. Maybe it could offer a "kselftest" package with common utilities that are used by multiple test packages and everything would be installed into a single virtualenv by makefiles. -- Regards, Leonard
On 8/25/21 8:55 PM, Eric Dumazet wrote: > On Wed, Aug 25, 2021 at 9:35 AM Leonard Crestez <cdleonard@gmail.com> wrote: >> >> On 25.08.2021 02:34, Eric Dumazet wrote: >>> On 8/24/21 2:34 PM, Leonard Crestez wrote: >>>> The crypto_shash API is used in order to compute packet signatures. The >>>> API comes with several unfortunate limitations: >>>> >>>> 1) Allocating a crypto_shash can sleep and must be done in user context. >>>> 2) Packet signatures must be computed in softirq context >>>> 3) Packet signatures use dynamic "traffic keys" which require exclusive >>>> access to crypto_shash for crypto_setkey. >>>> >>>> The solution is to allocate one crypto_shash for each possible cpu for >>>> each algorithm at setsockopt time. The per-cpu tfm is then borrowed from >>>> softirq context, signatures are computed and the tfm is returned. >>>> >>> >>> I could not see the per-cpu stuff that you mention in the changelog. >> >> That's a little embarrasing, I forgot to implement the actual per-cpu >> stuff. tcp_authopt_alg_imp.tfm is meant to be an array up to NR_CPUS and >> tcp_authopt_alg_get_tfm needs no locking other than preempt_disable >> (which should already be the case). > > Well, do not use arrays of NR_CPUS and instead use normal per_cpu > accessors (as in __tcp_alloc_md5sig_pool) > >> >> The reference counting would still only happen from very few places: >> setsockopt, close and openreq. This would only impact request/response >> traffic and relatively little. > > What I meant is that __tcp_alloc_md5sig_pool() allocates stuff one time, > we do not care about tcp_md5sig_pool_populated going back to false. > > Otherwise, a single user application constantly allocating a socket, > enabling MD5 (or authopt), then closing the socket would incur > a big cost on hosts with a lot of cpus. Allocating only once would definitely simply things. I don't know if this might end up tying hardware resources forever if some accelerators are in play but for this feature software-only crypto is perfectly fine. -- Regards, Leonard