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
        Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result pointer Andy Lutomirski <luto@amacapital.net> - 2016-08-26 16:40 +0200
          Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result         pointer Christoph Hellwig <hch@lst.de> - 2016-08-26 16:50 +0200
            Re: [PATCH] nvme: Fix nvme_get/set_features() with a NULL result  pointer Keith Busch <keith.busch@intel.com> - 2016-08-26 17:10 +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] | [next] | [standalone]

#1470768

On Aug 25, 2016 4:20 PM, "Jens Axboe" <axboe@fb.com> wrote:
>
> On 08/25/2016 01:54 AM, Andy Lutomirski wrote:
>>
>> 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.)
>
>
> Care to send out what you have? I'd be interested in seeing how much I
> can save on my laptop, haven't played with APST yet.

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=nvme/power

There are some todos:

 - Default to a nonzero latency (e.g. 7ms?  My SSD needs 5.5ms for max
power saving.)  To test it, write something like 7000000 to
apst_max_latency_ns.

 - Add a real changelog.

 - Optionally add a sysfs binfile or other interface to allow
uploading an entire custom table.

 - Consider *deleting* the SCSI translation layer's power saving code.
It looks almost entirely bogus to me.  It has an off-by-one in its
NPSS handling, it hardcodes power state indices which is total BS, it
ignores the distinction between operational and non-operational states
(which I think matters for non-APST usage).  It also seems likely to
be that it's never been used, since it's one of the formerly
crashy-looking set_features users.

You can inspect what it's doing with something like:

# nvme get-feature -f 0x0c -H -s 0 /dev/nvme0

if you have nvme-cli installed.

--Andy

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

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

On Fri, Aug 26, 2016 at 07:31:33AM -0700, Andy Lutomirski wrote:
>  - Consider *deleting* the SCSI translation layer's power saving code.
> It looks almost entirely bogus to me.  It has an off-by-one in its
> NPSS handling, it hardcodes power state indices which is total BS, it
> ignores the distinction between operational and non-operational states
> (which I think matters for non-APST usage).  It also seems likely to
> be that it's never been used, since it's one of the formerly
> crashy-looking set_features users.

Please go ahead and send a patch to delete it.  Adding the whole SCSI
layer was a mistake to start with, and it's always been horribly buggy.
Until I started running the libiscsi testsuite even fairly normal I/O
commands were a sure way to crash it, and crazy things like PM are
almost guaranteed to a) not actually be used by real application and
b) horrible buggy (as you've already noticed)

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

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

On Fri, Aug 26, 2016 at 04:35:57PM +0200, Christoph Hellwig wrote:
> On Fri, Aug 26, 2016 at 07:31:33AM -0700, Andy Lutomirski wrote:
> >  - Consider *deleting* the SCSI translation layer's power saving code.
> > It looks almost entirely bogus to me.  It has an off-by-one in its
> > NPSS handling, it hardcodes power state indices which is total BS, it
> > ignores the distinction between operational and non-operational states
> > (which I think matters for non-APST usage).  It also seems likely to
> > be that it's never been used, since it's one of the formerly
> > crashy-looking set_features users.
> 
> Please go ahead and send a patch to delete it.  Adding the whole SCSI
> layer was a mistake to start with, and it's always been horribly buggy.
> Until I started running the libiscsi testsuite even fairly normal I/O
> commands were a sure way to crash it, and crazy things like PM are
> almost guaranteed to a) not actually be used by real application and
> b) horrible buggy (as you've already noticed)

Ack. If no distros or tools rely on the the SCSI crutch anymore, then
by all means, let's delete it. It's been disabled default for a while
now, and I think/hope everyone we care about has since migrated to
nvme awareness.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web