netdev - Re: [PATCH v22 08/20] nvme-tcp: Deal with netdevice DOWN events

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: <f9e315d1-5876-49da-870b-a073c911df28@grimberg.me>
Date: Wed, 10 Jan 2024 18:01:50 +0200
From: Sagi Grimberg <sagi@...mberg.me>
To: Aurelien Aptel <aaptel@...dia.com>, linux-nvme@...ts.infradead.org,
 netdev@...r.kernel.org, hch@....de, kbusch@...nel.org, axboe@...com,
 chaitanyak@...dia.com, davem@...emloft.net, kuba@...nel.org
Cc: Or Gerlitz <ogerlitz@...dia.com>, aurelien.aptel@...il.com,
 smalin@...dia.com, malin1024@...il.com, yorayz@...dia.com,
 borisp@...dia.com, galshalom@...dia.com, mgurtovoy@...dia.com
Subject: Re: [PATCH v22 08/20] nvme-tcp: Deal with netdevice DOWN events



On 12/21/23 23:33, Aurelien Aptel wrote:
> From: Or Gerlitz <ogerlitz@...dia.com>
> 
> For ddp setup/teardown and resync, the offloading logic
> uses HW resources at the NIC driver such as SQ and CQ.
> 
> These resources are destroyed when the netdevice does down
> and hence we must stop using them before the NIC driver
> destroys them.
> 
> Use netdevice notifier for that matter -- offloaded connections
> are stopped before the stack continues to call the NIC driver
> close ndo.
> 
> We use the existing recovery flow which has the advantage
> of resuming the offload once the connection is re-set.
> 
> This also buys us proper handling for the UNREGISTER event
> b/c our offloading starts in the UP state, and down is always
> there between up to unregister.
> 
> Signed-off-by: Or Gerlitz <ogerlitz@...dia.com>
> Signed-off-by: Boris Pismenny <borisp@...dia.com>
> Signed-off-by: Ben Ben-Ishay <benishay@...dia.com>
> Signed-off-by: Yoray Zack <yorayz@...dia.com>
> Signed-off-by: Shai Malin <smalin@...dia.com>
> Signed-off-by: Aurelien Aptel <aaptel@...dia.com>
> Reviewed-by: Chaitanya Kulkarni <kch@...dia.com>
> Reviewed-by: Sagi Grimberg <sagi@...mberg.me>
> ---
>   drivers/nvme/host/tcp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 6eed24b5f90c..00cb1c8404c4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -235,6 +235,7 @@ struct nvme_tcp_ctrl {
>   
>   static LIST_HEAD(nvme_tcp_ctrl_list);
>   static DEFINE_MUTEX(nvme_tcp_ctrl_mutex);
> +static struct notifier_block nvme_tcp_netdevice_nb;
>   static struct workqueue_struct *nvme_tcp_wq;
>   static const struct blk_mq_ops nvme_tcp_mq_ops;
>   static const struct blk_mq_ops nvme_tcp_admin_mq_ops;
> @@ -3193,6 +3194,32 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
>   	return ERR_PTR(ret);
>   }
>   
> +static int nvme_tcp_netdev_event(struct notifier_block *this,
> +				 unsigned long event, void *ptr)
> +{
> +#ifdef CONFIG_ULP_DDP
> +	struct net_device *ndev = netdev_notifier_info_to_dev(ptr);
> +	struct nvme_tcp_ctrl *ctrl;
> +
> +	switch (event) {
> +	case NETDEV_GOING_DOWN:
> +		mutex_lock(&nvme_tcp_ctrl_mutex);
> +		list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) {
> +			if (ndev == ctrl->ddp_netdev)
> +				nvme_tcp_error_recovery(&ctrl->ctrl);
> +		}
> +		mutex_unlock(&nvme_tcp_ctrl_mutex);
> +		flush_workqueue(nvme_reset_wq);
> +		/*
> +		 * The associated controllers teardown has completed,
> +		 * ddp contexts were also torn down so we should be
> +		 * safe to continue...
> +		 */
> +	}

Will this handler ever react to another type of event? because
if not, maybe its better to just have:

	if (event != NETDEV_GOING_DOWN)
		return NOTIFY_DONE;

	...

> +#endif
> +	return NOTIFY_DONE;
> +}
> +
>   static struct nvmf_transport_ops nvme_tcp_transport = {
>   	.name		= "tcp",
>   	.module		= THIS_MODULE,
> @@ -3208,6 +3235,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
>   
>   static int __init nvme_tcp_init_module(void)
>   {
> +	int ret;
> +
>   	BUILD_BUG_ON(sizeof(struct nvme_tcp_hdr) != 8);
>   	BUILD_BUG_ON(sizeof(struct nvme_tcp_cmd_pdu) != 72);
>   	BUILD_BUG_ON(sizeof(struct nvme_tcp_data_pdu) != 24);
> @@ -3222,8 +3251,19 @@ static int __init nvme_tcp_init_module(void)
>   	if (!nvme_tcp_wq)
>   		return -ENOMEM;
>   
> +	nvme_tcp_netdevice_nb.notifier_call = nvme_tcp_netdev_event;
> +	ret = register_netdevice_notifier(&nvme_tcp_netdevice_nb);
> +	if (ret) {
> +		pr_err("failed to register netdev notifier\n");
> +		goto out_free_workqueue;
> +	}
> +
>   	nvmf_register_transport(&nvme_tcp_transport);
>   	return 0;
> +
> +out_free_workqueue:
> +	destroy_workqueue(nvme_tcp_wq);
> +	return ret;
>   }
>   
>   static void __exit nvme_tcp_cleanup_module(void)
> @@ -3231,6 +3271,7 @@ static void __exit nvme_tcp_cleanup_module(void)
>   	struct nvme_tcp_ctrl *ctrl;
>   
>   	nvmf_unregister_transport(&nvme_tcp_transport);
> +	unregister_netdevice_notifier(&nvme_tcp_netdevice_nb);
>   
>   	mutex_lock(&nvme_tcp_ctrl_mutex);
>   	list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list)