[PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Change to use strlen() of the desired string for the length
parameter to strncmp().  Otherwise one cannot simply use a
command like 'echo "writesame_16" > .../provisioning_mode'.
This patch makes sysfs writes consistent with other usage.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/sd.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f9d1432..a5eacea 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -402,15 +402,20 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
 	if (sdp->type != TYPE_DISK)
 		return -EINVAL;
 
-	if (!strncmp(buf, lbp_mode[SD_LBP_UNMAP], 20))
+	if (!strncmp(buf, lbp_mode[SD_LBP_UNMAP],
+		     strlen(lbp_mode[SD_LBP_UNMAP])))
 		sd_config_discard(sdkp, SD_LBP_UNMAP);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_WS16], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_WS16],
+			  strlen(lbp_mode[SD_LBP_WS16])))
 		sd_config_discard(sdkp, SD_LBP_WS16);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_WS10], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_WS10],
+			  strlen(lbp_mode[SD_LBP_WS10])))
 		sd_config_discard(sdkp, SD_LBP_WS10);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_ZERO], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_ZERO],
+			  strlen(lbp_mode[SD_LBP_ZERO])))
 		sd_config_discard(sdkp, SD_LBP_ZERO);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_DISABLE], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_DISABLE],
+			  strlen(lbp_mode[SD_LBP_DISABLE])))
 		sd_config_discard(sdkp, SD_LBP_DISABLE);
 	else
 		return -EINVAL;
@@ -444,13 +449,17 @@ zeroing_mode_store(struct device *dev, struct device_attribute *attr,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE], 20))
+	if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE],
+		     strlen(zeroing_mode[SD_ZERO_WRITE])))
 		sdkp->zeroing_mode = SD_ZERO_WRITE;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS],
+			  strlen(zeroing_mode[SD_ZERO_WS])))
 		sdkp->zeroing_mode = SD_ZERO_WS;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP],
+			  strlen(zeroing_mode[SD_ZERO_WS16_UNMAP])))
 		sdkp->zeroing_mode = SD_ZERO_WS16_UNMAP;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP],
+			  strlen(zeroing_mode[SD_ZERO_WS10_UNMAP])))
 		sdkp->zeroing_mode = SD_ZERO_WS10_UNMAP;
 	else
 		return -EINVAL;
-- 
2.1.0

Re: [PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Bart Van Assche]

On Mon, 2017-05-15 at 15:13 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> Change to use strlen() of the desired string for the length
> parameter to strncmp().  Otherwise one cannot simply use a
> command like 'echo "writesame_16" > .../provisioning_mode'.
> This patch makes sysfs writes consistent with other usage.

Hello Ewan,

Sorry but I don't like the approach of this patch. Have you considered
to strip the whitespace from 'buf' with e.g. strim() such that strcmp()
can be used instead of strncmp(buf, ..., strlen(...))?

Thanks,

Bart.

Re: [PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Ewan D. Milne]

On Mon, 2017-05-15 at 20:14 +0000, Bart Van Assche wrote:
> On Mon, 2017-05-15 at 15:13 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > Change to use strlen() of the desired string for the length
> > parameter to strncmp().  Otherwise one cannot simply use a
> > command like 'echo "writesame_16" > .../provisioning_mode'.
> > This patch makes sysfs writes consistent with other usage.
> 
> Hello Ewan,
> 
> Sorry but I don't like the approach of this patch. Have you considered
> to strip the whitespace from 'buf' with e.g. strim() such that strcmp()
> can be used instead of strncmp(buf, ..., strlen(...))?
> 
> Thanks,
> 
> Bart.

I think it's the '\n' that actually causes the problem.
But, what I think we should do is use the same technique everywhere.
So, are you suggesting we change all the other sysfs store routines?

The problem arises when scripts can manipulate some sysfs properties
but not others, and its not obvious why not.  It should be consistent.

Also any change has to be careful to avoid breaking existing scripts.

-Ewan

Re: [PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Bart Van Assche]

On Mon, 2017-05-15 at 17:02 -0400, Ewan D. Milne wrote:
> I think it's the '\n' that actually causes the problem.

Agreed, and that's why I proposed to strip trailing whitespace.

> But, what I think we should do is use the same technique everywhere.

If you have a look at the block layer you will see that the block layer
sysfs code is already using strim() and strstrip().

> So, are you suggesting we change all the other sysfs store routines?

I will leave it to Martin and James to comment on this.

Bart.

Re: [PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Martin K. Petersen]

Bart,

>> So, are you suggesting we change all the other sysfs store routines?
>
> I will leave it to Martin and James to comment on this.

As I mentioned a few weeks ago, I have a patch that converts all these
to sysfs_match_string(). Now that the latter is upstream I intend to
post the patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

[Ewan D. Milne]

On Mon, 2017-05-15 at 17:18 -0400, Martin K. Petersen wrote:
> Bart,
> 
> >> So, are you suggesting we change all the other sysfs store routines?
> >
> > I will leave it to Martin and James to comment on this.
> 
> As I mentioned a few weeks ago, I have a patch that converts all these
> to sysfs_match_string(). Now that the latter is upstream I intend to
> post the patch.
> 

OK good.  One other thing I wanted to check -- I assume it is
intentional that the provisioning mode cannot be set back to "full"
once the device probe has set it and/or it has been manually set via
sysfs?

-Ewan