* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-26 12:12 [PATCH] loop: fix backing file reference leak on validation error Li Chen
@ 2025-09-28 1:54 ` yangerkun
2025-09-29 12:54 ` Li Chen
2025-09-28 13:48 ` Markus Elfring
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: yangerkun @ 2025-09-28 1:54 UTC (permalink / raw)
To: Li Chen, Jens Axboe, linux-block, linux-kernel
在 2025/9/26 20:12, Li Chen 写道:
> loop_change_fd() and loop_configure() call loop_check_backing_file()
> to validate the new backing file. If validation fails, the reference
> acquired by fget() was not dropped, leaking a file reference.
>
> Fix this by calling fput(file) before returning the error.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
You'd better add a fix tag:
Fixes: f5c84eff634b ("loop: Add sanity check for read/write_iter")
Or this patch looks good to me.
Reviewed-by: Yang Erkun <yangerkun@huawei.com>
> ---
> drivers/block/loop.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 053a086d547e..94ec7f747f36 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -551,8 +551,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> return -EBADF;
>
> error = loop_check_backing_file(file);
> - if (error)
> + if (error) {
> + fput(file);
> return error;
> + }
>
> /* suppress uevents while reconfiguring the device */
> dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> @@ -993,8 +995,10 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> return -EBADF;
>
> error = loop_check_backing_file(file);
> - if (error)
> + if (error) {
> + fput(file);
> return error;
> + }
>
> is_loop = is_loop_device(file);
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-28 1:54 ` yangerkun
@ 2025-09-29 12:54 ` Li Chen
0 siblings, 0 replies; 10+ messages in thread
From: Li Chen @ 2025-09-29 12:54 UTC (permalink / raw)
To: yangerkun; +Cc: Jens Axboe, linux-block, linux-kernel
Hi Erkun,
---- On Sun, 28 Sep 2025 09:54:51 +0800 yangerkun <yangerkun@huawei.com> wrote ---
>
>
> 在 2025/9/26 20:12, Li Chen 写道:
> > loop_change_fd() and loop_configure() call loop_check_backing_file()
> > to validate the new backing file. If validation fails, the reference
> > acquired by fget() was not dropped, leaking a file reference.
> >
> > Fix this by calling fput(file) before returning the error.
> >
> > Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
>
> You'd better add a fix tag:
>
> Fixes: f5c84eff634b ("loop: Add sanity check for read/write_iter")
Thanks, I would add it in v2.
Regards,
Li
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-26 12:12 [PATCH] loop: fix backing file reference leak on validation error Li Chen
2025-09-28 1:54 ` yangerkun
@ 2025-09-28 13:48 ` Markus Elfring
2025-09-29 12:53 ` Li Chen
2025-09-29 1:11 ` Yu Kuai
2025-09-29 5:14 ` Ming Lei
3 siblings, 1 reply; 10+ messages in thread
From: Markus Elfring @ 2025-09-28 13:48 UTC (permalink / raw)
To: Li Chen, linux-block, Jens Axboe; +Cc: Li Chen, LKML, Yang Erkun
…
> Fix this by calling fput(file) before returning the error.
…
> +++ b/drivers/block/loop.c
…
How do you think about to increase the application of scope-based resource management?
https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/file.h#L97
Regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-28 13:48 ` Markus Elfring
@ 2025-09-29 12:53 ` Li Chen
2025-09-29 15:01 ` Ming Lei
0 siblings, 1 reply; 10+ messages in thread
From: Li Chen @ 2025-09-29 12:53 UTC (permalink / raw)
To: Markus Elfring; +Cc: Li Chen, linux-block, Jens Axboe, LKML, Yang Erkun
Hi Markus,
---- On Sun, 28 Sep 2025 21:48:23 +0800 Markus Elfring <Markus.Elfring@web.de> wrote ---
> …
> > Fix this by calling fput(file) before returning the error.
> …
> > +++ b/drivers/block/loop.c
> …
>
> How do you think about to increase the application of scope-based resource management?
> https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/file.h#L97
Looks good; I will add a commit to switch to scope-based resource management in v2.
Thanks for your suggestion!
Regards,
Li
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-29 12:53 ` Li Chen
@ 2025-09-29 15:01 ` Ming Lei
2025-09-30 0:05 ` Li Chen
0 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2025-09-29 15:01 UTC (permalink / raw)
To: Li Chen; +Cc: Markus Elfring, Li Chen, linux-block, Jens Axboe, LKML,
Yang Erkun
On Mon, Sep 29, 2025 at 8:54 PM Li Chen <me@linux.beauty> wrote:
>
> Hi Markus,
>
> ---- On Sun, 28 Sep 2025 21:48:23 +0800 Markus Elfring <Markus.Elfring@web.de> wrote ---
> > …
> > > Fix this by calling fput(file) before returning the error.
> > …
> > > +++ b/drivers/block/loop.c
> > …
> >
> > How do you think about to increase the application of scope-based resource management?
> > https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/file.h#L97
>
> Looks good; I will add a commit to switch to scope-based resource management in v2.
> Thanks for your suggestion!
Please don't do it as one bug fix, the whole fix chain needs to
backport, and scope-based
fput is just added in v6.15.
However, you can do it as one cleanup after the fix is merged.
Thanks,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-29 15:01 ` Ming Lei
@ 2025-09-30 0:05 ` Li Chen
0 siblings, 0 replies; 10+ messages in thread
From: Li Chen @ 2025-09-30 0:05 UTC (permalink / raw)
To: Ming Lei; +Cc: Markus Elfring, Li Chen, linux-block, Jens Axboe, LKML,
Yang Erkun
Hi Ming,
---- On Mon, 29 Sep 2025 23:01:50 +0800 Ming Lei <ming.lei@redhat.com> wrote ---
> On Mon, Sep 29, 2025 at 8:54 PM Li Chen <me@linux.beauty> wrote:
> >
> > Hi Markus,
> >
> > ---- On Sun, 28 Sep 2025 21:48:23 +0800 Markus Elfring <Markus.Elfring@web.de> wrote ---
> > > …
> > > > Fix this by calling fput(file) before returning the error.
> > > …
> > > > +++ b/drivers/block/loop.c
> > > …
> > >
> > > How do you think about to increase the application of scope-based resource management?
> > > https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/file.h#L97
> >
> > Looks good; I will add a commit to switch to scope-based resource management in v2.
> > Thanks for your suggestion!
>
> Please don't do it as one bug fix, the whole fix chain needs to
> backport, and scope-based
> fput is just added in v6.15.
>
> However, you can do it as one cleanup after the fix is merged.
Noted, thanks for your tip.
Regards,
Li
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-26 12:12 [PATCH] loop: fix backing file reference leak on validation error Li Chen
2025-09-28 1:54 ` yangerkun
2025-09-28 13:48 ` Markus Elfring
@ 2025-09-29 1:11 ` Yu Kuai
2025-09-29 12:56 ` Li Chen
2025-09-29 5:14 ` Ming Lei
3 siblings, 1 reply; 10+ messages in thread
From: Yu Kuai @ 2025-09-29 1:11 UTC (permalink / raw)
To: Li Chen, Jens Axboe, linux-block, linux-kernel, yukuai (C)
Hi,
在 2025/09/26 20:12, Li Chen 写道:
> loop_change_fd() and loop_configure() call loop_check_backing_file()
> to validate the new backing file. If validation fails, the reference
> acquired by fget() was not dropped, leaking a file reference.
>
> Fix this by calling fput(file) before returning the error.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
> drivers/block/loop.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 053a086d547e..94ec7f747f36 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -551,8 +551,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> return -EBADF;
>
> error = loop_check_backing_file(file);
> - if (error)
> + if (error) {
> + fput(file);
> return error;
> + }
>
> /* suppress uevents while reconfiguring the device */
> dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> @@ -993,8 +995,10 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> return -EBADF;
>
> error = loop_check_backing_file(file);
> - if (error)
> + if (error) {
> + fput(file);
> return error;
> + }
>
> is_loop = is_loop_device(file);
>
>
The changes look correct, however, I'll prefer to change the error path
to the reverse order and add a new error tag.
Thanks,
Kuai
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-29 1:11 ` Yu Kuai
@ 2025-09-29 12:56 ` Li Chen
0 siblings, 0 replies; 10+ messages in thread
From: Li Chen @ 2025-09-29 12:56 UTC (permalink / raw)
To: Yu Kuai; +Cc: Jens Axboe, linux-block, linux-kernel, yukuai
Hi Yu,
---- On Mon, 29 Sep 2025 09:11:04 +0800 Yu Kuai <yukuai1@huaweicloud.com> wrote ---
> Hi,
>
> 在 2025/09/26 20:12, Li Chen 写道:
> > loop_change_fd() and loop_configure() call loop_check_backing_file()
> > to validate the new backing file. If validation fails, the reference
> > acquired by fget() was not dropped, leaking a file reference.
> >
> > Fix this by calling fput(file) before returning the error.
> >
> > Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> > ---
> > drivers/block/loop.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 053a086d547e..94ec7f747f36 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -551,8 +551,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> > return -EBADF;
> >
> > error = loop_check_backing_file(file);
> > - if (error)
> > + if (error) {
> > + fput(file);
> > return error;
> > + }
> >
> > /* suppress uevents while reconfiguring the device */
> > dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> > @@ -993,8 +995,10 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> > return -EBADF;
> >
> > error = loop_check_backing_file(file);
> > - if (error)
> > + if (error) {
> > + fput(file);
> > return error;
> > + }
> >
> > is_loop = is_loop_device(file);
> >
> >
>
> The changes look correct, however, I'll prefer to change the error path
> to the reverse order and add a new error tag.
Thanks, but I will switch to scope-based resource management in v2, as suggested by Markus.
Regards,
Li
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] loop: fix backing file reference leak on validation error
2025-09-26 12:12 [PATCH] loop: fix backing file reference leak on validation error Li Chen
` (2 preceding siblings ...)
2025-09-29 1:11 ` Yu Kuai
@ 2025-09-29 5:14 ` Ming Lei
3 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2025-09-29 5:14 UTC (permalink / raw)
To: Li Chen; +Cc: Jens Axboe, linux-block, linux-kernel, Ming Lei
On Fri, Sep 26, 2025 at 8:13 PM Li Chen <me@linux.beauty> wrote:
>
> loop_change_fd() and loop_configure() call loop_check_backing_file()
> to validate the new backing file. If validation fails, the reference
> acquired by fget() was not dropped, leaking a file reference.
>
> Fix this by calling fput(file) before returning the error.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
Good catch:
Fixes: f5c84eff634b ("loop: Add sanity check for read/write_iter")
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Thanks,
^ permalink raw reply [flat|nested] 10+ messages in thread