| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 8 May 2015 00:11:55 +0900
From: Minchan Kim <minchan@...nel.org>
To: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCHv4 00/10] add on-demand device creation
Hey,
On Thu, May 07, 2015 at 10:04:54PM +0900, Sergey Senozhatsky wrote:
> On (05/07/15 21:09), Sergey Senozhatsky wrote:
> [..]
> >
> > and there is
> > http://git.kernel.dk/?p=linux-block.git;a=commitdiff;h=6cd18e71;hp=393a33970540ac6a2c894b0d6ef3f5d485860884
> >
> [..]
> > and here:
> > http://www.spinics.net/lists/dm-devel/msg23415.html
> >
> >
> > is there any chance to ask you to test with these patches (no rush, take your time)?
> > as I'm still unable to reproduce it locally.
>
> pathes attached.
It's good timing. When I'm about to dig in, I got your patches
and tested it.
Passed my test script!
BTW, patches you sent landed on mainline?
Anyway, Thanks!
>
> -ss
> From 7056096317825dbe582d005385bc4077ecb62c76 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@...e.de>
> Date: Mon, 27 Apr 2015 04:12:22 +1000
> Subject: [PATCH 2/2] block: destroy bdi before blockdev is unregistered.
>
> Because of the peculiar way that md devices are created (automatically
> when the device node is opened), a new device can be created and
> registered immediately after the
> blk_unregister_region(disk_devt(disk), disk->minors);
> call in del_gendisk().
>
> Therefore it is important that all visible artifacts of the previous
> device are removed before this call. In particular, the 'bdi'.
>
> Since:
> commit c4db59d31e39ea067c32163ac961e9c80198fd37
> Author: Christoph Hellwig <hch@....de>
> fs: don't reassign dirty inodes to default_backing_dev_info
>
> moved the
> device_unregister(bdi->dev);
> call from bdi_unregister() to bdi_destroy() it has been quite easy to
> lose a race and have a new (e.g.) "md127" be created after the
> blk_unregister_region() call and before bdi_destroy() is ultimately
> called by the final 'put_disk', which must come after del_gendisk().
>
> The new device finds that the bdi name is already registered in sysfs
> and complains
>
> > [ 9627.630029] WARNING: CPU: 18 PID: 3330 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x5a/0x70()
> > [ 9627.630032] sysfs: cannot create duplicate filename '/devices/virtual/bdi/9:127'
>
> We can fix this by moving the bdi_destroy() call out of
> blk_release_queue() (which can happen very late when a refcount
> reaches zero) and into blk_cleanup_queue() - which happens exactly when the md
> device driver calls it.
>
> Then it is only necessary for md to call blk_cleanup_queue() before
> del_gendisk(). As loop.c devices are also created on demand by
> opening the device node, we make the same change there.
>
> Fixes: c4db59d31e39ea067c32163ac961e9c80198fd37
> Reported-by: Azat Khuzhin <a3at.mail@...il.com>
> Cc: Christoph Hellwig <hch@....de>
> Cc: stable@...r.kernel.org (v4.0)
> Signed-off-by: NeilBrown <neilb@...e.de>
> Reviewed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Jens Axboe <axboe@...com>
> ---
> block/blk-core.c | 2 ++
> block/blk-sysfs.c | 2 --
> drivers/block/loop.c | 2 +-
> drivers/md/md.c | 4 ++--
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index fd154b9..7871603 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -552,6 +552,8 @@ void blk_cleanup_queue(struct request_queue *q)
> q->queue_lock = &q->__queue_lock;
> spin_unlock_irq(lock);
>
> + bdi_destroy(&q->backing_dev_info);
> +
> /* @q is and will stay empty, shutdown and put */
> blk_put_queue(q);
> }
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index faaf36a..2b8fd30 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -522,8 +522,6 @@ static void blk_release_queue(struct kobject *kobj)
>
> blk_trace_shutdown(q);
>
> - bdi_destroy(&q->backing_dev_info);
> -
> ida_simple_remove(&blk_queue_ida, q->id);
> call_rcu(&q->rcu_head, blk_free_queue_rcu);
> }
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ae3fcb4..d7173cb 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1620,8 +1620,8 @@ out:
>
> static void loop_remove(struct loop_device *lo)
> {
> - del_gendisk(lo->lo_disk);
> blk_cleanup_queue(lo->lo_queue);
> + del_gendisk(lo->lo_disk);
> blk_mq_free_tag_set(&lo->tag_set);
> put_disk(lo->lo_disk);
> kfree(lo);
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d4f31e1..593a024 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4818,12 +4818,12 @@ static void md_free(struct kobject *ko)
> if (mddev->sysfs_state)
> sysfs_put(mddev->sysfs_state);
>
> + if (mddev->queue)
> + blk_cleanup_queue(mddev->queue);
> if (mddev->gendisk) {
> del_gendisk(mddev->gendisk);
> put_disk(mddev->gendisk);
> }
> - if (mddev->queue)
> - blk_cleanup_queue(mddev->queue);
>
> kfree(mddev);
> }
> --
> 2.4.0
>
> From ef8878259ce360ec352b591746761249393cf952 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@...e.de>
> Date: Thu, 7 May 2015 21:49:41 +0900
> Subject: [PATCH 1/2] block: bdi_unregister() now contains very little
> functionality.
>
> It contains a "WARN_ON" if bdi->dev is NULL. This warning is of no
> real consequence as bdi->dev isn't needed by anything else in the function,
> and it triggers if
> blk_cleanup_queue() -> bdi_destroy()
> is called before bdi_unregister, which a subsequent patch will make happen.
> So this isn't wanted.
>
> It also calls bdi_set_min_ratio(). This needs to be called after
> writes through the bdi have all been flushed, and before the bdi is destroyed.
> Calling it early is better than calling it late as it frees up a global
> resource.
>
> Calling it immediately after bdi_wb_shutdown() in bdi_destroy()
> perfectly fits these requirements.
>
> So bdi_unregister can be discarded with the important content moved to
> bdi_destroy, as can the
> writeback_bdi_unregister
> event which is already not used.
>
> This is tagged for 'stable' as it is a pre-requisite for a subsequent
> patch which moves calls to blk_cleanup_queue() before calls to
> del_gendisk(). The commit identified as 'Fixes' removed a lot of
> other functionality from bdi_unregister(), and made a change which
> necessitated moving the blk_cleanup_queue() calls.
>
> Reported-by: Mike Snitzer <snitzer@...hat.com>
> Cc: Christoph Hellwig <hch@....de>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: stable@...r.kernel.org (v4.0)
> Fixes: c4db59d31e39ea067c32163ac961e9c80198fd37
> Signed-off-by: NeilBrown <neilb@...e.de>
> ---
> block/genhd.c | 1 -
> include/linux/backing-dev.h | 1 -
> include/trace/events/writeback.h | 1 -
> mm/backing-dev.c | 18 +-----------------
> 4 files changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 64600e9..2fba82c 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -653,7 +653,6 @@ void del_gendisk(struct gendisk *disk)
> disk->flags &= ~GENHD_FL_UP;
>
> sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
> - bdi_unregister(&disk->queue->backing_dev_info);
> blk_unregister_queue(disk);
> blk_unregister_region(disk_devt(disk), disk->minors);
>
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index aff923a..d87d8ec 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -116,7 +116,6 @@ __printf(3, 4)
> int bdi_register(struct backing_dev_info *bdi, struct device *parent,
> const char *fmt, ...);
> int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> -void bdi_unregister(struct backing_dev_info *bdi);
> int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> void bdi_start_writeback(struct backing_dev_info *bdi, long nr_pages,
> enum wb_reason reason);
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index 880dd74..c178d13 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -250,7 +250,6 @@ DEFINE_EVENT(writeback_class, name, \
> DEFINE_WRITEBACK_EVENT(writeback_nowork);
> DEFINE_WRITEBACK_EVENT(writeback_wake_background);
> DEFINE_WRITEBACK_EVENT(writeback_bdi_register);
> -DEFINE_WRITEBACK_EVENT(writeback_bdi_unregister);
>
> DECLARE_EVENT_CLASS(wbc_class,
> TP_PROTO(struct writeback_control *wbc, struct backing_dev_info *bdi),
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 6dc4580..000e7b3 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -359,23 +359,6 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi)
> flush_delayed_work(&bdi->wb.dwork);
> }
>
> -/*
> - * Called when the device behind @bdi has been removed or ejected.
> - *
> - * We can't really do much here except for reducing the dirty ratio at
> - * the moment. In the future we should be able to set a flag so that
> - * the filesystem can handle errors at mark_inode_dirty time instead
> - * of only at writeback time.
> - */
> -void bdi_unregister(struct backing_dev_info *bdi)
> -{
> - if (WARN_ON_ONCE(!bdi->dev))
> - return;
> -
> - bdi_set_min_ratio(bdi, 0);
> -}
> -EXPORT_SYMBOL(bdi_unregister);
> -
> static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi)
> {
> memset(wb, 0, sizeof(*wb));
> @@ -443,6 +426,7 @@ void bdi_destroy(struct backing_dev_info *bdi)
> int i;
>
> bdi_wb_shutdown(bdi);
> + bdi_set_min_ratio(bdi, 0);
>
> WARN_ON(!list_empty(&bdi->work_list));
> WARN_ON(delayed_work_pending(&bdi->wb.dwork));
> --
> 2.4.0
>
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists