[PATCH] loop: remember whether sysfs_create_group() succeeded

[PATCH] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

>>From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 2 May 2018 23:03:48 +0900
Subject: [PATCH] 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.

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

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+9f03168400f56df89dbc6f1751f4458fe739ff29@syzkaller.appspotmail.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/loop.c | 11 ++++++-----
 drivers/block/loop.h |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e316..1d758d8 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -809,16 +809,17 @@ static ssize_t loop_attr_dio_show(struct loop_device *lo, char *buf)
 	.attrs= loop_attrs,
 };
 
-static int loop_sysfs_init(struct loop_device *lo)
+static void loop_sysfs_init(struct loop_device *lo)
 {
-	return sysfs_create_group(&disk_to_dev(lo->lo_disk)->kobj,
-				  &loop_attribute_group);
+	lo->sysfs_ready = !sysfs_create_group(&disk_to_dev(lo->lo_disk)->kobj,
+					      &loop_attribute_group);
 }
 
 static void loop_sysfs_exit(struct loop_device *lo)
 {
-	sysfs_remove_group(&disk_to_dev(lo->lo_disk)->kobj,
-			   &loop_attribute_group);
+	if (lo->sysfs_ready)
+		sysfs_remove_group(&disk_to_dev(lo->lo_disk)->kobj,
+				   &loop_attribute_group);
 }
 
 static void loop_config_discard(struct loop_device *lo)
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index b78de98..73c801f 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -58,6 +58,7 @@ struct loop_device {
 	struct kthread_worker	worker;
 	struct task_struct	*worker_task;
 	bool			use_dio;
+	bool			sysfs_ready;
 
 	struct request_queue	*lo_queue;
 	struct blk_mq_tag_set	tag_set;
-- 
1.8.3.1

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Jens Axboe]

On 5/4/18 5:47 AM, Tetsuo Handa wrote:
>>>From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Wed, 2 May 2018 23:03:48 +0900
> Subject: [PATCH] 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.

Can we store this locally instead of in the loop_device? Also,
naming wise, something like sysfs_init_done would be more readily
understandable.

-- 
Jens Axboe

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

Jens Axboe wrote:
> On 5/4/18 5:47 AM, Tetsuo Handa wrote:
> >>From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Date: Wed, 2 May 2018 23:03:48 +0900
> > Subject: [PATCH] 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.
> 
> Can we store this locally instead of in the loop_device? Also,
> naming wise, something like sysfs_init_done would be more readily
> understandable.

Whether sysfs entry for this loop device exists is per "struct loop_device"
flag, isn't it? What does "locally" mean?

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Jens Axboe]

On 5/4/18 8:27 AM, Tetsuo Handa wrote:
> Jens Axboe wrote:
>> On 5/4/18 5:47 AM, Tetsuo Handa wrote:
>>> >From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Date: Wed, 2 May 2018 23:03:48 +0900
>>> Subject: [PATCH] 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.
>>
>> Can we store this locally instead of in the loop_device? Also,
>> naming wise, something like sysfs_init_done would be more readily
>> understandable.
> 
> Whether sysfs entry for this loop device exists is per "struct loop_device"
> flag, isn't it? What does "locally" mean?

I'm assuming this is calling remove in an error path when alloc fails.
So it should be possible to know locally whether this was done or not,
before calling the teardown. Storing this is in the loop_device seems
like a bit of a hack.

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.

-- 
Jens Axboe

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

Jens Axboe wrote:
> On 5/4/18 8:27 AM, Tetsuo Handa wrote:
> > Jens Axboe wrote:
> >> On 5/4/18 5:47 AM, Tetsuo Handa wrote:
> >>> >From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
> >>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >>> Date: Wed, 2 May 2018 23:03:48 +0900
> >>> Subject: [PATCH] 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.
> >>
> >> Can we store this locally instead of in the loop_device? Also,
> >> naming wise, something like sysfs_init_done would be more readily
> >> understandable.
> > 
> > Whether sysfs entry for this loop device exists is per "struct loop_device"
> > flag, isn't it? What does "locally" mean?
> 
> I'm assuming this is calling remove in an error path when alloc fails.
> So it should be possible to know locally whether this was done or not,
> before calling the teardown. Storing this is in the loop_device seems
> like a bit of a hack.

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?

> 
> 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.

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Jens Axboe]

On 5/4/18 8:40 AM, Tetsuo Handa wrote:
> Jens Axboe wrote:
>> On 5/4/18 8:27 AM, Tetsuo Handa wrote:
>>> Jens Axboe wrote:
>>>> On 5/4/18 5:47 AM, Tetsuo Handa wrote:
>>>>> >From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
>>>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>>>> Date: Wed, 2 May 2018 23:03:48 +0900
>>>>> Subject: [PATCH] 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.
>>>>
>>>> Can we store this locally instead of in the loop_device? Also,
>>>> naming wise, something like sysfs_init_done would be more readily
>>>> understandable.
>>>
>>> Whether sysfs entry for this loop device exists is per "struct loop_device"
>>> flag, isn't it? What does "locally" mean?
>>
>> I'm assuming this is calling remove in an error path when alloc fails.
>> So it should be possible to know locally whether this was done or not,
>> before calling the teardown. Storing this is in the loop_device seems
>> like a bit of a hack.
> 
> 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.

>> 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.

-- 
Jens Axboe

[PATCH v2] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

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 <penguin-kernel@I-love.SAKURA.ne.jp>
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 <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+9f03168400f56df89dbc6f1751f4458fe739ff29@syzkaller.appspotmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/loop.c      | 6 ++++--
 include/uapi/linux/loop.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

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 */
-- 
1.8.3.1

Re: [PATCH v2] loop: remember whether sysfs_create_group() succeeded

[Jens Axboe]

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.

-- 
Jens Axboe

Re: [PATCH v2] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

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.

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Milan Broz]

On 05/04/2018 04:40 PM, Tetsuo Handa 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.

IIRC we added sysfs attributes to easily access loop info for a regular user
in lsblk command (and perhaps even in udev rules).

The ioctl interface was still controlling the loop device, so the failure
here was not meant to be fatal. TBH I think was not a great idea.
 
> Do we want to abort LOOP_SET_FD request if sysfs_create_group() failed?

I would prefer failure - there are several utilities that expects attributes in
sysfs to be valid (for example I print info from here in cryptsetup status
if the backing image is an image), so ignoring failure put the system
in inconsistent state.

Thanks for fixing this,
Milan

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Tetsuo Handa]

Milan Broz wrote:
> On 05/04/2018 04:40 PM, Tetsuo Handa 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.
> 
> IIRC we added sysfs attributes to easily access loop info for a regular user
> in lsblk command (and perhaps even in udev rules).
> 
> The ioctl interface was still controlling the loop device, so the failure
> here was not meant to be fatal. TBH I think was not a great idea.

Thanks for reply.

>  
> > Do we want to abort LOOP_SET_FD request if sysfs_create_group() failed?
> 
> I would prefer failure - there are several utilities that expects attributes in
> sysfs to be valid (for example I print info from here in cryptsetup status
> if the backing image is an image), so ignoring failure put the system
> in inconsistent state.

I see. But can we for now send v1 patch for 4.17 release (and postpone making
LOOP_SET_FD request fail if sysfs_create_group() failed)? This bug has so far
crashed syzbot tests for 6432 times in 190 days.

We have a lot of bugs regarding loop module which prevent syzbot from
finding other bugs. I want to immediately squash bugs in block/loop so that
we can reduce false-positive hung task reports.

> 
> Thanks for fixing this,
> Milan
> 

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Milan Broz]

On 05/05/2018 01:49 PM, Tetsuo Handa wrote:
> Milan Broz wrote:
>>> Do we want to abort LOOP_SET_FD request if sysfs_create_group() failed?
>>
>> I would prefer failure - there are several utilities that expects attributes in
>> sysfs to be valid (for example I print info from here in cryptsetup status
>> if the backing image is an image), so ignoring failure put the system
>> in inconsistent state.
> 
> I see. But can we for now send v1 patch for 4.17 release (and postpone making
> LOOP_SET_FD request fail if sysfs_create_group() failed)? This bug has so far
> crashed syzbot tests for 6432 times in 190 days.

Jens already merged it in the block git. So syzbot should be more happy now :)
> We have a lot of bugs regarding loop module which prevent syzbot from
> finding other bugs. I want to immediately squash bugs in block/loop so that
> we can reduce false-positive hung task reports.

Sure, syzbot is definitely very useful idea, thanks!

Milan

Re: [PATCH] loop: remember whether sysfs_create_group() succeeded

[Jan Kara]

On Fri 04-05-18 20:47:29, Tetsuo Handa wrote:
> >From 626d33de1b70b11ecaf95a9f83f7644998e54cbb Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Wed, 2 May 2018 23:03:48 +0900
> Subject: [PATCH] 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.
> 
> [1] https://syzkaller.appspot.com/bug?id=3f86c0edf75c86d2633aeb9dd69eccc70bc7e90b
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+9f03168400f56df89dbc6f1751f4458fe739ff29@syzkaller.appspotmail.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jens Axboe <axboe@kernel.dk>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/block/loop.c | 11 ++++++-----
>  drivers/block/loop.h |  1 +
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 5d4e316..1d758d8 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -809,16 +809,17 @@ static ssize_t loop_attr_dio_show(struct loop_device *lo, char *buf)
>  	.attrs= loop_attrs,
>  };
>  
> -static int loop_sysfs_init(struct loop_device *lo)
> +static void loop_sysfs_init(struct loop_device *lo)
>  {
> -	return sysfs_create_group(&disk_to_dev(lo->lo_disk)->kobj,
> -				  &loop_attribute_group);
> +	lo->sysfs_ready = !sysfs_create_group(&disk_to_dev(lo->lo_disk)->kobj,
> +					      &loop_attribute_group);
>  }
>  
>  static void loop_sysfs_exit(struct loop_device *lo)
>  {
> -	sysfs_remove_group(&disk_to_dev(lo->lo_disk)->kobj,
> -			   &loop_attribute_group);
> +	if (lo->sysfs_ready)
> +		sysfs_remove_group(&disk_to_dev(lo->lo_disk)->kobj,
> +				   &loop_attribute_group);
>  }
>  
>  static void loop_config_discard(struct loop_device *lo)
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index b78de98..73c801f 100644
> --- a/drivers/block/loop.h
> +++ b/drivers/block/loop.h
> @@ -58,6 +58,7 @@ struct loop_device {
>  	struct kthread_worker	worker;
>  	struct task_struct	*worker_task;
>  	bool			use_dio;
> +	bool			sysfs_ready;
>  
>  	struct request_queue	*lo_queue;
>  	struct blk_mq_tag_set	tag_set;
> -- 
> 1.8.3.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR