From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:55344 "EHLO
        mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL)
        by vger.kernel.org with ESMTP id S1751420AbeBWRdL (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 23 Feb 2018 12:33:11 -0500
Date: Fri, 23 Feb 2018 09:32:52 -0800
From: Dave Watson <davejwatson@fb.com>
To: Atul Gupta <atul.gupta@chelsio.com>
CC: "davem@davemloft.net" <davem@davemloft.net>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "sd@queasysnail.net" <sd@queasysnail.net>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Ganesh GR <ganeshgr@chelsio.com>
Subject: Re: [Crypto v7 03/12] tls: support for inline tls
Message-ID: <20180223173252.GA91653@davejwatson-mba>
References: <1519321890-22919-1-git-send-email-atul.gupta@chelsio.com>
 <20180223162313.GA82586@davejwatson-mba>
 <CY4PR1201MB023019CF5E1D47CA9D136B4797CC0@CY4PR1201MB0230.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CY4PR1201MB023019CF5E1D47CA9D136B4797CC0@CY4PR1201MB0230.namprd12.prod.outlook.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.