RE: [Crypto v7 03/12] tls: support for inline tls

[Atul Gupta]

-Original Message-
From: Dave Watson [mailto:davejwat...@fb.com] 
Sent: Friday, February 23, 2018 11:03 PM
To: Atul Gupta <atul.gu...@chelsio.com>
Cc: da...@davemloft.net; herb...@gondor.apana.org.au; s...@queasysnail.net; 
linux-cry...@vger.kernel.org; netdev@vger.kernel.org; Ganesh GR 
<ganes...@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 
_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.

Re: [Crypto v7 03/12] tls: support for inline tls

[Dave Watson]

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
_prot[TLS_SW_TX], and the unhash function you set here in
TLS_BASE_TX won't be called.

RE: [Crypto v7 03/12] tls: support for inline tls

[Atul Gupta]

-Original Message-
From: Dave Watson [mailto:davejwat...@fb.com] 
Sent: Friday, February 23, 2018 9:53 PM
To: Atul Gupta <atul.gu...@chelsio.com>
Cc: da...@davemloft.net; herb...@gondor.apana.org.au; s...@queasysnail.net; 
linux-cry...@vger.kernel.org; netdev@vger.kernel.org; Ganesh GR 
<ganes...@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(_mutex);
> + list_for_each_entry(dev, _list, dev_list) {
> + if (dev->unhash)
> + dev->unhash(dev, sk);
> + }
> + mutex_unlock(_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.

Re: [Crypto v7 03/12] tls: support for inline tls

[Dave Watson]

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(_mutex);
> + list_for_each_entry(dev, _list, dev_list) {
> + if (dev->unhash)
> + dev->unhash(dev, sk);
> + }
> + mutex_unlock(_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.

[Crypto v7 03/12] tls: support for inline tls

[Atul Gupta]

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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -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(_mutex);
+   list_for_each_entry(dev, _list, dev_list) {
+   if (dev->netdev && dev->netdev(dev, netdev)) {
+   rc = -EEXIST;
+   break;
+   }
+   }
+   mutex_unlock(_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(_mutex);
+   list_for_each_entry(dev, _list, dev_list) {
+   if (dev->feature && dev->feature(dev)) {
+   ctx->tx_conf = TLS_FULL_HW;
+   update_sk_prot(sk, ctx);
+   break;
+   }
+   }
+   mutex_unlock(_mutex);
+   return ctx->tx_conf;
+}
+
+static void tls_hw_unhash(struct sock *sk)
+{
+   struct tls_device *dev;
+
+   mutex_lock(_mutex);
+   list_for_each_entry(dev, _list, dev_list) {
+   if (dev->unhash)
+   dev->unhash(dev, sk);
+   }
+   mutex_unlock(_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(_mutex);
+   list_for_each_entry(dev, _list, dev_list) {
+   if (dev->hash)
+   err |= dev->hash(dev, sk);
+   }
+   mutex_unlock(_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