[permalink] [raw]

Subject: Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()

On 5/20/18 4:25 PM, Kent Overstreet wrote:
> Jens - this series does the rest of the conversions that Christoph wanted, and
> drops bioset_create().
>
> Only lightly tested, but the changes are pretty mechanical. Based on your
> for-next tree.
>
> It's also in the for-jens branch at https://evilpiepirate.org/git/bcachefs.git
>
> Kent Overstreet (12):
> block: convert bounce, q->bio_split to bioset_init()/mempool_init()
> drbd: convert to bioset_init()/mempool_init()
> pktcdvd: convert to bioset_init()/mempool_init()
> lightnvm: convert to bioset_init()/mempool_init()
> bcache: convert to bioset_init()/mempool_init()
> md: convert to bioset_init()/mempool_init()
> dm: convert to bioset_init()/mempool_init()
> target: convert to bioset_init()/mempool_init()
> fs: convert block_dev.c to bioset_init()
> btrfs: convert to bioset_init()/mempool_init()
> xfs: convert to bioset_init()/mempool_init()
> block: Drop bioset_create()

With the reviews, I've queued this up for 4.18.

--
Jens Axboe

2018-06-01 10:53:07

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 06/12] md: convert to bioset_init()/mempool_init()

On Mon, May 21, 2018 at 12:25 AM, Kent Overstreet
<[email protected]> wrote:

> @@ -510,7 +510,10 @@ static void mddev_delayed_delete(struct work_struct *ws);
>
> static void mddev_put(struct mddev *mddev)
> {
> - struct bio_set *bs = NULL, *sync_bs = NULL;
> + struct bio_set bs, sync_bs;
> +
> + memset(&bs, 0, sizeof(bs));
> + memset(&sync_bs, 0, sizeof(sync_bs));
>
> if (!atomic_dec_and_lock(&mddev->active, &all_mddevs_lock))
> return;
> @@ -521,8 +524,8 @@ static void mddev_put(struct mddev *mddev)
> list_del_init(&mddev->all_mddevs);
> bs = mddev->bio_set;
> sync_bs = mddev->sync_set;
> - mddev->bio_set = NULL;
> - mddev->sync_set = NULL;
> + memset(&mddev->bio_set, 0, sizeof(mddev->bio_set));
> + memset(&mddev->sync_set, 0, sizeof(mddev->sync_set));
> if (mddev->gendisk) {
> /* We did a probe so need to clean up. Call
> * queue_work inside the spinlock so that
> @@ -535,10 +538,8 @@ static void mddev_put(struct mddev *mddev)
> kfree(mddev);
> }
> spin_unlock(&all_mddevs_lock);
> - if (bs)
> - bioset_free(bs);
> - if (sync_bs)
> - bioset_free(sync_bs);
> + bioset_exit(&bs);
> + bioset_exit(&sync_bs);
> }

This has triggered a new warning in randconfig builds for me, on 32-bit:

drivers/md/md.c: In function 'mddev_put':
drivers/md/md.c:564:1: error: the frame size of 1096 bytes is larger
than 1024 bytes [-Werror=frame-larger-than=]

and on 64-bit:

drivers/md/md.c: In function 'mddev_put':
drivers/md/md.c:564:1: error: the frame size of 2112 bytes is larger
than 2048 bytes [-Werror=frame-larger-than=]

The size of bio_set is a bit configuration dependent, but it looks like this is
a bit too big to put on the stack either way, epsecially when you traverse
a list and copy two of them with assignments for each loop in 'bs =
mddev->bio_set'.

A related problem is that calling bioset_exit on a copy of the bio_set
might not do the right thing. The function is defined as

void bioset_exit(struct bio_set *bs)
{
if (bs->rescue_workqueue)
destroy_workqueue(bs->rescue_workqueue);
bs->rescue_workqueue = NULL;

mempool_exit(&bs->bio_pool);
mempool_exit(&bs->bvec_pool);

bioset_integrity_free(bs);
if (bs->bio_slab)
bio_put_slab(bs);
bs->bio_slab = NULL;
}

So it calls a couple of functions (detroy_workqueue, mempool_exit,
bioset_integrity_free, bio_put_slab), each of which might rely on
a the structure being the same one that was originally allocated.
If the structure contains any kind of linked list entry, passing a copy
into the list management functions would guarantee corruption.

I could not come up with an obvious fix, but maybe it's possible
to change mddev_put() to do call bioset_exit() before the
structure gets freed? Something like the (completely untested
and probably wrong somewhere) patch below

Signed-off-by: Arnd Bergmann <[email protected]>

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 411f60d591d1..3bf54591ddcd 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -531,11 +531,6 @@ static void mddev_delayed_delete(struct work_struct *ws);

static void mddev_put(struct mddev *mddev)
{
- struct bio_set bs, sync_bs;
-
- memset(&bs, 0, sizeof(bs));
- memset(&sync_bs, 0, sizeof(sync_bs));
-
if (!atomic_dec_and_lock(&mddev->active, &all_mddevs_lock))
return;
if (!mddev->raid_disks && list_empty(&mddev->disks) &&
@@ -543,10 +538,14 @@ static void mddev_put(struct mddev *mddev)
/* Array is not configured at all, and not held active,
* so destroy it */
list_del_init(&mddev->all_mddevs);
- bs = mddev->bio_set;
- sync_bs = mddev->sync_set;
+ spin_unlock(&all_mddevs_lock);
+
+ bioset_exit(&mddev->bio_set);
memset(&mddev->bio_set, 0, sizeof(mddev->bio_set));
+
+ bioset_exit(&mddev->sync_set);
memset(&mddev->sync_set, 0, sizeof(mddev->sync_set));
+
if (mddev->gendisk) {
/* We did a probe so need to clean up. Call
* queue_work inside the spinlock so that
@@ -557,10 +556,9 @@ static void mddev_put(struct mddev *mddev)
queue_work(md_misc_wq, &mddev->del_work);
} else
kfree(mddev);
+ } else {
+ spin_unlock(&all_mddevs_lock);
}
- spin_unlock(&all_mddevs_lock);
- bioset_exit(&bs);
- bioset_exit(&sync_bs);
}

static void md_safemode_timeout(struct timer_list *t);