| Message ID | 1519321890-22919-1-git-send-email-atul.gupta@chelsio.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
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.
-----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.
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.
-----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.
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) {
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(-)