linux-kernel - Re: [PATCH] nvme-pci: use NOWAIT flag for nvme_set_host

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] [day] [month] [year] [list]

Date:   Tue, 30 Jan 2018 09:13:02 -0700
From:   Keith Busch <keith.busch@...el.com>
To:     "jianchao.wang" <jianchao.w.wang@...cle.com>
Cc:     Sagi Grimberg <sagi@...mberg.me>, axboe@...com, hch@....de,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvme-pci: use NOWAIT flag for nvme_set_host_mem

On Tue, Jan 30, 2018 at 11:41:07AM +0800, jianchao.wang wrote:
> Another point that confuses me is that whether nvme_set_host_mem is necessary
> in nvme_dev_disable ?
> As the comment:
> ----
> 		/*
> 		 * If the controller is still alive tell it to stop using the
> 		 * host memory buffer.  In theory the shutdown / reset should
> 		 * make sure that it doesn't access the host memoery anymore,
> 		 * but I'd rather be safe than sorry..
> 		 */
> 		if (dev->host_mem_descs)
> 			nvme_set_host_mem(dev, 0);
> ----
> It is to avoid accessing to host memory from controller. But the host memory is just
> freed when nvme_remove. It looks like we just need to do this in nvme_remove.
> For example:
> -----
> @@ -2553,6 +2545,14 @@ static void nvme_remove(struct pci_dev *pdev)
>         flush_work(&dev->ctrl.reset_work);
>         nvme_stop_ctrl(&dev->ctrl);
>         nvme_remove_namespaces(&dev->ctrl);
> +       /*
> +        * If the controller is still alive tell it to stop using the
> +        * host memory buffer.  In theory the shutdown / reset should
> +        * make sure that it doesn't access the host memoery anymore,
> +        * but I'd rather be safe than sorry..
> +        */
> +       if (dev->host_mem_descs)
> +               nvme_set_host_mem(dev, 0);
>         nvme_dev_disable(dev, true);
>         nvme_free_host_mem(dev);
> ----
> 
> If anything missed, please point out.
> That's really appreciated.

I don't make any such controller, so I can't speak to the necessity of
having this here. I just think having it in the controller disabling
path is for safety. We want the controller to acknowledge that it has
completed any use of the HMB before we make it so it can't access it.