linux-kernel - Re: [PATCH v5 05/12] block: Kill bi

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Aug 2012 17:21:54 -0700
From:	Kent Overstreet <koverstreet@...gle.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org,
	dm-devel@...hat.com, axboe@...nel.dk, agk@...hat.com,
	neilb@...e.de, drbd-dev@...ts.linbit.com, vgoyal@...hat.com,
	mpatocka@...hat.com, sage@...dream.net, yehuda@...newdream.net
Subject: Re: [PATCH v5 05/12] block: Kill bi_destructor

On Wed, Aug 08, 2012 at 03:22:23PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Mon, Aug 06, 2012 at 03:08:34PM -0700, Kent Overstreet wrote:
> > Now that we've got generic code for freeing bios allocated from bio
> > pools, this isn't needed anymore.
> > 
> > This also changes the semantics of bio_free() a bit - it now also frees
> > bios allocated by bio_kmalloc(). It's also no longer exported, as
> > without bi_destructor there should be no need for it to be called
> > anywhere else.
> > 
> > v5: Switch to BIO_KMALLOC_POOL ((void *)~0), per Boaz
> > 
> > Signed-off-by: Kent Overstreet <koverstreet@...gle.com>
> > ---
> > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> > index 920ede2..19bf632 100644
> > --- a/drivers/block/drbd/drbd_main.c
> > +++ b/drivers/block/drbd/drbd_main.c
> > @@ -161,23 +161,12 @@ static const struct block_device_operations drbd_ops = {
> >  	.release = drbd_release,
> >  };
> >  
> > -static void bio_destructor_drbd(struct bio *bio)
> > -{
> > -	bio_free(bio, drbd_md_io_bio_set);
> > -}
> > -
> >  struct bio *bio_alloc_drbd(gfp_t gfp_mask)
> >  {
> > -	struct bio *bio;
> > -
> >  	if (!drbd_md_io_bio_set)
> >  		return bio_alloc(gfp_mask, 1);
> >  
> > -	bio = bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> > -	if (!bio)
> > -		return NULL;
> > -	bio->bi_destructor = bio_destructor_drbd;
> > -	return bio;
> > +	return bio_alloc_bioset(gfp_mask, 1, drbd_md_io_bio_set);
> >  }
> 
> Does this chunk belong to this patch?

Hrm, that should've been in the first patch. Will move it.

> 
> > @@ -56,6 +56,8 @@ static struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly = {
> >   */
> >  struct bio_set *fs_bio_set;
> >  
> > +#define BIO_KMALLOC_POOL ((void *) ~0)
> 
> What's wrong with good ol' NULL?

If it's NULL, we can't distinguish between bios where that field wasn't
set (i.e. bios that were statically allocated somewhere) from bios that
were allocated by bio_kmalloc().

It's just there to make debugging easier - if bi_cnt goes to 0 on a bio
where it shouldn't we'll catch it at the BUG_ON() in bio_free() instead
of kfreeing a bad pointer.

> 
> Thanks.
> 
> -- 
> tejun
--
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/