[PATCH net] tls: handle NETDEV_UNREGISTER for tls device

[PATCH net] tls: handle NETDEV_UNREGISTER for tls device

[Raed Salem]

This patch to handle the asynchronous unregister
device event so the device tls offload resources
could be cleanly released.

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Signed-off-by: Raed Salem <raeds@mellanox.com>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
---
 net/tls/tls_device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cd91ad8..bbd2a53 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1246,6 +1246,7 @@ static int tls_dev_event(struct notifier_block *this, unsigned long event,
 		else
 			return NOTIFY_BAD;
 	case NETDEV_DOWN:
+	case NETDEV_UNREGISTER:
 		return tls_device_down(dev);
 	}
 	return NOTIFY_DONE;
-- 
1.9.4

Re: [PATCH net] tls: handle NETDEV_UNREGISTER for tls device

[Jakub Kicinski]

On Sun,  2 Feb 2020 13:22:52 +0200, Raed Salem wrote:
> This patch to handle the asynchronous unregister

By asynchronous you mean callback, right?

> device event so the device tls offload resources
> could be cleanly released.

Please tell us how you trigger the error, code is rather self
explanatory.. 

Note that TLS offload can only be installed when device is UP:

	down_read(&device_offload_lock);
	if (!(netdev->flags & IFF_UP)) {
		rc = -EINVAL;
		goto release_lock;
	}

Is there an unregister that doesn't close first?

> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Raed Salem <raeds@mellanox.com>
> Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>

Re: [PATCH net] tls: handle NETDEV_UNREGISTER for tls device

[David Miller]

From: Raed Salem <raeds@mellanox.com>
Date: Sun,  2 Feb 2020 13:22:52 +0200

> This patch to handle the asynchronous unregister
> device event so the device tls offload resources
> could be cleanly released.
> 
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Raed Salem <raeds@mellanox.com>
> Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
> Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>

As per Jakub's feedback, only devices which are UP can have TLS
rules installed to them.

Therefore we will always get the NETDEV_DOWN event first, to release
the TLS resources.

So I am tossing this patch.  If there is a real problem, you must
describe it in detail in the commit message.

RE: [PATCH net] tls: handle NETDEV_UNREGISTER for tls device

[Raed Salem]

Thanks David/Jakubs for the review,

I had problems with the setup, once I have results I resubmit on need.


> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, February 04, 2020 1:31 PM
> To: Raed Salem <raeds@mellanox.com>
> Cc: john.fastabend@gmail.com; daniel@iogearbox.net; kuba@kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net] tls: handle NETDEV_UNREGISTER for tls device
> 
> From: Raed Salem <raeds@mellanox.com>
> Date: Sun,  2 Feb 2020 13:22:52 +0200
> 
> > This patch to handle the asynchronous unregister device event so the
> > device tls offload resources could be cleanly released.
> >
> > Fixes: e8f69799810c ("net/tls: Add generic NIC offload
> > infrastructure")
> > Signed-off-by: Raed Salem <raeds@mellanox.com>
> > Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
> > Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
> 
> As per Jakub's feedback, only devices which are UP can have TLS rules
> installed to them.
> 
> Therefore we will always get the NETDEV_DOWN event first, to release the
> TLS resources.
> 
> So I am tossing this patch.  If there is a real problem, you must describe it in
> detail in the commit message.