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

2018-02-24 Thread 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-crypto@vger.kernel.org; net...@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

2018-02-23 Thread 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

2018-02-23 Thread 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-crypto@vger.kernel.org; net...@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

2018-02-23 Thread 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.