[v2] loop: remember whether sysfs_create_group() succeeded

Commit Message

Tetsuo Handa May 4, 2018, 4:14 p.m. UTC

Jens Axboe wrote:
> > The loop module ignores sysfs_create_group() failure and pretends that
> > LOOP_SET_FD request succeeded. I guess that the author of commit
> > ee86273062cbb310 ("loop: add some basic read-only sysfs attributes")
> > assumed that it is not a fatal error enough to abort LOOP_SET_FD request.
> > 
> > Do we want to abort LOOP_SET_FD request if sysfs_create_group() failed?
> 
> Probably safer to retain that behavior.

OK.

> 
> >> If that's not easily done, then my next suggestion would be to
> >> use a loop flag for it, LO_FLAGS_SYSFS_SETUP or something like that.
> > 
> > Yes, that would be possible.
> 
> Let's make that change.

Since LO_FLAGS_* are defined in include/uapi/linux/loop.h as userspace visible
flags, I feel that using "struct loop_device"->lo_flags for recording whether
sysfs entry exists might be strange... Anyway, updated patch is shown below.



>From c9897b6387b13b533c32dcc624e12a93f23224d0 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Sat, 5 May 2018 00:52:59 +0900
Subject: [PATCH v2] loop: remember whether sysfs_create_group() succeeded

syzbot is hitting WARN() triggered by memory allocation fault
injection [1] because loop module is calling sysfs_remove_group()
when sysfs_create_group() failed.

Fix this by remembering whether sysfs_create_group() succeeded.
To remember it, userspace visible API flag LO_FLAGS_SYSFS_ENTRY is
introduced. This flag indicates that sysfs entries are available, and
should be always set if CONFIG_SYSFS=y because sysfs entries will be
created unless fault injection prevents it.

[1] https://syzkaller.appspot.com/bug?id=3f86c0edf75c86d2633aeb9dd69eccc70bc7e90b

Signed-off-by: Tetsuo Handa <[e-mail]>
Reported-by: syzbot <[e-mail]>
Cc: Jens Axboe <[e-mail]>
---
 drivers/block/loop.c      | 6 ++++--
 include/uapi/linux/loop.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Jens Axboe May 4, 2018, 4:27 p.m. UTC | #1

On 5/4/18 10:14 AM, Tetsuo Handa wrote:
> Jens Axboe wrote:
>>> The loop module ignores sysfs_create_group() failure and pretends that
>>> LOOP_SET_FD request succeeded. I guess that the author of commit
>>> ee86273062cbb310 ("loop: add some basic read-only sysfs attributes")
>>> assumed that it is not a fatal error enough to abort LOOP_SET_FD request.
>>>
>>> Do we want to abort LOOP_SET_FD request if sysfs_create_group() failed?
>>
>> Probably safer to retain that behavior.
> 
> OK.
> 
>>
>>>> If that's not easily done, then my next suggestion would be to
>>>> use a loop flag for it, LO_FLAGS_SYSFS_SETUP or something like that.
>>>
>>> Yes, that would be possible.
>>
>> Let's make that change.
> 
> Since LO_FLAGS_* are defined in include/uapi/linux/loop.h as userspace visible
> flags, I feel that using "struct loop_device"->lo_flags for recording whether
> sysfs entry exists might be strange... Anyway, updated patch is shown below.

Hmm yes, I forgot about that, I guess that makes the flags approach
pretty much useless. Let's just go with your v1 in that case.

Tetsuo Handa May 4, 2018, 4:47 p.m. UTC | #2

Jens Axboe wrote:
> On 5/4/18 10:14 AM, Tetsuo Handa wrote:
> >>>> If that's not easily done, then my next suggestion would be to
> >>>> use a loop flag for it, LO_FLAGS_SYSFS_SETUP or something like that.
> >>>
> >>> Yes, that would be possible.
> >>
> >> Let's make that change.
> > 
> > Since LO_FLAGS_* are defined in include/uapi/linux/loop.h as userspace visible
> > flags, I feel that using "struct loop_device"->lo_flags for recording whether
> > sysfs entry exists might be strange... Anyway, updated patch is shown below.
> 
> Hmm yes, I forgot about that, I guess that makes the flags approach
> pretty much useless. Let's just go with your v1 in that case.
> 

OK. You can s/sysfs_ready/sysfs_init_done/ before you apply to your tree.

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e316..499eafd 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -951,7 +951,8 @@  static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	loop_update_dio(lo);
 	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);
-	loop_sysfs_init(lo);
+	if (IS_ENABLED(CONFIG_SYSFS) && loop_sysfs_init(lo) == 0)
+		lo->lo_flags |= LO_FLAGS_SYSFS_ENTRY;
 	/* let user-space know about the new size */
 	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 
@@ -1070,7 +1071,8 @@  static int loop_clr_fd(struct loop_device *lo)
 		invalidate_bdev(bdev);
 	}
 	set_capacity(lo->lo_disk, 0);
-	loop_sysfs_exit(lo);
+	if (lo->lo_flags & LO_FLAGS_SYSFS_ENTRY)
+		loop_sysfs_exit(lo);
 	if (bdev) {
 		bd_set_size(bdev, 0);
 		/* let user-space know about this change */
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index 080a8df..5de1eaa6 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -23,6 +23,7 @@  enum {
 	LO_FLAGS_AUTOCLEAR	= 4,
 	LO_FLAGS_PARTSCAN	= 8,
 	LO_FLAGS_DIRECT_IO	= 16,
+	LO_FLAGS_SYSFS_ENTRY    = 32,
 };
 
 #include <asm/posix_types.h>	/* for __kernel_old_dev_t */