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

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

[Andy Lutomirski]

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

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

[Sagi Grimberg]

Looks fine,

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

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

[Jens Axboe]

On 08/24/2016 04:52 AM, Andy Lutomirski wrote:
> 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.

Thanks, applied for 4.8.


-- 
Jens Axboe

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

[Christoph Hellwig]

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)

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

[Andy Lutomirski]

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

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

[Christoph Hellwig]

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.

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Andy Lutomirski]

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

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

[Christoph Hellwig]

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)

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

[Jens Axboe]

On 08/26/2016 08:31 AM, Andy Lutomirski wrote:
> 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.

Thanks, I'll give it a whirl. One issue in a previous patch - you have
nvme_set_features() take a const buffer, which makes sense. But then you
pass it to __nvme_submit_sync_cmd(), which of course takes both
read/write commands, and hence the buffer isn't const.

-- 
Jens Axboe

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

[Jens Axboe]

On 08/26/2016 09:17 AM, Keith Busch wrote:
> 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.

On the FB management front, we don't depend on it. The only case that
has come up where it was needed, was some Intel closed tool ;-)

-- 
Jens Axboe

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

[Keith Busch]

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.