[Drbd-dev] [patch] __bio_clone() behaviour

[Drbd-dev] [patch] __bio_clone() behaviour

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

With the __bio_clone() bugfix by Jens Axboe (in the recent 2.6.10-ac
kernels or SLES9 SP1) which causes __bio_clone() to copy the bi_io_vec,
it would always try to copy the _maximum_ size, as defined by
bio_src->bi_max_vecs:

inline void __bio_clone(struct bio *bio, struct bio *bio_src)
{
        request_queue_t *q = bdev_get_queue(bio_src->bi_bdev);
        memcpy(bio->bi_io_vec, bio_src->bi_io_vec, bio_src->bi_max_vecs * sizeof(struct bio_vec));
...

drbd however only has space for a single iovec (because it's all
statically allocated right now), and so the memcpy would silently
overwrite memory.

The attached patch 'fixes' this up.

Note that it is a bit ugly but safe, as drbd already asserts that
bi_vcnt == 1 anyway.

FWIW, drbd seems to be the only user of __bio_clone() I could find,
there's no in-tree users, everything goes through bio_clone() otherwise,
which would have dynamically allocated the properly sized structures.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business


[-- Attachment #2: drbd-0.7.9-2.diff --]
[-- Type: text/plain, Size: 741 bytes --]

Index: drbd_compat_wrappers.h
===================================================================
--- drbd_compat_wrappers.h	(revision 1736)
+++ drbd_compat_wrappers.h	(working copy)
@@ -538,7 +538,9 @@
 	bio_init(bio); // bio->bi_flags   = 0;
 	bio->bi_io_vec = bvec;
 	bio->bi_max_vecs = 1;
-
+	
+	/* FIXME: __bio_clone() workaround, fix me properly later! */
+	bio_src->bi_max_vecs = 1;
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
@@ -559,6 +561,8 @@
 	bio->bi_io_vec = bvec;
 	bio->bi_max_vecs = 1;
 
+	/* FIXME: __bio_clone() workaround, fix me properly later! */
+	bio_src->bi_max_vecs = 1;
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;

Re: [Drbd-dev] [patch] __bio_clone() behaviour

[Philipp Reisner]

Am Mittwoch, 26. Januar 2005 17:46 schrieb Lars Marowsky-Bree:
> With the __bio_clone() bugfix by Jens Axboe (in the recent 2.6.10-ac
> kernels or SLES9 SP1) which causes __bio_clone() to copy the bi_io_vec,
> it would always try to copy the _maximum_ size, as defined by
> bio_src->bi_max_vecs:
>
> inline void __bio_clone(struct bio *bio, struct bio *bio_src)
> {
>         request_queue_t *q = bdev_get_queue(bio_src->bi_bdev);
>         memcpy(bio->bi_io_vec, bio_src->bi_io_vec, bio_src->bi_max_vecs *
> sizeof(struct bio_vec)); ...
>
> drbd however only has space for a single iovec (because it's all
> statically allocated right now), and so the memcpy would silently
> overwrite memory.
>
> The attached patch 'fixes' this up.
>
> Note that it is a bit ugly but safe, as drbd already asserts that
> bi_vcnt == 1 anyway.
>
> FWIW, drbd seems to be the only user of __bio_clone() I could find,
> there's no in-tree users, everything goes through bio_clone() otherwise,
> which would have dynamically allocated the properly sized structures.
>

This really gives me the feeling that I should not have done the 
0.7.9 release. --- The only one to blame is myself, who thought it 
would be a nice idea to have the same release as SUSE.
Bullshit -> The next release will only happen when I am convinced
that the new release is necessary, and that it will not be a 
disaster.


BTW, regarind this patch: We now modify someone else's BIO. 
     Is this a good idea ?

-Philipp
-- 
: Dipl-Ing Philipp Reisner                      Tel +43-1-8178292-50 :
: LINBIT Information Technologies GmbH          Fax +43-1-8178292-82 :
: Schönbrunnerstr 244, 1120 Vienna, Austria    http://www.linbit.com :

Re: [Drbd-dev] [patch] __bio_clone() behaviour

[Lars Marowsky-Bree]

On 2005-01-27T09:56:16, Philipp Reisner <philipp.reisner@linbit.com> wrote:

> This really gives me the feeling that I should not have done the 
> 0.7.9 release. --- The only one to blame is myself, who thought it 
> would be a nice idea to have the same release as SUSE.
> Bullshit -> The next release will only happen when I am convinced
> that the new release is necessary, and that it will not be a 
> disaster.

Yeah, that's probably a good idea. We're used to releasing patched
versions anyway ;-)

(The 0.7.9 we had picked up had to have it's version number (and only
the version number) patched back to 0.7.5; can't update the version
number in a released product, even if the rest of the code is the same.
That's political games for you. *sigh*)

> BTW, regarind this patch: We now modify someone else's BIO. 
>      Is this a good idea ?

Well, quite frankly, it is not entirely a good idea. However, it's as
sane as the other stuff drbd does with bios. It doesn't have any other
side effect beyond making the __bio_clone() call not misbehave.

The bi_max_vecs doesn't have any other uses in that scenario and it's
safe, but not sane ;-)

I checked with axboe and grepped the kernel source, and it's the best
work around we can do right now short of converting drbd to use the
"proper" APIs. Jens thinks the __bio_clone() call probably should be
unexported; the "real" API (bio_clone()) would have automatically done
the right thing, and in fact drbd seems to be the _only_ user of
__bio_clone() anywhere. Everyone else goes via bio_alloc(), bio_clone(),
bio_get/_put() et cetera.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business