linux-kernel - Re: [RFC PATCH] bio integrity: do not assume bio_integrity

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.LRH.2.02.1507141348360.14474@file01.intranet.prod.int.rdu2.redhat.com>
Date:	Tue, 14 Jul 2015 13:48:54 -0400 (EDT)
From:	Mikulas Patocka <mpatocka@...hat.com>
To:	Mike Snitzer <snitzer@...hat.com>
cc:	"Martin K. Petersen" <martin.petersen@...cle.com>, axboe@...nel.dk,
	dm-devel@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] bio integrity: do not assume bio_integrity_pool
 exists if bioset exists



On Wed, 1 Jul 2015, Mike Snitzer wrote:

> bio_integrity_alloc() and bio_integrity_free() assume that if a bio was
> allocated from a bioset that that bioset also had its bio_integrity_pool
> allocated using bioset_integrity_create().  This is a very bad
> assumption given that bioset_create() and bioset_integrity_create() are
> completely disjoint.  Not all callers of bioset_create() have been
> trained to also call bioset_integrity_create() -- and they may not care
> to be.
> 
> Fix this by falling back to kmalloc'ing 'struct bio_integrity_payload'
> rather than force all bioset consumers to (wastefully) preallocate a
> bio_integrity_pool that they very likely won't actually need (given the
> niche nature of the current block integrity support).
> 
> Otherwise, a NULL pointer "Kernel BUG" with a trace like the following
> will be observed (as seen on s390x using zfcp storage) because dm-io
> doesn't use bioset_integrity_create() when creating its bioset:
> 
>     [  791.643338] Call Trace:
>     [  791.643339] ([<00000003df98b848>] 0x3df98b848)
>     [  791.643341]  [<00000000002c5de8>] bio_integrity_alloc+0x48/0xf8
>     [  791.643348]  [<00000000002c6486>] bio_integrity_prep+0xae/0x2f0
>     [  791.643349]  [<0000000000371e38>] blk_queue_bio+0x1c8/0x3d8
>     [  791.643355]  [<000000000036f8d0>] generic_make_request+0xc0/0x100
>     [  791.643357]  [<000000000036f9b2>] submit_bio+0xa2/0x198
>     [  791.643406]  [<000003ff801f9774>] dispatch_io+0x15c/0x3b0 [dm_mod]
>     [  791.643419]  [<000003ff801f9b3e>] dm_io+0x176/0x2f0 [dm_mod]
>     [  791.643423]  [<000003ff8074b28a>] do_reads+0x13a/0x1a8 [dm_mirror]
>     [  791.643425]  [<000003ff8074b43a>] do_mirror+0x142/0x298 [dm_mirror]
>     [  791.643428]  [<0000000000154fca>] process_one_work+0x18a/0x3f8
>     [  791.643432]  [<000000000015598a>] worker_thread+0x132/0x3b0
>     [  791.643435]  [<000000000015d49a>] kthread+0xd2/0xd8
>     [  791.643438]  [<00000000005bc0ca>] kernel_thread_starter+0x6/0xc
>     [  791.643446]  [<00000000005bc0c4>] kernel_thread_starter+0x0/0xc
> 
> Signed-off-by: Mike Snitzer <snitzer@...hat.com>
> Cc: stable@...r.kernel.org

Acked-by: Mikulas Patocka <mpatocka@...hat.com>

> ---
>  block/bio-integrity.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> NOTE: this serves as a more generic fix in the block layer rather than
> the dm-io specific fix which isn't ideal (due to potential for memory
> waste), see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=17dbe96d4f8a6f87004e6cfb5944872dfe2edb9f
> 
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 0436c21..719b715 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -51,7 +51,7 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
>  	unsigned long idx = BIO_POOL_NONE;
>  	unsigned inline_vecs;
>  
> -	if (!bs) {
> +	if (!bs || !bs->bio_integrity_pool) {
>  		bip = kmalloc(sizeof(struct bio_integrity_payload) +
>  			      sizeof(struct bio_vec) * nr_vecs, gfp_mask);
>  		inline_vecs = nr_vecs;
> @@ -104,7 +104,7 @@ void bio_integrity_free(struct bio *bio)
>  		kfree(page_address(bip->bip_vec->bv_page) +
>  		      bip->bip_vec->bv_offset);
>  
> -	if (bs) {
> +	if (bs && bs->bio_integrity_pool) {
>  		if (bip->bip_slab != BIO_POOL_NONE)
>  			bvec_free(bs->bvec_integrity_pool, bip->bip_vec,
>  				  bip->bip_slab);
> -- 
> 2.3.2 (Apple Git-55)
> 
> 
--
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/