* Re: [PATCH] kernfs: move return value check after kmalloc()
2021-05-21 2:55 [PATCH] kernfs: move return value check after kmalloc() Austin Kim
@ 2021-05-21 3:28 ` NeilBrown
2021-05-21 4:39 ` Greg KH
2021-05-21 12:55 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2021-05-21 3:28 UTC (permalink / raw)
To: Austin Kim; +Cc: gregkh, tj, linux-kernel, austin.kim, austindh.kim
On Fri, 21 May 2021, Austin Kim wrote:
> With 414985ae23c0 ("sysfs, kernfs: move file core code to fs/kernfs/file.c"),
> 'return -ENOMEM' is executed when kmalloc() returns NULL.
>
> Since 'commit 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc
> files use the buffer.")', 'return -ENOMEM' statement is not properly located.
>
> Fix it by moving 'return -ENOMEM' after return from kmalloc().
I don't think there is anything to "fix" here. The current code is
correct.
The difference between the current code and your new code is purely a
difference in style. I don't object to your change, but only if it is
presented as a style improvement. It is not a code fix.
NeilBrown
>
> Fixes: 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc files use the buffer.")
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
> fs/kernfs/file.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index c75719312147..c5e2429af836 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -191,10 +191,11 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> buf = of->prealloc_buf;
> if (buf)
> mutex_lock(&of->prealloc_mutex);
> - else
> + else {
> buf = kmalloc(len, GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> + if (!buf)
> + return -ENOMEM;
> + }
>
> /*
> * @of->mutex nests outside active ref and is used both to ensure that
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kernfs: move return value check after kmalloc()
2021-05-21 2:55 [PATCH] kernfs: move return value check after kmalloc() Austin Kim
2021-05-21 3:28 ` NeilBrown
@ 2021-05-21 4:39 ` Greg KH
2021-05-21 6:55 ` Austin Kim
2021-05-21 12:55 ` David Laight
2 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2021-05-21 4:39 UTC (permalink / raw)
To: Austin Kim; +Cc: tj, neilb, linux-kernel, austin.kim
On Fri, May 21, 2021 at 03:55:25AM +0100, Austin Kim wrote:
> With 414985ae23c0 ("sysfs, kernfs: move file core code to fs/kernfs/file.c"),
> 'return -ENOMEM' is executed when kmalloc() returns NULL.
>
> Since 'commit 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc
> files use the buffer.")', 'return -ENOMEM' statement is not properly located.
>
> Fix it by moving 'return -ENOMEM' after return from kmalloc().
>
> Fixes: 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc files use the buffer.")
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
> fs/kernfs/file.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index c75719312147..c5e2429af836 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -191,10 +191,11 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> buf = of->prealloc_buf;
> if (buf)
> mutex_lock(&of->prealloc_mutex);
> - else
> + else {
> buf = kmalloc(len, GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> + if (!buf)
> + return -ENOMEM;
> + }
>
> /*
> * @of->mutex nests outside active ref and is used both to ensure that
> --
> 2.20.1
>
Like Neil said, I don't see the "bug" you are fixing here. What is
currently wrong with the existing code?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kernfs: move return value check after kmalloc()
2021-05-21 4:39 ` Greg KH
@ 2021-05-21 6:55 ` Austin Kim
2021-05-21 12:15 ` David Laight
0 siblings, 1 reply; 6+ messages in thread
From: Austin Kim @ 2021-05-21 6:55 UTC (permalink / raw)
To: Greg KH; +Cc: tj, neilb, linux-kernel, 김동현
2021년 5월 21일 (금) 오후 1:39, Greg KH <gregkh@linuxfoundation.org>님이 작성:
>
> On Fri, May 21, 2021 at 03:55:25AM +0100, Austin Kim wrote:
> > With 414985ae23c0 ("sysfs, kernfs: move file core code to fs/kernfs/file.c"),
> > 'return -ENOMEM' is executed when kmalloc() returns NULL.
> >
> > Since 'commit 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc
> > files use the buffer.")', 'return -ENOMEM' statement is not properly located.
> >
> > Fix it by moving 'return -ENOMEM' after return from kmalloc().
> >
> > Fixes: 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc files use the buffer.")
> > Signed-off-by: Austin Kim <austindh.kim@gmail.com>
[...]
>
> Like Neil said, I don't see the "bug" you are fixing here. What is
> currently wrong with the existing code?
I just found something to improve a little, which has nothing to do with 'Bug'.
(of->prealloc_buf is allocated ahead of kernfs_file_read_iter().)
Should be cautious of adding 'Fixes' tag.
Thanks
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] kernfs: move return value check after kmalloc()
2021-05-21 6:55 ` Austin Kim
@ 2021-05-21 12:15 ` David Laight
0 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2021-05-21 12:15 UTC (permalink / raw)
To: 'Austin Kim', Greg KH
Cc: tj, neilb, linux-kernel, 김동현
From: Austin Kim
> Sent: 21 May 2021 07:55
>
> 2021년 5월 21일 (금) 오후 1:39, Greg KH <gregkh@linuxfoundation.org>님이 작성:
> >
> > On Fri, May 21, 2021 at 03:55:25AM +0100, Austin Kim wrote:
> > > With 414985ae23c0 ("sysfs, kernfs: move file core code to fs/kernfs/file.c"),
> > > 'return -ENOMEM' is executed when kmalloc() returns NULL.
> > >
> > > Since 'commit 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc
> > > files use the buffer.")', 'return -ENOMEM' statement is not properly located.
> > >
> > > Fix it by moving 'return -ENOMEM' after return from kmalloc().
> > >
> > > Fixes: 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc files use the buffer.")
> > > Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> [...]
> >
> > Like Neil said, I don't see the "bug" you are fixing here. What is
> > currently wrong with the existing code?
...
If you look at the generated code you'll almost certainly
find no significant differences.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] kernfs: move return value check after kmalloc()
2021-05-21 2:55 [PATCH] kernfs: move return value check after kmalloc() Austin Kim
2021-05-21 3:28 ` NeilBrown
2021-05-21 4:39 ` Greg KH
@ 2021-05-21 12:55 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2021-05-21 12:55 UTC (permalink / raw)
To: 'Austin Kim', gregkh, tj, neilb; +Cc: linux-kernel, austin.kim
From: Austin Kim
> Sent: 21 May 2021 03:55
>
> With 414985ae23c0 ("sysfs, kernfs: move file core code to fs/kernfs/file.c"),
> 'return -ENOMEM' is executed when kmalloc() returns NULL.
>
> Since 'commit 4ef67a8c95f3 ("sysfs/kernfs: make read requests on pre-alloc
> files use the buffer.")', 'return -ENOMEM' statement is not properly located.
Much more interesting is that the read is bounded to PAGE_SIZE
but the buffer is of->atomic_write_len bytes long.
Seems far too easy for the read() function to overwrite the buffer.
Either that, or you can have files longer than PAGE_SIZE that you
can write but not read all of!
There is also the question of whether the atomic operations
needed lock and unlock the preallocated buffer are actually
slower than kmalloc removing a buffer from a cpu-local list.
And, of course, kmalloc(PAGE_SIZE + 1, ) is horrid...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 6+ messages in thread