Re: [PATCH] zram: fix bug storing backing_dev

Re: [PATCH] zram: fix bug storing backing_dev

[Minchan Kim]

On Wed, Aug 08, 2018 at 03:31:00PM -0700, Peter Kalauskas wrote:
> The call to strlcpy in backing_dev_store is incorrect. It should take
> the size of the destination buffer instead of the size of the source
> buffer.  Additionally, ignore the newline character (\n) when reading
> the new file_name buffer. This makes it possible to set the backing_dev
> as follows:
> 
>   echo /dev/sdX > /sys/block/zram0/backing_dev
> 
> Signed-off-by: Peter Kalauskas <peskal@google.com>
Acked-by: Minchan Kim <minchan@kernel.org>

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
CC: LKML <linux-kernel@vger.kernel.org>
Cc: <stable@vger.kernel.org>    [4.14+]

> ---
>  drivers/block/zram/zram_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 7436b2d27fa3..3137faea1493 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -336,6 +336,7 @@ static ssize_t backing_dev_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
>  	char *file_name;
> +	size_t sz;
>  	struct file *backing_dev = NULL;
>  	struct inode *inode;
>  	struct address_space *mapping;
> @@ -356,7 +357,11 @@ static ssize_t backing_dev_store(struct device *dev,
>  		goto out;
>  	}
>  
> -	strlcpy(file_name, buf, len);
> +	strlcpy(file_name, buf, PATH_MAX);
> +	/* ignore trailing newline */
> +	sz = strlen(file_name);
> +	if (sz > 0 && file_name[sz - 1] == '\n')
> +		file_name[sz - 1] = 0x00;
>  
>  	backing_dev = filp_open(file_name, O_RDWR|O_LARGEFILE, 0);
>  	if (IS_ERR(backing_dev)) {
> -- 
> 2.18.0.597.ga71716f1ad-goog
> 

Re: [PATCH] zram: fix bug storing backing_dev

[Sergey Senozhatsky]

On (08/13/18 15:16), Minchan Kim wrote:
> > The call to strlcpy in backing_dev_store is incorrect. It should take
> > the size of the destination buffer instead of the size of the source
> > buffer.  Additionally, ignore the newline character (\n) when reading
> > the new file_name buffer. This makes it possible to set the backing_dev
> > as follows:
> > 
> >   echo /dev/sdX > /sys/block/zram0/backing_dev
> > 
> > Signed-off-by: Peter Kalauskas <peskal@google.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> CC: LKML <linux-kernel@vger.kernel.org>
> Cc: <stable@vger.kernel.org>    [4.14+]

Thanks for Cc-ing Minchan.

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

> > -	strlcpy(file_name, buf, len);

This is quite interesting. The reason it worked before was the fact that
strlcpy() copies 'len - 1' bytes, which is strlen(buf) - 1 in our case,
so it accidentally didn't copy the trailing new line symbol. Which also
means that "echo -n /dev/sdX" most likely was broken.

	-ss

Re: [PATCH] zram: fix bug storing backing_dev

[Andrew Morton]

On Mon, 13 Aug 2018 16:38:25 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (08/13/18 15:16), Minchan Kim wrote:
> > > The call to strlcpy in backing_dev_store is incorrect. It should take
> > > the size of the destination buffer instead of the size of the source
> > > buffer.  Additionally, ignore the newline character (\n) when reading
> > > the new file_name buffer. This makes it possible to set the backing_dev
> > > as follows:
> > > 
> > >   echo /dev/sdX > /sys/block/zram0/backing_dev
> > > 
> > > Signed-off-by: Peter Kalauskas <peskal@google.com>
> > Acked-by: Minchan Kim <minchan@kernel.org>
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> > CC: LKML <linux-kernel@vger.kernel.org>
> > Cc: <stable@vger.kernel.org>    [4.14+]
> 
> Thanks for Cc-ing Minchan.
> 
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> > > -	strlcpy(file_name, buf, len);
> 
> This is quite interesting. The reason it worked before was the fact that
> strlcpy() copies 'len - 1' bytes, which is strlen(buf) - 1 in our case,
> so it accidentally didn't copy the trailing new line symbol. Which also
> means that "echo -n /dev/sdX" most likely was broken.
> 

I can't find the original email on lkml for some reason, but I
recreated the patch.

The changelog doesn't describe the end-user impact of the bug, which is
very desirable when tagging a patch for -stable backporting.  Can we
have that paragraph please?

The implementation might be able to use strim() somehow.

Re: [PATCH] zram: fix bug storing backing_dev

[Sergey Senozhatsky]

On (08/14/18 16:45), Andrew Morton wrote:
> > 
> > > > -	strlcpy(file_name, buf, len);
> > 
> > This is quite interesting. The reason it worked before was the fact that
> > strlcpy() copies 'len - 1' bytes, which is strlen(buf) - 1 in our case,
> > so it accidentally didn't copy the trailing new line symbol. Which also
> > means that "echo -n /dev/sdX" most likely was broken.
> > 
> 
> I can't find the original email on lkml for some reason, but I
> recreated the patch.

Neither can I.

> The changelog doesn't describe the end-user impact of the bug, which is
> very desirable when tagging a patch for -stable backporting.  Can we
> have that paragraph please?

The problem is that strlcpy() copies as many bytes as the source string
has, not as many bytes as destination string can fit.

IOW:

	char dst[100];
	char src[1000];
	...
	strlcpy(dst, src, strlen(src));

where it should do

	strlcpy(dst, src, strlen(dst));


> The implementation might be able to use strim() somehow.

strim() trims white-spaces. What we have here is a trailing new line symbol,
which echo appends to the string it writes to the kernel [echo -n switch
disables it]. So we receive a "/dev/name\n" device name from sysfs, which we
unsuccessfully try to open(). To make it all work we need to remove that
trailing new line.

A side note,
There is sysfs_strcmp(), which takes care of that "user space may append
a new line to the string" case, I wonder if we should finally have
sysfs_strcpy(), which would not copy the trailing new line. I think this
"if string[sz - 1] == '\n' then string[sz - 1] == 0x00" is quite common.

	-ss

Re: [PATCH] zram: fix bug storing backing_dev

[Sergey Senozhatsky]

A bunch of corrections

On (08/16/18 10:48), Sergey Senozhatsky wrote:
> The problem is that strlcpy() copies as many bytes as the source string
> has, not as many bytes as destination string can fit.

I mean - strlcpy() expects that the 3rd argument will be sizeof(dst), but
we passed the wrong argument strlen(src), so we used strlcpy() like
if it was strncpy(). The difference between strncpy() and strlcpy() is in
the 3rd argument.

> IOW:
> 
> 	char dst[100];
> 	char src[1000];
> 	...
> 	strlcpy(dst, src, strlen(src));
> 
> where it should do
> 
> 	strlcpy(dst, src, strlen(dst));

			^^^ sizeof()

> A side note,
> There is sysfs_strcmp(), which takes care of that "user space may append

	^^^ sysfs_streq()

	-ss

Re: [PATCH] zram: fix bug storing backing_dev

[Andrew Morton]

On Thu, 16 Aug 2018 10:48:35 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> > The implementation might be able to use strim() somehow.
> 
> strim() trims white-spaces.

Which includes \n.

> What we have here is a trailing new line symbol,
> which echo appends to the string it writes to the kernel [echo -n switch
> disables it]. So we receive a "/dev/name\n" device name from sysfs, which we
> unsuccessfully try to open(). To make it all work we need to remove that
> trailing new line.
> 
> A side note,
> There is sysfs_strcmp(), which takes care of that "user space may append
> a new line to the string" case, I wonder if we should finally have
> sysfs_strcpy(), which would not copy the trailing new line. I think this
> "if string[sz - 1] == '\n' then string[sz - 1] == 0x00" is quite common.

Sure, some additional well-chosen helpers here would be good.  There's
a LOT of code which does basically-the-same-thing with sysfs input. 
And a lot of it misses things, such as leading whitespace.  Passing all
this through helpers would provide consistency as well as code-size
reductions, improved reviewability, etc.

Re: [PATCH] zram: fix bug storing backing_dev

[Peter Kalauskas]

On Tue, Aug 14, 2018 at 04:45:23PM -0700, Andrew Morton wrote:
> The changelog doesn't describe the end-user impact of the bug, which is
> very desirable when tagging a patch for -stable backporting.  Can we
> have that paragraph please?

The end-user impact is that echoing to writeback_dev would not cause it
to change. Would the following statement suffice?

  Without this change, it is not possible to change writeback_dev unless
  the tool used to do so does not include a newline character.  For
  example, 'echo -n' or 'printf' would work, but 'echo' would not work.

I can also include Sergey's description of the bug if that would be
helpful.


On Thu, Aug 16, 2018 at 10:48:35AM +0900, Sergey Senozhatsky wrote:
> On (08/14/18 16:45), Andrew Morton wrote:
> > >
> > > > > -     strlcpy(file_name, buf, len);
> > >
> > > This is quite interesting. The reason it worked before was the fact that
> > > strlcpy() copies 'len - 1' bytes, which is strlen(buf) - 1 in our case,
> > > so it accidentally didn't copy the trailing new line symbol. Which also
> > > means that "echo -n /dev/sdX" most likely was broken.
> > >
> >
> > I can't find the original email on lkml for some reason, but I
> > recreated the patch.
>
> Neither can I.
>

Sorry, I sent it to Minchan directly. I'll send PATCH v2 to lkml, if v2
is needed.


On Wed, Aug 15, 2018 at 08:32:26PM -0700, Andrew Morton wrote:
> On Thu, 16 Aug 2018 10:48:35 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>
> > > The implementation might be able to use strim() somehow.
> >
> > strim() trims white-spaces.
>
> Which includes \n.
>

The if statement is already used by comp_algorithm_store in zram_drv.c,
so perhaps it should remain the if statement for consistency. What do
you think, should I use strim or the if statement?

Re: [PATCH] zram: fix bug storing backing_dev

[Peter Kalauskas]

On Tue, Aug 21, 2018 at 5:45 PM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> Hi,
>
> Did you intend to remove lkml mailing list from Cc?
>

No, that was an accident. I resent it so it's on the record.

I will submit a v2 patch with an updated commit message then.