| Message ID | 201805042047.EFJ26068.OtOJFSLFFQOVHM@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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?
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 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.
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.
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
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 >
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
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 >
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;