On Wed, 27 May 2020 10:02:42 +0530 rohit maheshwari wrote:
> On 27/05/20 4:12 AM, Jakub Kicinski wrote:
> > On Tue, 26 May 2020 19:36:34 +0530 Rohit Maheshwari wrote:
> >> Current design enables ktls setting from start, which is not
> >> efficient. Now the feature will be enabled when user demands
> >> TLS offload on any interface.
> >>
> >> v1->v2:
> >> - taking ULD module refcount till any single connection exists.
> >> - taking rtnl_lock() before clearing tls_devops.
> > Callers of tls_devops don't hold the rtnl_lock.
> I think I should correct the statement here, " taking rtnl_lock()
> before clearing tls_devops and device flags". There won't be any
> synchronization issue while clearing tls_devops now, because I
> am incrementing module refcount of CRYPTO ULD, so this will
> never be called if there is any connection (new connection
> request) exists.
Please take a look at tls_set_device_offload():
if (!(netdev->features & NETIF_F_HW_TLS_TX)) {
rc = -EOPNOTSUPP;
goto release_netdev;
}
/* Avoid offloading if the device is down
* We don't want to offload new flows after
* the NETDEV_DOWN event
*
* device_offload_lock is taken in tls_devices's NETDEV_DOWN
* handler thus protecting from the device going down before
* ctx was added to tls_device_list.
*/
down_read(&device_offload_lock);
if (!(netdev->flags & IFF_UP)) {
rc = -EINVAL;
goto release_lock;
}
ctx->priv_ctx_tx = offload_ctx;
rc = netdev->tlsdev_ops->tls_dev_add(netdev, sk, TLS_OFFLOAD_CTX_DIR_TX,
&ctx->crypto_send.info,
tcp_sk(sk)->write_seq);
This does not hold rtnl_lock. If you clear the ops between the feature
check and the call - there will be a crash. Never clear tls ops on a
registered netdev.
Why do you clear the ops in the first place? It shouldn't be necessary.
