* [Crypto v7 03/12] tls: support for inline tls
@ 2018-02-22 17:51 Atul Gupta
2018-02-23 16:23 ` Dave Watson
0 siblings, 1 reply; 5+ messages in thread
From: Atul Gupta @ 2018-02-22 17:51 UTC (permalink / raw)
To: davejwatson, davem, herbert; +Cc: sd, linux-crypto, netdev, ganeshgr
Facility to register Inline TLS drivers to net/tls. Setup
TLS_FULL_HW prot to listen on offload device.
Cases handled
1. Inline TLS device exists, setup prot for TLS_FULL_HW
2. Atleast one Inline TLS exists, sets TLS_FULL_HW. If
non-inline capable device establish connection, move to TLS_SW_TX
3. default mode TLS_SW_TX continues
Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>
---
net/tls/tls_main.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 116 insertions(+), 7 deletions(-)
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index b0d5fce..34f8781 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -38,6 +38,7 @@
#include <linux/highmem.h>
#include <linux/netdevice.h>
#include <linux/sched/signal.h>
+#include <linux/inetdevice.h>
#include <net/tls.h>
@@ -45,13 +46,9 @@
MODULE_DESCRIPTION("Transport Layer Security Support");
MODULE_LICENSE("Dual BSD/GPL");
-enum {
- TLS_BASE_TX,
- TLS_SW_TX,
- TLS_NUM_CONFIG,
-};
-
-static struct proto tls_prots[TLS_NUM_CONFIG];
+static LIST_HEAD(device_list);
+static DEFINE_MUTEX(device_mutex);
+struct proto tls_prots[TLS_NUM_CONFIG];
static inline void update_sk_prot(struct sock *sk, struct tls_context *ctx)
{
@@ -260,6 +257,37 @@ static void tls_sk_proto_close(struct sock *sk, long timeout)
sk_proto_close(sk, timeout);
}
+static struct net_device *get_netdev(struct sock *sk)
+{
+ struct inet_sock *inet = inet_sk(sk);
+ struct net_device *netdev;
+
+ netdev = dev_get_by_index(sock_net(sk), inet->cork.fl.flowi_oif);
+ return netdev;
+}
+
+static int tls_offload_dev_absent(struct sock *sk)
+{
+ struct net_device *netdev;
+ struct tls_device *dev;
+ int rc = 0;
+
+ netdev = get_netdev(sk);
+ if (!netdev)
+ return -EINVAL;
+
+ mutex_lock(&device_mutex);
+ list_for_each_entry(dev, &device_list, dev_list) {
+ if (dev->netdev && dev->netdev(dev, netdev)) {
+ rc = -EEXIST;
+ break;
+ }
+ }
+ mutex_unlock(&device_mutex);
+ dev_put(netdev);
+ return rc;
+}
+
static int do_tls_getsockopt_tx(struct sock *sk, char __user *optval,
int __user *optlen)
{
@@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
goto err_crypto_info;
}
+ rc = tls_offload_dev_absent(sk);
+ if (rc == -EINVAL) {
+ goto out;
+ } else if (rc == -EEXIST) {
+ /* Retain HW unhash for cleanup and move to SW Tx */
+ sk->sk_prot[TLS_BASE_TX].unhash =
+ sk->sk_prot[TLS_FULL_HW].unhash;
+ }
+
/* currently SW is default, we will have ethtool in future */
rc = tls_set_sw_offload(sk, ctx);
tx_conf = TLS_SW_TX;
@@ -450,6 +487,54 @@ static int tls_setsockopt(struct sock *sk, int level, int optname,
return do_tls_setsockopt(sk, optname, optval, optlen);
}
+static int tls_hw_prot(struct sock *sk)
+{
+ struct tls_context *ctx = tls_get_ctx(sk);
+ struct tls_device *dev;
+
+ mutex_lock(&device_mutex);
+ list_for_each_entry(dev, &device_list, dev_list) {
+ if (dev->feature && dev->feature(dev)) {
+ ctx->tx_conf = TLS_FULL_HW;
+ update_sk_prot(sk, ctx);
+ break;
+ }
+ }
+ mutex_unlock(&device_mutex);
+ return ctx->tx_conf;
+}
+
+static void tls_hw_unhash(struct sock *sk)
+{
+ struct tls_device *dev;
+
+ mutex_lock(&device_mutex);
+ list_for_each_entry(dev, &device_list, dev_list) {
+ if (dev->unhash)
+ dev->unhash(dev, sk);
+ }
+ mutex_unlock(&device_mutex);
+ sk->sk_prot->unhash(sk);
+}
+
+static int tls_hw_hash(struct sock *sk)
+{
+ struct tls_device *dev;
+ int err;
+
+ err = sk->sk_prot->hash(sk);
+ mutex_lock(&device_mutex);
+ list_for_each_entry(dev, &device_list, dev_list) {
+ if (dev->hash)
+ err |= dev->hash(dev, sk);
+ }
+ mutex_unlock(&device_mutex);
+
+ if (err)
+ tls_hw_unhash(sk);
+ return err;
+}
+
static int tls_init(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
@@ -477,6 +562,9 @@ static int tls_init(struct sock *sk)
ctx->sk_proto_close = sk->sk_prot->close;
ctx->tx_conf = TLS_BASE_TX;
+ if (tls_hw_prot(sk) == TLS_FULL_HW)
+ goto out;
+
update_sk_prot(sk, ctx);
out:
return rc;
@@ -500,7 +588,27 @@ static void build_protos(struct proto *prot, struct proto *base)
prot[TLS_SW_TX] = prot[TLS_BASE_TX];
prot[TLS_SW_TX].sendmsg = tls_sw_sendmsg;
prot[TLS_SW_TX].sendpage = tls_sw_sendpage;
+
+ prot[TLS_FULL_HW] = prot[TLS_BASE_TX];
+ prot[TLS_FULL_HW].hash = tls_hw_hash;
+ prot[TLS_FULL_HW].unhash = tls_hw_unhash;
+}
+
+void tls_register_device(struct tls_device *device)
+{
+ mutex_lock(&device_mutex);
+ list_add_tail(&device->dev_list, &device_list);
+ mutex_unlock(&device_mutex);
+}
+EXPORT_SYMBOL(tls_register_device);
+
+void tls_unregister_device(struct tls_device *device)
+{
+ mutex_lock(&device_mutex);
+ list_del(&device->dev_list);
+ mutex_unlock(&device_mutex);
}
+EXPORT_SYMBOL(tls_unregister_device);
static int __init tls_register(void)
{
@@ -510,6 +618,7 @@ static int __init tls_register(void)
return 0;
}
+EXPORT_SYMBOL(tls_prots);
static void __exit tls_unregister(void)
{
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Crypto v7 03/12] tls: support for inline tls
2018-02-22 17:51 [Crypto v7 03/12] tls: support for inline tls Atul Gupta
@ 2018-02-23 16:23 ` Dave Watson
2018-02-23 16:58 ` Atul Gupta
0 siblings, 1 reply; 5+ messages in thread
From: Dave Watson @ 2018-02-23 16:23 UTC (permalink / raw)
To: Atul Gupta; +Cc: davem, herbert, sd, linux-crypto, netdev, ganeshgr
On 02/22/18 11:21 PM, Atul Gupta wrote:
> @@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
> goto err_crypto_info;
> }
>
> + rc = tls_offload_dev_absent(sk);
> + if (rc == -EINVAL) {
> + goto out;
> + } else if (rc == -EEXIST) {
> + /* Retain HW unhash for cleanup and move to SW Tx */
> + sk->sk_prot[TLS_BASE_TX].unhash =
> + sk->sk_prot[TLS_FULL_HW].unhash;
I'm still confused by this, it lookes like it is modifying the global
tls_prots without taking a lock? And modifying it for all sockets,
not just this one? One way to fix might be to always set an unhash in
TLS_BASE_TX, and then have a function pointer unhash in ctx.
> +static void tls_hw_unhash(struct sock *sk)
> +{
> + struct tls_device *dev;
> +
> + mutex_lock(&device_mutex);
> + list_for_each_entry(dev, &device_list, dev_list) {
> + if (dev->unhash)
> + dev->unhash(dev, sk);
> + }
> + mutex_unlock(&device_mutex);
> + sk->sk_prot->unhash(sk);
I would have thought unhash() here was tls_hw_unhash, doesn't the
original callback need to be saved like the other ones
(set/getsockopt, etc) in tls_init? Similar for hash().
It looks like in patch 11 you directly call tcp_prot.hash/unhash, so
it doesn't have this issue.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [Crypto v7 03/12] tls: support for inline tls
2018-02-23 16:23 ` Dave Watson
@ 2018-02-23 16:58 ` Atul Gupta
2018-02-23 17:32 ` Dave Watson
0 siblings, 1 reply; 5+ messages in thread
From: Atul Gupta @ 2018-02-23 16:58 UTC (permalink / raw)
To: Dave Watson; +Cc: davem, herbert, sd, linux-crypto, netdev, Ganesh GR
-----Original Message-----
From: Dave Watson [mailto:davejwatson@fb.com]
Sent: Friday, February 23, 2018 9:53 PM
To: Atul Gupta <atul.gupta@chelsio.com>
Cc: davem@davemloft.net; herbert@gondor.apana.org.au; sd@queasysnail.net; linux-crypto@vger.kernel.org; netdev@vger.kernel.org; Ganesh GR <ganeshgr@chelsio.com>
Subject: Re: [Crypto v7 03/12] tls: support for inline tls
On 02/22/18 11:21 PM, Atul Gupta wrote:
> @@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
> goto err_crypto_info;
> }
>
> + rc = tls_offload_dev_absent(sk);
> + if (rc == -EINVAL) {
> + goto out;
> + } else if (rc == -EEXIST) {
> + /* Retain HW unhash for cleanup and move to SW Tx */
> + sk->sk_prot[TLS_BASE_TX].unhash =
> + sk->sk_prot[TLS_FULL_HW].unhash;
I'm still confused by this, it lookes like it is modifying the global tls_prots without taking a lock? And modifying it for all sockets, not just this one? One way to fix might be to always set an unhash in TLS_BASE_TX, and then have a function pointer unhash in ctx.
code enters do_tls_setsockopt_tx only for those offload capable dev which does not define FULL_HW setsockopt as done by chtls, unhash prot update is required for cleanup/revert of setup done in tls_hw_hash. This update does not impact SW or other Inline HW path.
> +static void tls_hw_unhash(struct sock *sk) {
> + struct tls_device *dev;
> +
> + mutex_lock(&device_mutex);
> + list_for_each_entry(dev, &device_list, dev_list) {
> + if (dev->unhash)
> + dev->unhash(dev, sk);
> + }
> + mutex_unlock(&device_mutex);
> + sk->sk_prot->unhash(sk);
I would have thought unhash() here was tls_hw_unhash, doesn't the original callback need to be saved like the other ones (set/getsockopt, etc) in tls_init? Similar for hash().
Yes, good to store it or have it the way I had in v6 [tcp_prot.hash], can this correction go in patch than submit the whole series?
It looks like in patch 11 you directly call tcp_prot.hash/unhash, so it doesn't have this issue.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Crypto v7 03/12] tls: support for inline tls
2018-02-23 16:58 ` Atul Gupta
@ 2018-02-23 17:32 ` Dave Watson
2018-02-24 11:34 ` Atul Gupta
0 siblings, 1 reply; 5+ messages in thread
From: Dave Watson @ 2018-02-23 17:32 UTC (permalink / raw)
To: Atul Gupta; +Cc: davem, herbert, sd, linux-crypto, netdev, Ganesh GR
On 02/23/18 04:58 PM, Atul Gupta wrote:
> > On 02/22/18 11:21 PM, Atul Gupta wrote:
> > > @@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
> > > goto err_crypto_info;
> > > }
> > >
> > > + rc = tls_offload_dev_absent(sk);
> > > + if (rc == -EINVAL) {
> > > + goto out;
> > > + } else if (rc == -EEXIST) {
> > > + /* Retain HW unhash for cleanup and move to SW Tx */
> > > + sk->sk_prot[TLS_BASE_TX].unhash =
> > > + sk->sk_prot[TLS_FULL_HW].unhash;
> >
> > I'm still confused by this, it lookes like it is modifying the global tls_prots without taking a lock? And modifying it for all sockets, not just this one? One way to fix might be to always set an unhash in TLS_BASE_TX, and then have a function pointer unhash in ctx.
>
> code enters do_tls_setsockopt_tx only for those offload capable dev which does not define FULL_HW setsockopt as done by chtls, unhash prot update is required for cleanup/revert of setup done in tls_hw_hash. This update does not impact SW or other Inline HW path.
I still don't follow. If it doesn't impact SW, then what is it doing?
According to the comment, we're moving to SW tx, where sk_prot will be
&tls_prot[TLS_SW_TX], and the unhash function you set here in
TLS_BASE_TX won't be called.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [Crypto v7 03/12] tls: support for inline tls
2018-02-23 17:32 ` Dave Watson
@ 2018-02-24 11:34 ` Atul Gupta
0 siblings, 0 replies; 5+ messages in thread
From: Atul Gupta @ 2018-02-24 11:34 UTC (permalink / raw)
To: Dave Watson; +Cc: davem, herbert, sd, linux-crypto, netdev, Ganesh GR
-----Original Message-----
From: Dave Watson [mailto:davejwatson@fb.com]
Sent: Friday, February 23, 2018 11:03 PM
To: Atul Gupta <atul.gupta@chelsio.com>
Cc: davem@davemloft.net; herbert@gondor.apana.org.au; sd@queasysnail.net; linux-crypto@vger.kernel.org; netdev@vger.kernel.org; Ganesh GR <ganeshgr@chelsio.com>
Subject: Re: [Crypto v7 03/12] tls: support for inline tls
On 02/23/18 04:58 PM, Atul Gupta wrote:
> > On 02/22/18 11:21 PM, Atul Gupta wrote:
> > > @@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
> > > goto err_crypto_info;
> > > }
> > >
> > > + rc = tls_offload_dev_absent(sk);
> > > + if (rc == -EINVAL) {
> > > + goto out;
> > > + } else if (rc == -EEXIST) {
> > > + /* Retain HW unhash for cleanup and move to SW Tx */
> > > + sk->sk_prot[TLS_BASE_TX].unhash =
> > > + sk->sk_prot[TLS_FULL_HW].unhash;
> >
> > I'm still confused by this, it lookes like it is modifying the global tls_prots without taking a lock? And modifying it for all sockets, not just this one? One way to fix might be to always set an unhash in TLS_BASE_TX, and then have a function pointer unhash in ctx.
>
> code enters do_tls_setsockopt_tx only for those offload capable dev which does not define FULL_HW setsockopt as done by chtls, unhash prot update is required for cleanup/revert of setup done in tls_hw_hash. This update does not impact SW or other Inline HW path.
I still don't follow. If it doesn't impact SW, then what is it doing?
According to the comment, we're moving to SW tx, where sk_prot will be &tls_prot[TLS_SW_TX], and the unhash function you set here in TLS_BASE_TX won't be called.
some of the scenarios I originally thought:
- tls_init finds the Inline offload dev and sets the TLS_FULL_HW but setsockopt remains do_tls_setsockopt_tx, In the above path we continue in TLS_SW_TX mode with updated unhash. Since, sw_tx prot is borrowed from base_tx we modified the base_tx prot unhash for cleanup.
- tls_offload_dev_absent finds no device i.e rc=0. Continue in TLS_SW_TX mode. No change required.
- Inline tls device is added after tls_init is called, do_tls_setsockopt_tx will see tls_offload_dev_absent return EEXIST and will modify unhash but only if tx_conf = TLS_FULL_HW [missing now] and you rightly pointed that it ends up modifying base prot for all sk which is not we want. My worry was losing hw specific unhash.
I see calling tls_offload_dev_absent in do_tls_setsockopt_tx an overkill, the sk here perhaps require no update to continue in SW_TX, the HW unhash is still assigned to tls_init 'sk' for cleanup in the close path, better to remove tls_offload_dev_absent altogether and simplify.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-24 11:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22 17:51 [Crypto v7 03/12] tls: support for inline tls Atul Gupta
2018-02-23 16:23 ` Dave Watson
2018-02-23 16:58 ` Atul Gupta
2018-02-23 17:32 ` Dave Watson
2018-02-24 11:34 ` Atul Gupta
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.