netdev - Re: [PATCH net] net/tls: Act on going down event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200213103228.2123025f@kicinski-fedora-PC1C0HJN>
Date:   Thu, 13 Feb 2020 10:32:28 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Or Gerlitz <ogerlitz@...lanox.com>
Cc:     Tariq Toukan <tariqt@...lanox.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net/tls: Act on going down event

On Thu, 13 Feb 2020 16:54:07 +0000 Or Gerlitz wrote:
> By the time of the down event, the netdevice stop ndo was
> already called and the nic driver is likely to destroy the HW
> objects/constructs which are used for the tls_dev_resync op.
> 
> Instead, act on the going down event which is triggered before
> the stop ndo.
>
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Or Gerlitz <ogerlitz@...lanox.com>
> ---
> 
> compile tested only.

For a fix that you have hardware to test that is a little 
disappointing :(

> # vim net/core/dev.c +1555
>  *	This function moves an active device into down state. A
>  *	%NETDEV_GOING_DOWN is sent to the netdev notifier chain. The device
>  *	is then deactivated and finally a %NETDEV_DOWN is sent to the notifier chain.
> [..]
> void dev_close(struct net_device *dev)

As is quoting a comment rather than justifying changes based on the
code.

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 1ba5a92832bb..457c4b8352d8 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -1246,7 +1246,7 @@ static int tls_dev_event(struct notifier_block *this, unsigned long event,
>  			return NOTIFY_DONE;
>  		else
>  			return NOTIFY_BAD;
> -	case NETDEV_DOWN:
> +	case NETDEV_GOING_DOWN:

Now we'll have two race conditions. 1. Traffic is still running while
we remove the connection state. 2. We clean out the state and then
close the device. Between the two new connections may be installed.

I think it's an inherently racy to only be able to perform clean up 
while the device is still up.

>  		return tls_device_down(dev);
>  	}
>  	return NOTIFY_DONE;