Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1469338 > unrolled thread

[PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

Back to article view | Back to linux.kernel

Contents

  [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer Andy Lutomirski <luto@kernel.org> - 2016-08-24 13:00 +0200
    Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result  pointer Sagi Grimberg <sagi@grimberg.me> - 2016-08-24 13:10 +0200
    Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result         pointer Christoph Hellwig <hch@lst.de> - 2016-08-25 09:40 +0200
      Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer Andy Lutomirski <luto@amacapital.net> - 2016-08-25 10:00 +0200
        Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result         pointer Christoph Hellwig <hch@lst.de> - 2016-08-25 10:30 +0200

#1469338 — [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

nvme_set_features() callers seem to expect that passing NULL as the
result pointer is acceptable.  Teach nvme_set_features() not to try to
write to the NULL address.

For symmetry, make the same change to nvme_get_features(), despite the
fact that all current callers pass a valid result pointer.

I assume that this bug hasn't been reported in practice because
the callers that pass NULL are all in the SCSI translation layer
and no one uses the relevant operations.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7ff2e820bbf4..ebae74f6da9c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -608,7 +608,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 
 	ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
 			NVME_QID_ANY, 0, 0);
-	if (ret >= 0)
+	if (ret >= 0 && result)
 		*result = le32_to_cpu(cqe.result);
 	return ret;
 }
@@ -628,7 +628,7 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 
 	ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
 			NVME_QID_ANY, 0, 0);
-	if (ret >= 0)
+	if (ret >= 0 && result)
 		*result = le32_to_cpu(cqe.result);
 	return ret;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1469339 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

Looks fine,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[toc] | [prev] | [next] | [standalone]

#1469916 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

Ooops, yes.

Are you looking into new nvme_set_features users?  Another thing
we need to tackle is either replacing dma_addr argument with a
a real kernel pointer (or just kill it until users show up)

[toc] | [prev] | [next] | [standalone]

#1469941

On Thu, Aug 25, 2016 at 12:38 AM, Christoph Hellwig <hch@lst.de> wrote:
> Ooops, yes.
>
> Are you looking into new nvme_set_features users?  Another thing
> we need to tackle is either replacing dma_addr argument with a
> a real kernel pointer (or just kill it until users show up)

I am, and I have a patch to do the former (and to add a length
argument).  But that's not -stable material.

While I have your attention: the new use is to enable APST (power
saving).  In theory, it seems like I should integrate with dev_pm_qos
so that the standard interface for setting a latency limit will work,
but, on brief inspection, there are literally no drivers in the entire
tree that do this.  Am I missing something?  My current draft patch
just adds a sysfs attribute.  (It saves a *lot* of power on my laptop,
so supporting APST is worth doing.)

--Andy

[toc] | [prev] | [next] | [standalone]

#1469960 — Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer

On Thu, Aug 25, 2016 at 12:54:00AM -0700, Andy Lutomirski wrote:
> I am, and I have a patch to do the former (and to add a length
> argument).  But that's not -stable material.

Great!

> While I have your attention: the new use is to enable APST (power
> saving).  In theory, it seems like I should integrate with dev_pm_qos
> so that the standard interface for setting a latency limit will work,
> but, on brief inspection, there are literally no drivers in the entire
> tree that do this.  Am I missing something?  My current draft patch
> just adds a sysfs attribute.  (It saves a *lot* of power on my laptop,
> so supporting APST is worth doing.)

I'm proably the wrong person to talk about PM interfaces, but not
having it used anywhere is a red herring and needs a ping to the PM
folks why we even have that code in the tree.

Integrating it with the PM core would be preferable, but until that
happens we should just enable it by default in the nvme driver and
have a local tweak (sysfs file, or maybe even just a run-time writeable
module parameter) to turn it off.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web