| Message ID | 20160405133657.GA3078@soda.linbit (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 05 2016, Lars Ellenberg wrote: > blk_check_plugged() will return a pointer > to an object linked on current->plug->cb_list. > > That list may "at any time" be implicitly cleared by > blk_flush_plug_list() > flush_plug_callbacks() > either as a result of blk_finish_plug(), > or implicitly by schedule() [and maybe other implicit mechanisms?] I think the only risk here is preemption, so preempt_disable() / preempt_enable() or as you say a spinlock, is sufficient protection. I would suggest preempt_{dis,en}able for the raid5 code. Maybe for raid1/raid10 too just for consistency. > > If there is no protection against an implicit unplug > between the call to blk_check_plug() and using its return value, > that implicit unplug may have already happened, > even before the plug is actually initialized or populated, > and we may be using a pointer to already free()d data. > > I suggest that both raid1 and raid10 can easily be fixed > by moving the call to blk_check_plugged() inside the spinlock. > > For md/raid5 and btrfs/raid56, > I'm unsure how (if) this needs to be fixed. > > The other current in-tree users of blk_check_plugged() > are mm_check_plugged(), and mddev_check_plugged(). > > mm_check_plugged() is already used safely inside a spinlock. > > with mddev_check_plugged() I'm unsure, at least on a preempt kernel. I think this is only an issue on a preempt kernel, and in that case: yes - mddev_check_plugged() needs protection. Maybe preempt enable/disable could be done in blk_check_plugged() so those calls which don't dereference the pointer don't need further protection. Or maybe blk_check_plugged should have WARN_ON_ONCE(!in_atomic()); > > Did I overlook any magic that protects against such implicit unplug? Just the fortunate lack of preemption probably. > > Also, why pretend that a custom plug struct (such as raid1_plug_cb) > may have its member "struct blk_plug_cb cb" at an arbitrary offset? > As it is, raid1_check_plugged() below is actually just a cast. Fair point. I generally prefer container_of to casts because it is more obviously correct, and type checked. However as blk_check_plugged performs the allocation, the blk_plug_cb must be at the start of the containing structure, so the complex tests for handling NULL are just noise. I'd be happy for that to be changed. Thanks, NeilBrown > > Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> > --- > drivers/md/raid1.c | 19 +++++++++++++------ > drivers/md/raid10.c | 21 +++++++++++++-------- > drivers/md/raid5.c | 5 +++++ > fs/btrfs/raid56.c | 5 +++++ > 4 files changed, 36 insertions(+), 14 deletions(-) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index 39fb21e..55dc960 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -1044,6 +1044,18 @@ static void raid1_unplug(struct blk_plug_cb *cb, bool from_schedule) > kfree(plug); > } > > +static struct raid1_plug_cb *raid1_check_plugged(struct mddev *mddev) > +{ > + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ > + struct blk_plug_cb *cb; > + struct raid1_plug_cb *plug = NULL; > + > + cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); > + if (cb) > + plug = container_of(cb, struct raid1_plug_cb, cb); > + return plug; > +} > + > static void raid1_make_request(struct mddev *mddev, struct bio * bio) > { > struct r1conf *conf = mddev->private; > @@ -1060,7 +1072,6 @@ static void raid1_make_request(struct mddev *mddev, struct bio * bio) > & (REQ_DISCARD | REQ_SECURE)); > const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); > struct md_rdev *blocked_rdev; > - struct blk_plug_cb *cb; > struct raid1_plug_cb *plug = NULL; > int first_clone; > int sectors_handled; > @@ -1382,12 +1393,8 @@ read_again: > > atomic_inc(&r1_bio->remaining); > > - cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); > - if (cb) > - plug = container_of(cb, struct raid1_plug_cb, cb); > - else > - plug = NULL; > spin_lock_irqsave(&conf->device_lock, flags); > + plug = raid1_check_plugged(mddev); > if (plug) { > bio_list_add(&plug->pending, mbio); > plug->pending_cnt++; > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index e3fd725..d7d4397 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -1052,6 +1052,18 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule) > kfree(plug); > } > > +static struct raid10_plug_cb *raid10_check_plugged(struct mddev *mddev) > +{ > + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ > + struct blk_plug_cb *cb; > + struct raid10_plug_cb *plug = NULL; > + > + cb = blk_check_plugged(raid10_unplug, mddev, sizeof(*plug)); > + if (cb) > + plug = container_of(cb, struct raid10_plug_cb, cb); > + return plug; > +} > + > static void __make_request(struct mddev *mddev, struct bio *bio) > { > struct r10conf *conf = mddev->private; > @@ -1066,7 +1078,6 @@ static void __make_request(struct mddev *mddev, struct bio *bio) > const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); > unsigned long flags; > struct md_rdev *blocked_rdev; > - struct blk_plug_cb *cb; > struct raid10_plug_cb *plug = NULL; > int sectors_handled; > int max_sectors; > @@ -1369,14 +1380,8 @@ retry_write: > > atomic_inc(&r10_bio->remaining); > > - cb = blk_check_plugged(raid10_unplug, mddev, > - sizeof(*plug)); > - if (cb) > - plug = container_of(cb, struct raid10_plug_cb, > - cb); > - else > - plug = NULL; > spin_lock_irqsave(&conf->device_lock, flags); > + plug = raid10_check_plugged(mddev); > if (plug) { > bio_list_add(&plug->pending, mbio); > plug->pending_cnt++; > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index 8ab8b65..4e3b02b 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -5034,6 +5034,11 @@ static void release_stripe_plug(struct mddev *mddev, > } > > cb = container_of(blk_cb, struct raid5_plug_cb, cb); > +/* FIXME > + * Nothing protects current from being scheduled, which means cb, aka plug, > + * may implicitly be "unplugged" any time now, before it even is initialized, > + * and will then be a pointer to free()d space. > + */ > > if (cb->list.next == NULL) { > int i; > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 0b7792e..17757d4 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -1774,6 +1774,11 @@ int raid56_parity_write(struct btrfs_root *root, struct bio *bio, > cb = blk_check_plugged(btrfs_raid_unplug, root->fs_info, > sizeof(*plug)); > if (cb) { > +/* FIXME > + * Nothing protects current from being scheduled, which means cb, aka plug, > + * may implicitly be "unplugged" any time now, before it even is initialized, > + * and will then be a pointer to free()d space. > + */ > plug = container_of(cb, struct btrfs_plug_cb, cb); > if (!plug->info) { > plug->info = root->fs_info; > -- > 1.9.1
On Tue, Apr 05, 2016 at 03:36:57PM +0200, Lars Ellenberg wrote: > blk_check_plugged() will return a pointer > to an object linked on current->plug->cb_list. > > That list may "at any time" be implicitly cleared by > blk_flush_plug_list() > flush_plug_callbacks() > either as a result of blk_finish_plug(), > or implicitly by schedule() [and maybe other implicit mechanisms?] > > If there is no protection against an implicit unplug > between the call to blk_check_plug() and using its return value, > that implicit unplug may have already happened, > even before the plug is actually initialized or populated, > and we may be using a pointer to already free()d data. > > I suggest that both raid1 and raid10 can easily be fixed > by moving the call to blk_check_plugged() inside the spinlock. > > For md/raid5 and btrfs/raid56, > I'm unsure how (if) this needs to be fixed. I think you're right, digging in to see if there's something I missed. But as Neil said, it looks like we just got saved by preemption being off by default. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 05, 2016 at 03:36:57PM +0200, Lars Ellenberg wrote: > blk_check_plugged() will return a pointer > to an object linked on current->plug->cb_list. > > That list may "at any time" be implicitly cleared by > blk_flush_plug_list() > flush_plug_callbacks() > either as a result of blk_finish_plug(), > or implicitly by schedule() [and maybe other implicit mechanisms?] > > If there is no protection against an implicit unplug > between the call to blk_check_plug() and using its return value, > that implicit unplug may have already happened, > even before the plug is actually initialized or populated, > and we may be using a pointer to already free()d data. This isn't correct. flush plug is never called in preemption, which is designed only called when the task is going to sleep. See sched_submit_work. Am I missing anything? Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 06 2016, Shaohua Li wrote: > On Tue, Apr 05, 2016 at 03:36:57PM +0200, Lars Ellenberg wrote: >> blk_check_plugged() will return a pointer >> to an object linked on current->plug->cb_list. >> >> That list may "at any time" be implicitly cleared by >> blk_flush_plug_list() >> flush_plug_callbacks() >> either as a result of blk_finish_plug(), >> or implicitly by schedule() [and maybe other implicit mechanisms?] >> >> If there is no protection against an implicit unplug >> between the call to blk_check_plug() and using its return value, >> that implicit unplug may have already happened, >> even before the plug is actually initialized or populated, >> and we may be using a pointer to already free()d data. > > This isn't correct. flush plug is never called in preemption, which is designed > only called when the task is going to sleep. See sched_submit_work. Am I > missing anything? Ahh yes, thanks. Only two places call blk_schedule_flush_plug(). One is io_schedule_timeout() which must be called explicitly. There other is, as you say, sched_submit_work(). It starts: static inline void sched_submit_work(struct task_struct *tsk) { if (!tsk->state || tsk_is_pi_blocked(tsk)) return; so if the task is runnable, then as include/linux/sched.h:#define TASK_RUNNING 0 it will never call blk_schedule_flush_plug(). So I don't think you are missing anything, we were. Lars: have your concerns been relieved or do you still have reason to think there is a problem? Thanks, NeilBrown
On Wed, Apr 06, 2016 at 01:10:57PM +1000, NeilBrown wrote: > On Wed, Apr 06 2016, Shaohua Li wrote: > > > On Tue, Apr 05, 2016 at 03:36:57PM +0200, Lars Ellenberg wrote: > >> blk_check_plugged() will return a pointer > >> to an object linked on current->plug->cb_list. > >> > >> That list may "at any time" be implicitly cleared by > >> blk_flush_plug_list() > >> flush_plug_callbacks() > >> either as a result of blk_finish_plug(), > >> or implicitly by schedule() [and maybe other implicit mechanisms?] > >> > >> If there is no protection against an implicit unplug > >> between the call to blk_check_plug() and using its return value, > >> that implicit unplug may have already happened, > >> even before the plug is actually initialized or populated, > >> and we may be using a pointer to already free()d data. > > > > This isn't correct. flush plug is never called in preemption, which is designed > > only called when the task is going to sleep. See sched_submit_work. Am I > > missing anything? > > Ahh yes, thanks. > > Only two places call blk_schedule_flush_plug(). > One is io_schedule_timeout() which must be called explicitly. > There other is, as you say, sched_submit_work(). It starts: > > static inline void sched_submit_work(struct task_struct *tsk) > { > if (!tsk->state || tsk_is_pi_blocked(tsk)) > return; > > so if the task is runnable, then as > include/linux/sched.h:#define TASK_RUNNING 0 > > it will never call blk_schedule_flush_plug(). > > So I don't think you are missing anything, we were. > > Lars: have your concerns been relieved or do you still have reason to > think there is a problem? So just don't call anything that might_sleep() between blk_check_plug() and using its return value. All good. I thought I must have overlooked something. Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 39fb21e..55dc960 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -1044,6 +1044,18 @@ static void raid1_unplug(struct blk_plug_cb *cb, bool from_schedule) kfree(plug); } +static struct raid1_plug_cb *raid1_check_plugged(struct mddev *mddev) +{ + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ + struct blk_plug_cb *cb; + struct raid1_plug_cb *plug = NULL; + + cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); + if (cb) + plug = container_of(cb, struct raid1_plug_cb, cb); + return plug; +} + static void raid1_make_request(struct mddev *mddev, struct bio * bio) { struct r1conf *conf = mddev->private; @@ -1060,7 +1072,6 @@ static void raid1_make_request(struct mddev *mddev, struct bio * bio) & (REQ_DISCARD | REQ_SECURE)); const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); struct md_rdev *blocked_rdev; - struct blk_plug_cb *cb; struct raid1_plug_cb *plug = NULL; int first_clone; int sectors_handled; @@ -1382,12 +1393,8 @@ read_again: atomic_inc(&r1_bio->remaining); - cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); - if (cb) - plug = container_of(cb, struct raid1_plug_cb, cb); - else - plug = NULL; spin_lock_irqsave(&conf->device_lock, flags); + plug = raid1_check_plugged(mddev); if (plug) { bio_list_add(&plug->pending, mbio); plug->pending_cnt++; diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index e3fd725..d7d4397 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -1052,6 +1052,18 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule) kfree(plug); } +static struct raid10_plug_cb *raid10_check_plugged(struct mddev *mddev) +{ + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ + struct blk_plug_cb *cb; + struct raid10_plug_cb *plug = NULL; + + cb = blk_check_plugged(raid10_unplug, mddev, sizeof(*plug)); + if (cb) + plug = container_of(cb, struct raid10_plug_cb, cb); + return plug; +} + static void __make_request(struct mddev *mddev, struct bio *bio) { struct r10conf *conf = mddev->private; @@ -1066,7 +1078,6 @@ static void __make_request(struct mddev *mddev, struct bio *bio) const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); unsigned long flags; struct md_rdev *blocked_rdev; - struct blk_plug_cb *cb; struct raid10_plug_cb *plug = NULL; int sectors_handled; int max_sectors; @@ -1369,14 +1380,8 @@ retry_write: atomic_inc(&r10_bio->remaining); - cb = blk_check_plugged(raid10_unplug, mddev, - sizeof(*plug)); - if (cb) - plug = container_of(cb, struct raid10_plug_cb, - cb); - else - plug = NULL; spin_lock_irqsave(&conf->device_lock, flags); + plug = raid10_check_plugged(mddev); if (plug) { bio_list_add(&plug->pending, mbio); plug->pending_cnt++; diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 8ab8b65..4e3b02b 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -5034,6 +5034,11 @@ static void release_stripe_plug(struct mddev *mddev, } cb = container_of(blk_cb, struct raid5_plug_cb, cb); +/* FIXME + * Nothing protects current from being scheduled, which means cb, aka plug, + * may implicitly be "unplugged" any time now, before it even is initialized, + * and will then be a pointer to free()d space. + */ if (cb->list.next == NULL) { int i; diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 0b7792e..17757d4 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -1774,6 +1774,11 @@ int raid56_parity_write(struct btrfs_root *root, struct bio *bio, cb = blk_check_plugged(btrfs_raid_unplug, root->fs_info, sizeof(*plug)); if (cb) { +/* FIXME + * Nothing protects current from being scheduled, which means cb, aka plug, + * may implicitly be "unplugged" any time now, before it even is initialized, + * and will then be a pointer to free()d space. + */ plug = container_of(cb, struct btrfs_plug_cb, cb); if (!plug->info) { plug->info = root->fs_info;
blk_check_plugged() will return a pointer to an object linked on current->plug->cb_list. That list may "at any time" be implicitly cleared by blk_flush_plug_list() flush_plug_callbacks() either as a result of blk_finish_plug(), or implicitly by schedule() [and maybe other implicit mechanisms?] If there is no protection against an implicit unplug between the call to blk_check_plug() and using its return value, that implicit unplug may have already happened, even before the plug is actually initialized or populated, and we may be using a pointer to already free()d data. I suggest that both raid1 and raid10 can easily be fixed by moving the call to blk_check_plugged() inside the spinlock. For md/raid5 and btrfs/raid56, I'm unsure how (if) this needs to be fixed. The other current in-tree users of blk_check_plugged() are mm_check_plugged(), and mddev_check_plugged(). mm_check_plugged() is already used safely inside a spinlock. with mddev_check_plugged() I'm unsure, at least on a preempt kernel. Did I overlook any magic that protects against such implicit unplug? Also, why pretend that a custom plug struct (such as raid1_plug_cb) may have its member "struct blk_plug_cb cb" at an arbitrary offset? As it is, raid1_check_plugged() below is actually just a cast. Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com> --- drivers/md/raid1.c | 19 +++++++++++++------ drivers/md/raid10.c | 21 +++++++++++++-------- drivers/md/raid5.c | 5 +++++ fs/btrfs/raid56.c | 5 +++++ 4 files changed, 36 insertions(+), 14 deletions(-)