Possible unwanted behaviour in configfs

Possible unwanted behaviour in configfs

[Tal Shorer]

Hey,
I encountered a possible unwanted behaviour in the way configfs
handles read() on items' attributes.
The show() function is called only once (which is good) on the first
time the user attempts to read from the file. If show() returns an
error that error is returned to the user. However, if the user calls
read() again, no data is copied (good) and a value of zero is
returned.
This is especially evident when using busybox's cat utility, which
first attempts sendfile64() and if that fails falls back to read(),
giving the user the impression that reading the attribute was
successful but no data was returned.

I traced this to the function configfs_read_file in fs/configfs/file.c
and have two fixes in mind:
1. In fill_read_buffer(), If an error is returned by show(), don't set
buffer->needs_read_fill to zero, making the next read() call show()
again and either get an error or data.
2. Add an "err" field to struct configfs_buffer and keep the returned
error from show() there. Any additional read() on this file will
return this error.

Any thoughts?

Re: Possible unwanted behaviour in configfs

[Christoph Hellwig]

On Sat, Jun 25, 2016 at 12:43:34PM +0300, Tal Shorer wrote:
> I traced this to the function configfs_read_file in fs/configfs/file.c
> and have two fixes in mind:
> 1. In fill_read_buffer(), If an error is returned by show(), don't set
> buffer->needs_read_fill to zero, making the next read() call show()
> again and either get an error or data.
> 2. Add an "err" field to struct configfs_buffer and keep the returned
> error from show() there. Any additional read() on this file will
> return this error.
> 
> Any thoughts?

I think fix 1 is a good idea and I'd love to see a patch for it.
Fix 2 might be problematic as the errors don't need to be persistent,
but I think fix 1 alone should be enough anyway.

[PATCH] fs: configfs: don't set buffer_needs_fill to zero if show() returns error

[Tal Shorer]

A confgifs attribute's show() callback is called once the first time
the user attempts to read from it. If it returns an error, that
error is returned to the user. However, the open file's
buffer_needs_fill is still set to zero and consecutive read() calls
will find an empty buffer that doesn't need filling and return 0 to
the user. This could give the user the wrong impression that the
attribute was read successfully.

Fix this by not setting buffer_needs_fill if show() returns an error,
making consecutive read() calls call show() again and either get an
error again or get data.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 fs/configfs/file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 33b7ee3..68645c9 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -80,11 +80,11 @@ static int fill_read_buffer(struct dentry * dentry, struct configfs_buffer * buf
 
 	count = attr->show(item, buffer->page);
 
-	buffer->needs_read_fill = 0;
 	BUG_ON(count > (ssize_t)SIMPLE_ATTR_SIZE);
-	if (count >= 0)
+	if (count >= 0) {
+		buffer->needs_read_fill = 0;
 		buffer->count = count;
-	else
+	} else
 		ret = count;
 	return ret;
 }
-- 
2.7.4

Re: [PATCH] fs: configfs: don't set buffer_needs_fill to zero if show() returns error

[Tal Shorer]

On Fri, Jul 1, 2016 at 12:28 PM, Tal Shorer <tal.shorer@gmail.com> wrote:
> A confgifs attribute's show() callback is called once the first time
> the user attempts to read from it. If it returns an error, that
> error is returned to the user. However, the open file's
> buffer_needs_fill is still set to zero and consecutive read() calls
> will find an empty buffer that doesn't need filling and return 0 to
> the user. This could give the user the wrong impression that the
> attribute was read successfully.
>
> Fix this by not setting buffer_needs_fill if show() returns an error,
> making consecutive read() calls call show() again and either get an
> error again or get data.
>
> Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
> ---
>  fs/configfs/file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 33b7ee3..68645c9 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -80,11 +80,11 @@ static int fill_read_buffer(struct dentry * dentry, struct configfs_buffer * buf
>
>         count = attr->show(item, buffer->page);
>
> -       buffer->needs_read_fill = 0;
>         BUG_ON(count > (ssize_t)SIMPLE_ATTR_SIZE);
> -       if (count >= 0)
> +       if (count >= 0) {
> +               buffer->needs_read_fill = 0;
>                 buffer->count = count;
> -       else
> +       } else
>                 ret = count;
>         return ret;
>  }
> --
> 2.7.4
>
Ping?

Re: [PATCH] fs: configfs: don't set buffer_needs_fill to zero if show() returns error

[Christoph Hellwig]

Hi Tal,

I've queued up the patch a while ago, and it will go to Linus later this week.