Re: [PATCH 4/4] nvme: re-check security protocol support after reset

On Fri, Feb 17, 2017 at 06:01:28PM +0100, Christoph Hellwig wrote:

On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote:

quoted

On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote:

quoted

@@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
 	if (result)
 		goto out;
 
-	if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
+	kfree(dev->ctrl.opal_dev);
+	if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
 		dev->ctrl.opal_dev =
 			init_opal_dev(&dev->ctrl, &nvme_sec_submit);
 	}

A couple things.

This has a use-after-free in opal_unlock_from_suspend if the nvme
device had an opal_dev before, but no longer support the capability
after resume. So you'd want to set ctrl.opal_dev to NULL after the free.

But we don't want to unconditionally free it anyway during resume
since opal_unlock_from_suspend requires the exisiting opal_dev state
information saved in the 'unlk_list'.

Something like this instead:

Yes, that looks fine to me.  We'll probably also need the additional
fixup Scott pointed out.

I'm working on it now. Do you want a diff like Keith did or a separate patch?