-----Original Message----- From: Dave Watson [mailto:davejwatson@xxxxxx] Sent: Friday, February 23, 2018 9:53 PM To: Atul Gupta <atul.gupta@xxxxxxxxxxx> Cc: davem@xxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; sd@xxxxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Ganesh GR <ganeshgr@xxxxxxxxxxx> 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.
