[PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[Kent Overstreet]

mempool_init()/bioset_init() require that the mempools/biosets be zeroed
first; they probably should not _require_ this, but not allocating those
structs with kzalloc is a fairly nonsensical thing to do (calling
mempool_exit()/bioset_exit() on an uninitialized mempool/bioset is legal
and safe, but only works if said memory was zeroed.)

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---

Linus,

I fucked up majorly on the bioset/mempool conversion - I forgot to check that
everything biosets/mempools were being embedded in was actually being zeroed on
allocation. Device mapper currently explodes, you'll probably want to apply this
patch post haste.

I have now done that auditing, for every single conversion - this patch fixes
everything I found. There do not seem to be any incorrect ones outside of device
mapper...

We'll probably want a second patch that either a) changes
bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
my preference, WARN() or BUG() if they're passed memory that isn't zeroed.

 drivers/md/dm-bio-prison-v1.c | 2 +-
 drivers/md/dm-bio-prison-v2.c | 2 +-
 drivers/md/dm-io.c            | 2 +-
 drivers/md/dm-kcopyd.c        | 2 +-
 drivers/md/dm-region-hash.c   | 2 +-
 drivers/md/dm-snap.c          | 2 +-
 drivers/md/dm-thin.c          | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-bio-prison-v1.c b/drivers/md/dm-bio-prison-v1.c
index 8e33a38083..e794e3662f 100644
--- a/drivers/md/dm-bio-prison-v1.c
+++ b/drivers/md/dm-bio-prison-v1.c
@@ -33,7 +33,7 @@ static struct kmem_cache *_cell_cache;
  */
 struct dm_bio_prison *dm_bio_prison_create(void)
 {
-	struct dm_bio_prison *prison = kmalloc(sizeof(*prison), GFP_KERNEL);
+	struct dm_bio_prison *prison = kzalloc(sizeof(*prison), GFP_KERNEL);
 	int ret;
 
 	if (!prison)
diff --git a/drivers/md/dm-bio-prison-v2.c b/drivers/md/dm-bio-prison-v2.c
index 601b156920..f866bc97b0 100644
--- a/drivers/md/dm-bio-prison-v2.c
+++ b/drivers/md/dm-bio-prison-v2.c
@@ -35,7 +35,7 @@ static struct kmem_cache *_cell_cache;
  */
 struct dm_bio_prison_v2 *dm_bio_prison_create_v2(struct workqueue_struct *wq)
 {
-	struct dm_bio_prison_v2 *prison = kmalloc(sizeof(*prison), GFP_KERNEL);
+	struct dm_bio_prison_v2 *prison = kzalloc(sizeof(*prison), GFP_KERNEL);
 	int ret;
 
 	if (!prison)
diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index 53c6ed0eaa..81ffc59d05 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -51,7 +51,7 @@ struct dm_io_client *dm_io_client_create(void)
 	unsigned min_ios = dm_get_reserved_bio_based_ios();
 	int ret;
 
-	client = kmalloc(sizeof(*client), GFP_KERNEL);
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
index c89a675a2a..ce7efc7434 100644
--- a/drivers/md/dm-kcopyd.c
+++ b/drivers/md/dm-kcopyd.c
@@ -882,7 +882,7 @@ struct dm_kcopyd_client *dm_kcopyd_client_create(struct dm_kcopyd_throttle *thro
 	int r;
 	struct dm_kcopyd_client *kc;
 
-	kc = kmalloc(sizeof(*kc), GFP_KERNEL);
+	kc = kzalloc(sizeof(*kc), GFP_KERNEL);
 	if (!kc)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
index 43149eb493..abf3521b80 100644
--- a/drivers/md/dm-region-hash.c
+++ b/drivers/md/dm-region-hash.c
@@ -180,7 +180,7 @@ struct dm_region_hash *dm_region_hash_create(
 		;
 	nr_buckets >>= 1;
 
-	rh = kmalloc(sizeof(*rh), GFP_KERNEL);
+	rh = kzalloc(sizeof(*rh), GFP_KERNEL);
 	if (!rh) {
 		DMERR("unable to allocate region hash memory");
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index b11ddc55f2..f745404da7 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1120,7 +1120,7 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		origin_mode = FMODE_WRITE;
 	}
 
-	s = kmalloc(sizeof(*s), GFP_KERNEL);
+	s = kzalloc(sizeof(*s), GFP_KERNEL);
 	if (!s) {
 		ti->error = "Cannot allocate private snapshot structure";
 		r = -ENOMEM;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 6c923824ec..5772756c63 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -2861,7 +2861,7 @@ static struct pool *pool_create(struct mapped_device *pool_md,
 		return (struct pool *)pmd;
 	}
 
-	pool = kmalloc(sizeof(*pool), GFP_KERNEL);
+	pool = kzalloc(sizeof(*pool), GFP_KERNEL);
 	if (!pool) {
 		*error = "Error allocating memory for pool";
 		err_p = ERR_PTR(-ENOMEM);
-- 
2.17.1

Re: [PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[Jens Axboe]

On 6/5/18 3:26 AM, Kent Overstreet wrote:
> mempool_init()/bioset_init() require that the mempools/biosets be zeroed
> first; they probably should not _require_ this, but not allocating those
> structs with kzalloc is a fairly nonsensical thing to do (calling
> mempool_exit()/bioset_exit() on an uninitialized mempool/bioset is legal
> and safe, but only works if said memory was zeroed.)
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
> 
> Linus,
> 
> I fucked up majorly on the bioset/mempool conversion - I forgot to check that
> everything biosets/mempools were being embedded in was actually being zeroed on
> allocation. Device mapper currently explodes, you'll probably want to apply this
> patch post haste.
> 
> I have now done that auditing, for every single conversion - this patch fixes
> everything I found. There do not seem to be any incorrect ones outside of device
> mapper...
> 
> We'll probably want a second patch that either a) changes
> bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
> my preference, WARN() or BUG() if they're passed memory that isn't zeroed.

Odd, haven't seen a crash, but probably requires kasan or poisoning to
trigger anything? Mike's tree also had the changes, since they were based
on the block tree.

I can queue this up and ship it later today. Mike, you want to review
this one?

-- 
Jens Axboe

Re: [PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[David Sterba]

On Tue, Jun 05, 2018 at 08:22:22AM -0600, Jens Axboe wrote:
> > I fucked up majorly on the bioset/mempool conversion - I forgot to check that
> > everything biosets/mempools were being embedded in was actually being zeroed on
> > allocation. Device mapper currently explodes, you'll probably want to apply this
> > patch post haste.
> > 
> > I have now done that auditing, for every single conversion - this patch fixes
> > everything I found. There do not seem to be any incorrect ones outside of device
> > mapper...
> > 
> > We'll probably want a second patch that either a) changes
> > bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
> > my preference, WARN() or BUG() if they're passed memory that isn't zeroed.
> 
> Odd, haven't seen a crash, but probably requires kasan or poisoning to
> trigger anything? Mike's tree also had the changes, since they were based
> on the block tree.

eg. fstests/generic/081 crashes (trace below), no KASAN, PAGE_POISONING=y,
PAGE_POISONING_NO_SANITY=y.

> I can queue this up and ship it later today. Mike, you want to review
> this one?

Would be great to push that soon. The fstests build on several DM targets, the
crashes lead to many test failures. I'm going to test the kzalloc fix now.

[ 8546.936276] BUG: unable to handle kernel paging request at ffff8a3314cabf98
[ 8546.943407] PGD 1e4915067 P4D 1e4915067 PUD 0 
[ 8546.948006] Oops: 0000 [#1] PREEMPT SMP
[ 8546.951984] CPU: 5 PID: 11452 Comm: lvm Not tainted 4.17.0-1.ge195904-vanilla+ #249
[ 8546.959849] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
[ 8546.966532] RIP: 0010:remove_element.isra.8+0x2e/0x200
[ 8546.991185] RSP: 0018:ffff9af9c1bf3ba8 EFLAGS: 00010206
[ 8546.996553] RAX: 000000006b6b6b6a RBX: ffff8a2fb95d8008 RCX: 0000000000000000
[ 8547.003831] RDX: 000000006b6b6b6b RSI: 0000000000000000 RDI: ffff8a2fb95d8008
[ 8547.011107] RBP: 000000006b6b6b6a R08: 0000000000000000 R09: 0000000000000001
[ 8547.018378] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8a2fb96f6448
[ 8547.025668] R13: ffff8a2fb0ee6d58 R14: ffffffffc05d2a00 R15: ffff9af9c1bf3d08
[ 8547.032956] FS:  00007fe863936880(0000) GS:ffff8a2fe7000000(0000) knlGS:0000000000000000
[ 8547.041269] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 8547.047167] CR2: ffff8a3314cabf98 CR3: 00000001fee98000 CR4: 00000000000006e0
[ 8547.054457] Call Trace:
[ 8547.057078]  ? dev_wait+0xa0/0xa0 [dm_mod]
[ 8547.061323]  mempool_exit+0x18/0x50
[ 8547.064974]  dm_io_client_destroy+0xe/0x30 [dm_mod]
[ 8547.070028]  dm_kcopyd_client_destroy+0x86/0x130 [dm_mod]
[ 8547.075614]  ? dev_wait+0xa0/0xa0 [dm_mod]
[ 8547.079875]  snapshot_dtr+0xb3/0x170 [dm_snapshot]
[ 8547.084844]  dm_table_destroy+0x62/0x140 [dm_mod]
[ 8547.089720]  ? dev_wait+0xa0/0xa0 [dm_mod]
[ 8547.094000]  dev_suspend+0xe6/0x270 [dm_mod]
[ 8547.098448]  ctl_ioctl+0x220/0x540 [dm_mod]
[ 8547.102845]  dm_ctl_ioctl+0xa/0x10 [dm_mod]
[ 8547.107196]  do_vfs_ioctl+0x91/0x6c0
[ 8547.110923]  ? kfree+0x1e5/0x310
[ 8547.114313]  ? syscall_trace_enter+0x1ce/0x3c0
[ 8547.118915]  ksys_ioctl+0x70/0x80
[ 8547.122388]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[ 8547.127247]  __x64_sys_ioctl+0x16/0x20
[ 8547.131140]  do_syscall_64+0x62/0x1c0
[ 8547.134968]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Re: [PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[Jens Axboe]

On 6/5/18 8:35 AM, David Sterba wrote:
> On Tue, Jun 05, 2018 at 08:22:22AM -0600, Jens Axboe wrote:
>>> I fucked up majorly on the bioset/mempool conversion - I forgot to check that
>>> everything biosets/mempools were being embedded in was actually being zeroed on
>>> allocation. Device mapper currently explodes, you'll probably want to apply this
>>> patch post haste.
>>>
>>> I have now done that auditing, for every single conversion - this patch fixes
>>> everything I found. There do not seem to be any incorrect ones outside of device
>>> mapper...
>>>
>>> We'll probably want a second patch that either a) changes
>>> bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
>>> my preference, WARN() or BUG() if they're passed memory that isn't zeroed.
>>
>> Odd, haven't seen a crash, but probably requires kasan or poisoning to
>> trigger anything? Mike's tree also had the changes, since they were based
>> on the block tree.
> 
> eg. fstests/generic/081 crashes (trace below), no KASAN, PAGE_POISONING=y,
> PAGE_POISONING_NO_SANITY=y.
> 
>> I can queue this up and ship it later today. Mike, you want to review
>> this one?
> 
> Would be great to push that soon. The fstests build on several DM targets, the
> crashes lead to many test failures. I'm going to test the kzalloc fix now.

For sure, it should go asap.

-- 
Jens Axboe

Re: dm: Use kzalloc for all structs with embedded biosets/mempools

[Mike Snitzer]

On Tue, Jun 05 2018 at 10:22P -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> On 6/5/18 3:26 AM, Kent Overstreet wrote:
> > mempool_init()/bioset_init() require that the mempools/biosets be zeroed
> > first; they probably should not _require_ this, but not allocating those
> > structs with kzalloc is a fairly nonsensical thing to do (calling
> > mempool_exit()/bioset_exit() on an uninitialized mempool/bioset is legal
> > and safe, but only works if said memory was zeroed.)
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> > ---
> > 
> > Linus,
> > 
> > I fucked up majorly on the bioset/mempool conversion - I forgot to check that
> > everything biosets/mempools were being embedded in was actually being zeroed on
> > allocation. Device mapper currently explodes, you'll probably want to apply this
> > patch post haste.
> > 
> > I have now done that auditing, for every single conversion - this patch fixes
> > everything I found. There do not seem to be any incorrect ones outside of device
> > mapper...
> > 
> > We'll probably want a second patch that either a) changes
> > bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
> > my preference, WARN() or BUG() if they're passed memory that isn't zeroed.
> 
> Odd, haven't seen a crash, but probably requires kasan or poisoning to
> trigger anything? Mike's tree also had the changes, since they were based
> on the block tree.
> 
> I can queue this up and ship it later today. Mike, you want to review
> this one?

Yes, looks good.

>From the start of revisiting these changes last week, Kent and I
discussed whether it was safe to call mempool_exit() even if
mempool_init() failed or was never called.  He advised that it was so
long as the containing structure was zeroed.  But I forgot to audit that 
aspect.  So this was an oversight by both of us.

DM core uses kvzalloc_node for struct mapped_device and cache, crypt,
integrity, verity-fec and zoned targets are already using kzalloc as
needed.

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: dm: Use kzalloc for all structs with embedded biosets/mempools

[Jens Axboe]

On 6/5/18 8:45 AM, Mike Snitzer wrote:
> On Tue, Jun 05 2018 at 10:22P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
> 
>> On 6/5/18 3:26 AM, Kent Overstreet wrote:
>>> mempool_init()/bioset_init() require that the mempools/biosets be zeroed
>>> first; they probably should not _require_ this, but not allocating those
>>> structs with kzalloc is a fairly nonsensical thing to do (calling
>>> mempool_exit()/bioset_exit() on an uninitialized mempool/bioset is legal
>>> and safe, but only works if said memory was zeroed.)
>>>
>>> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
>>> ---
>>>
>>> Linus,
>>>
>>> I fucked up majorly on the bioset/mempool conversion - I forgot to check that
>>> everything biosets/mempools were being embedded in was actually being zeroed on
>>> allocation. Device mapper currently explodes, you'll probably want to apply this
>>> patch post haste.
>>>
>>> I have now done that auditing, for every single conversion - this patch fixes
>>> everything I found. There do not seem to be any incorrect ones outside of device
>>> mapper...
>>>
>>> We'll probably want a second patch that either a) changes
>>> bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
>>> my preference, WARN() or BUG() if they're passed memory that isn't zeroed.
>>
>> Odd, haven't seen a crash, but probably requires kasan or poisoning to
>> trigger anything? Mike's tree also had the changes, since they were based
>> on the block tree.
>>
>> I can queue this up and ship it later today. Mike, you want to review
>> this one?
> 
> Yes, looks good.
> 
> From the start of revisiting these changes last week, Kent and I
> discussed whether it was safe to call mempool_exit() even if
> mempool_init() failed or was never called.  He advised that it was so
> long as the containing structure was zeroed.  But I forgot to audit that 
> aspect.  So this was an oversight by both of us.
> 
> DM core uses kvzalloc_node for struct mapped_device and cache, crypt,
> integrity, verity-fec and zoned targets are already using kzalloc as
> needed.
> 
> Acked-by: Mike Snitzer <snitzer@redhat.com>

Thanks Mike, I'll push this out this morning.

-- 
Jens Axboe

Re: [PATCH] dm: Use kzalloc for all structs with embedded biosets/mempools

[David Sterba]

On Tue, Jun 05, 2018 at 04:35:07PM +0200, David Sterba wrote:
> On Tue, Jun 05, 2018 at 08:22:22AM -0600, Jens Axboe wrote:
> > > I fucked up majorly on the bioset/mempool conversion - I forgot to check that
> > > everything biosets/mempools were being embedded in was actually being zeroed on
> > > allocation. Device mapper currently explodes, you'll probably want to apply this
> > > patch post haste.
> > > 
> > > I have now done that auditing, for every single conversion - this patch fixes
> > > everything I found. There do not seem to be any incorrect ones outside of device
> > > mapper...
> > > 
> > > We'll probably want a second patch that either a) changes
> > > bioset_init()/mempool_init() to zero the passed in bioset/mempool first, or b)
> > > my preference, WARN() or BUG() if they're passed memory that isn't zeroed.
> > 
> > Odd, haven't seen a crash, but probably requires kasan or poisoning to
> > trigger anything? Mike's tree also had the changes, since they were based
> > on the block tree.
> 
> eg. fstests/generic/081 crashes (trace below), no KASAN, PAGE_POISONING=y,
> PAGE_POISONING_NO_SANITY=y.
> 
> > I can queue this up and ship it later today. Mike, you want to review
> > this one?
> 
> Would be great to push that soon. The fstests build on several DM targets, the
> crashes lead to many test failures. I'm going to test the kzalloc fix now.

Tested-by: David Sterba <dsterba@suse.com>

on targets snapshot and thin.