Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
> "Kent" == Kent Overstreet writes: >> > + *bp->bip1.bip_vec = bip->bip_vec[0]; >> > + *bp->bip2.bip_vec = bip->bip_vec[0]; >> >> I think this is horrible. Yep. >> Why not introduce bvec pointer in bip (like bio), to cover the case >> when bvec are not inline. Kent> That's... exactly what the next patch in the series does. I'm perfectly ok with a patch that introduces the pointer and fixes the bio_pair case. As long as that's all it does. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 05:58:45PM -0400, Vivek Goyal wrote: > On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote: > > I'm honestly not sure what your complaint about my bugfix patch was - > > it's small and complete, it does fix the bug. I don't follow why you > > think we need to introduce the bip->bio_vec pointer early... > > I think having iv1 and iv2 and then not even accessing these using > bp->iv1 and bp->iv2 is a bad idea even for bugfix. > > I have never seen a code which says, hey I have defined two fields in a > struct but, don't access those fields directly(as there might be padding > issues). These fields are just there for blocking a chunk of memory but are > never meant to be accessed directly. I think, that's what my issue is. It > is bad programming (does not matter whether it is bug fix or not). > > For your series it probably is still fine as you will overide it pretty > soon but what about stable. Anybody looking at that code might want > to say, hey why not directly initialize bp->iv1 instead of trying to > do *bp->bip1.bip_vec. And everybody will say, yes looks fine and boom > a bug is introduced because we did bad programming. Ok. It's definitely a bit weird and unusual, and if I wasn't getting rid of it in the next patch it would definitely merit a comment. For stable... wtf would they be making that kind of change for, and without reading the relevant code? Eh, maybe I will stick in that comment and take it out in the next patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote: > On Tue, Oct 02, 2012 at 04:32:53PM -0400, Vivek Goyal wrote: > > On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: > > > On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: > > > > On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: > > > > > > > > [..] > > > > > Here's the new patch: > > > > > > > > > > > > > > > commit e270c9ca843b5c86d59431b0d7a676b7846946d6 > > > > > Author: Kent Overstreet > > > > > Date: Mon Oct 1 14:41:08 2012 -0700 > > > > > > > > > > block: Fix a buffer overrun in bio_integrity_split() > > > > > > > > > > bio_integrity_split() seemed to be confusing pointers and arrays - > > > > > bip_vec in bio_integrity_payload is an array appended to the end > > > > > of the > > > > > payload, so the bio_vecs in struct bio_pair need to come > > > > > immediately > > > > > after the bio_integrity_payload they're for, and there was an > > > > > assignment > > > > > in bio_integrity_split() that didn't make any sense. > > > > > > > > > > Also, changed bio_integrity_split() to not refer to the bvecs > > > > > embedded > > > > > in struct bio_pair, in case there's padding between them and > > > > > bip->bip_vec. > > > > > > > > > > Signed-off-by: Kent Overstreet > > > > > CC: Jens Axboe > > > > > CC: Martin K. Petersen > > > > > > > > > > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c > > > > > index a3f28f3..4ae22a8 100644 > > > > > --- a/fs/bio-integrity.c > > > > > +++ b/fs/bio-integrity.c > > > > > @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, > > > > > struct bio_pair *bp, int sectors) > > > > > bp->bio1.bi_integrity = >bip1; > > > > > bp->bio2.bi_integrity = >bip2; > > > > > > > > > > - bp->iv1 = bip->bip_vec[0]; > > > > > - bp->iv2 = bip->bip_vec[0]; > > > > > + *bp->bip1.bip_vec = bip->bip_vec[0]; > > > > > + *bp->bip2.bip_vec = bip->bip_vec[0]; > > > > > > > > I think this is horrible. Why not introduce bvec pointer in bip (like > > > > bio), > > > > to cover the case when bvec are not inline. > > > > > > That's... exactly what the next patch in the series does. > > > > Yes, but if you want to push some of the these bug fixes in stable (as > > martin > > had said), we need to introduce that bip->bio_vec pointer early. Also that > > next patch is doing lot other other things like getting rid of bip_slabs > > and we don't require all that to fix this particular bug. > > > > In fact I would say that it is beter to fix this blk integrity bug in a > > separate patchset so that it can be pushed out earlier. > > I'm honestly not sure what your complaint about my bugfix patch was - > it's small and complete, it does fix the bug. I don't follow why you > think we need to introduce the bip->bio_vec pointer early... I think having iv1 and iv2 and then not even accessing these using bp->iv1 and bp->iv2 is a bad idea even for bugfix. I have never seen a code which says, hey I have defined two fields in a struct but, don't access those fields directly(as there might be padding issues). These fields are just there for blocking a chunk of memory but are never meant to be accessed directly. I think, that's what my issue is. It is bad programming (does not matter whether it is bug fix or not). For your series it probably is still fine as you will overide it pretty soon but what about stable. Anybody looking at that code might want to say, hey why not directly initialize bp->iv1 instead of trying to do *bp->bip1.bip_vec. And everybody will say, yes looks fine and boom a bug is introduced because we did bad programming. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 04:32:53PM -0400, Vivek Goyal wrote: > On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: > > On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: > > > On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: > > > > > > [..] > > > > Here's the new patch: > > > > > > > > > > > > commit e270c9ca843b5c86d59431b0d7a676b7846946d6 > > > > Author: Kent Overstreet > > > > Date: Mon Oct 1 14:41:08 2012 -0700 > > > > > > > > block: Fix a buffer overrun in bio_integrity_split() > > > > > > > > bio_integrity_split() seemed to be confusing pointers and arrays - > > > > bip_vec in bio_integrity_payload is an array appended to the end of > > > > the > > > > payload, so the bio_vecs in struct bio_pair need to come immediately > > > > after the bio_integrity_payload they're for, and there was an > > > > assignment > > > > in bio_integrity_split() that didn't make any sense. > > > > > > > > Also, changed bio_integrity_split() to not refer to the bvecs > > > > embedded > > > > in struct bio_pair, in case there's padding between them and > > > > bip->bip_vec. > > > > > > > > Signed-off-by: Kent Overstreet > > > > CC: Jens Axboe > > > > CC: Martin K. Petersen > > > > > > > > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c > > > > index a3f28f3..4ae22a8 100644 > > > > --- a/fs/bio-integrity.c > > > > +++ b/fs/bio-integrity.c > > > > @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct > > > > bio_pair *bp, int sectors) > > > > bp->bio1.bi_integrity = >bip1; > > > > bp->bio2.bi_integrity = >bip2; > > > > > > > > - bp->iv1 = bip->bip_vec[0]; > > > > - bp->iv2 = bip->bip_vec[0]; > > > > + *bp->bip1.bip_vec = bip->bip_vec[0]; > > > > + *bp->bip2.bip_vec = bip->bip_vec[0]; > > > > > > I think this is horrible. Why not introduce bvec pointer in bip (like > > > bio), > > > to cover the case when bvec are not inline. > > > > That's... exactly what the next patch in the series does. > > Yes, but if you want to push some of the these bug fixes in stable (as martin > had said), we need to introduce that bip->bio_vec pointer early. Also that > next patch is doing lot other other things like getting rid of bip_slabs > and we don't require all that to fix this particular bug. > > In fact I would say that it is beter to fix this blk integrity bug in a > separate patchset so that it can be pushed out earlier. I'm honestly not sure what your complaint about my bugfix patch was - it's small and complete, it does fix the bug. I don't follow why you think we need to introduce the bip->bio_vec pointer early... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: > On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: > > On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: > > > > [..] > > > Here's the new patch: > > > > > > > > > commit e270c9ca843b5c86d59431b0d7a676b7846946d6 > > > Author: Kent Overstreet > > > Date: Mon Oct 1 14:41:08 2012 -0700 > > > > > > block: Fix a buffer overrun in bio_integrity_split() > > > > > > bio_integrity_split() seemed to be confusing pointers and arrays - > > > bip_vec in bio_integrity_payload is an array appended to the end of > > > the > > > payload, so the bio_vecs in struct bio_pair need to come immediately > > > after the bio_integrity_payload they're for, and there was an > > > assignment > > > in bio_integrity_split() that didn't make any sense. > > > > > > Also, changed bio_integrity_split() to not refer to the bvecs embedded > > > in struct bio_pair, in case there's padding between them and > > > bip->bip_vec. > > > > > > Signed-off-by: Kent Overstreet > > > CC: Jens Axboe > > > CC: Martin K. Petersen > > > > > > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c > > > index a3f28f3..4ae22a8 100644 > > > --- a/fs/bio-integrity.c > > > +++ b/fs/bio-integrity.c > > > @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct > > > bio_pair *bp, int sectors) > > > bp->bio1.bi_integrity = >bip1; > > > bp->bio2.bi_integrity = >bip2; > > > > > > - bp->iv1 = bip->bip_vec[0]; > > > - bp->iv2 = bip->bip_vec[0]; > > > + *bp->bip1.bip_vec = bip->bip_vec[0]; > > > + *bp->bip2.bip_vec = bip->bip_vec[0]; > > > > I think this is horrible. Why not introduce bvec pointer in bip (like bio), > > to cover the case when bvec are not inline. > > That's... exactly what the next patch in the series does. Yes, but if you want to push some of the these bug fixes in stable (as martin had said), we need to introduce that bip->bio_vec pointer early. Also that next patch is doing lot other other things like getting rid of bip_slabs and we don't require all that to fix this particular bug. In fact I would say that it is beter to fix this blk integrity bug in a separate patchset so that it can be pushed out earlier. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: > On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: > > [..] > > Here's the new patch: > > > > > > commit e270c9ca843b5c86d59431b0d7a676b7846946d6 > > Author: Kent Overstreet > > Date: Mon Oct 1 14:41:08 2012 -0700 > > > > block: Fix a buffer overrun in bio_integrity_split() > > > > bio_integrity_split() seemed to be confusing pointers and arrays - > > bip_vec in bio_integrity_payload is an array appended to the end of the > > payload, so the bio_vecs in struct bio_pair need to come immediately > > after the bio_integrity_payload they're for, and there was an assignment > > in bio_integrity_split() that didn't make any sense. > > > > Also, changed bio_integrity_split() to not refer to the bvecs embedded > > in struct bio_pair, in case there's padding between them and > > bip->bip_vec. > > > > Signed-off-by: Kent Overstreet > > CC: Jens Axboe > > CC: Martin K. Petersen > > > > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c > > index a3f28f3..4ae22a8 100644 > > --- a/fs/bio-integrity.c > > +++ b/fs/bio-integrity.c > > @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct > > bio_pair *bp, int sectors) > > bp->bio1.bi_integrity = >bip1; > > bp->bio2.bi_integrity = >bip2; > > > > - bp->iv1 = bip->bip_vec[0]; > > - bp->iv2 = bip->bip_vec[0]; > > + *bp->bip1.bip_vec = bip->bip_vec[0]; > > + *bp->bip2.bip_vec = bip->bip_vec[0]; > > I think this is horrible. Why not introduce bvec pointer in bip (like bio), > to cover the case when bvec are not inline. That's... exactly what the next patch in the series does. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] > Here's the new patch: > > > commit e270c9ca843b5c86d59431b0d7a676b7846946d6 > Author: Kent Overstreet > Date: Mon Oct 1 14:41:08 2012 -0700 > > block: Fix a buffer overrun in bio_integrity_split() > > bio_integrity_split() seemed to be confusing pointers and arrays - > bip_vec in bio_integrity_payload is an array appended to the end of the > payload, so the bio_vecs in struct bio_pair need to come immediately > after the bio_integrity_payload they're for, and there was an assignment > in bio_integrity_split() that didn't make any sense. > > Also, changed bio_integrity_split() to not refer to the bvecs embedded > in struct bio_pair, in case there's padding between them and > bip->bip_vec. > > Signed-off-by: Kent Overstreet > CC: Jens Axboe > CC: Martin K. Petersen > > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c > index a3f28f3..4ae22a8 100644 > --- a/fs/bio-integrity.c > +++ b/fs/bio-integrity.c > @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct > bio_pair *bp, int sectors) > bp->bio1.bi_integrity = >bip1; > bp->bio2.bi_integrity = >bip2; > > - bp->iv1 = bip->bip_vec[0]; > - bp->iv2 = bip->bip_vec[0]; > + *bp->bip1.bip_vec = bip->bip_vec[0]; > + *bp->bip2.bip_vec = bip->bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] Here's the new patch: commit e270c9ca843b5c86d59431b0d7a676b7846946d6 Author: Kent Overstreet koverstr...@google.com Date: Mon Oct 1 14:41:08 2012 -0700 block: Fix a buffer overrun in bio_integrity_split() bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload is an array appended to the end of the payload, so the bio_vecs in struct bio_pair need to come immediately after the bio_integrity_payload they're for, and there was an assignment in bio_integrity_split() that didn't make any sense. Also, changed bio_integrity_split() to not refer to the bvecs embedded in struct bio_pair, in case there's padding between them and bip-bip_vec. Signed-off-by: Kent Overstreet koverstr...@google.com CC: Jens Axboe ax...@kernel.dk CC: Martin K. Petersen martin.peter...@oracle.com diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..4ae22a8 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp-bio1.bi_integrity = bp-bip1; bp-bio2.bi_integrity = bp-bip2; - bp-iv1 = bip-bip_vec[0]; - bp-iv2 = bip-bip_vec[0]; + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] Here's the new patch: commit e270c9ca843b5c86d59431b0d7a676b7846946d6 Author: Kent Overstreet koverstr...@google.com Date: Mon Oct 1 14:41:08 2012 -0700 block: Fix a buffer overrun in bio_integrity_split() bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload is an array appended to the end of the payload, so the bio_vecs in struct bio_pair need to come immediately after the bio_integrity_payload they're for, and there was an assignment in bio_integrity_split() that didn't make any sense. Also, changed bio_integrity_split() to not refer to the bvecs embedded in struct bio_pair, in case there's padding between them and bip-bip_vec. Signed-off-by: Kent Overstreet koverstr...@google.com CC: Jens Axboe ax...@kernel.dk CC: Martin K. Petersen martin.peter...@oracle.com diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..4ae22a8 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp-bio1.bi_integrity = bp-bip1; bp-bio2.bi_integrity = bp-bip2; - bp-iv1 = bip-bip_vec[0]; - bp-iv2 = bip-bip_vec[0]; + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. That's... exactly what the next patch in the series does. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] Here's the new patch: commit e270c9ca843b5c86d59431b0d7a676b7846946d6 Author: Kent Overstreet koverstr...@google.com Date: Mon Oct 1 14:41:08 2012 -0700 block: Fix a buffer overrun in bio_integrity_split() bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload is an array appended to the end of the payload, so the bio_vecs in struct bio_pair need to come immediately after the bio_integrity_payload they're for, and there was an assignment in bio_integrity_split() that didn't make any sense. Also, changed bio_integrity_split() to not refer to the bvecs embedded in struct bio_pair, in case there's padding between them and bip-bip_vec. Signed-off-by: Kent Overstreet koverstr...@google.com CC: Jens Axboe ax...@kernel.dk CC: Martin K. Petersen martin.peter...@oracle.com diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..4ae22a8 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp-bio1.bi_integrity = bp-bip1; bp-bio2.bi_integrity = bp-bip2; - bp-iv1 = bip-bip_vec[0]; - bp-iv2 = bip-bip_vec[0]; + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. That's... exactly what the next patch in the series does. Yes, but if you want to push some of the these bug fixes in stable (as martin had said), we need to introduce that bip-bio_vec pointer early. Also that next patch is doing lot other other things like getting rid of bip_slabs and we don't require all that to fix this particular bug. In fact I would say that it is beter to fix this blk integrity bug in a separate patchset so that it can be pushed out earlier. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 04:32:53PM -0400, Vivek Goyal wrote: On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] Here's the new patch: commit e270c9ca843b5c86d59431b0d7a676b7846946d6 Author: Kent Overstreet koverstr...@google.com Date: Mon Oct 1 14:41:08 2012 -0700 block: Fix a buffer overrun in bio_integrity_split() bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload is an array appended to the end of the payload, so the bio_vecs in struct bio_pair need to come immediately after the bio_integrity_payload they're for, and there was an assignment in bio_integrity_split() that didn't make any sense. Also, changed bio_integrity_split() to not refer to the bvecs embedded in struct bio_pair, in case there's padding between them and bip-bip_vec. Signed-off-by: Kent Overstreet koverstr...@google.com CC: Jens Axboe ax...@kernel.dk CC: Martin K. Petersen martin.peter...@oracle.com diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..4ae22a8 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp-bio1.bi_integrity = bp-bip1; bp-bio2.bi_integrity = bp-bip2; - bp-iv1 = bip-bip_vec[0]; - bp-iv2 = bip-bip_vec[0]; + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. That's... exactly what the next patch in the series does. Yes, but if you want to push some of the these bug fixes in stable (as martin had said), we need to introduce that bip-bio_vec pointer early. Also that next patch is doing lot other other things like getting rid of bip_slabs and we don't require all that to fix this particular bug. In fact I would say that it is beter to fix this blk integrity bug in a separate patchset so that it can be pushed out earlier. I'm honestly not sure what your complaint about my bugfix patch was - it's small and complete, it does fix the bug. I don't follow why you think we need to introduce the bip-bio_vec pointer early... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote: On Tue, Oct 02, 2012 at 04:32:53PM -0400, Vivek Goyal wrote: On Tue, Oct 02, 2012 at 01:26:43PM -0700, Kent Overstreet wrote: On Tue, Oct 02, 2012 at 10:08:47AM -0400, Vivek Goyal wrote: On Mon, Oct 01, 2012 at 02:42:41PM -0700, Kent Overstreet wrote: [..] Here's the new patch: commit e270c9ca843b5c86d59431b0d7a676b7846946d6 Author: Kent Overstreet koverstr...@google.com Date: Mon Oct 1 14:41:08 2012 -0700 block: Fix a buffer overrun in bio_integrity_split() bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload is an array appended to the end of the payload, so the bio_vecs in struct bio_pair need to come immediately after the bio_integrity_payload they're for, and there was an assignment in bio_integrity_split() that didn't make any sense. Also, changed bio_integrity_split() to not refer to the bvecs embedded in struct bio_pair, in case there's padding between them and bip-bip_vec. Signed-off-by: Kent Overstreet koverstr...@google.com CC: Jens Axboe ax...@kernel.dk CC: Martin K. Petersen martin.peter...@oracle.com diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..4ae22a8 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp-bio1.bi_integrity = bp-bip1; bp-bio2.bi_integrity = bp-bip2; - bp-iv1 = bip-bip_vec[0]; - bp-iv2 = bip-bip_vec[0]; + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. That's... exactly what the next patch in the series does. Yes, but if you want to push some of the these bug fixes in stable (as martin had said), we need to introduce that bip-bio_vec pointer early. Also that next patch is doing lot other other things like getting rid of bip_slabs and we don't require all that to fix this particular bug. In fact I would say that it is beter to fix this blk integrity bug in a separate patchset so that it can be pushed out earlier. I'm honestly not sure what your complaint about my bugfix patch was - it's small and complete, it does fix the bug. I don't follow why you think we need to introduce the bip-bio_vec pointer early... I think having iv1 and iv2 and then not even accessing these using bp-iv1 and bp-iv2 is a bad idea even for bugfix. I have never seen a code which says, hey I have defined two fields in a struct but, don't access those fields directly(as there might be padding issues). These fields are just there for blocking a chunk of memory but are never meant to be accessed directly. I think, that's what my issue is. It is bad programming (does not matter whether it is bug fix or not). For your series it probably is still fine as you will overide it pretty soon but what about stable. Anybody looking at that code might want to say, hey why not directly initialize bp-iv1 instead of trying to do *bp-bip1.bip_vec. And everybody will say, yes looks fine and boom a bug is introduced because we did bad programming. Thanks Vivek -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Tue, Oct 02, 2012 at 05:58:45PM -0400, Vivek Goyal wrote: On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote: I'm honestly not sure what your complaint about my bugfix patch was - it's small and complete, it does fix the bug. I don't follow why you think we need to introduce the bip-bio_vec pointer early... I think having iv1 and iv2 and then not even accessing these using bp-iv1 and bp-iv2 is a bad idea even for bugfix. I have never seen a code which says, hey I have defined two fields in a struct but, don't access those fields directly(as there might be padding issues). These fields are just there for blocking a chunk of memory but are never meant to be accessed directly. I think, that's what my issue is. It is bad programming (does not matter whether it is bug fix or not). For your series it probably is still fine as you will overide it pretty soon but what about stable. Anybody looking at that code might want to say, hey why not directly initialize bp-iv1 instead of trying to do *bp-bip1.bip_vec. And everybody will say, yes looks fine and boom a bug is introduced because we did bad programming. Ok. It's definitely a bit weird and unusual, and if I wasn't getting rid of it in the next patch it would definitely merit a comment. For stable... wtf would they be making that kind of change for, and without reading the relevant code? Eh, maybe I will stick in that comment and take it out in the next patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
Kent == Kent Overstreet koverstr...@google.com writes: + *bp-bip1.bip_vec = bip-bip_vec[0]; + *bp-bip2.bip_vec = bip-bip_vec[0]; I think this is horrible. Yep. Why not introduce bvec pointer in bip (like bio), to cover the case when bvec are not inline. Kent That's... exactly what the next patch in the series does. I'm perfectly ok with a patch that introduces the pointer and fixes the bio_pair case. As long as that's all it does. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 05:23:36PM -0400, Vivek Goyal wrote:
> On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
> > bio_integrity_split() seemed to be confusing pointers and arrays -
> > bip_vec in bio_integrity_payload is an array appended to the end of the
> > payload, so the bio_vecs in struct bio_pair need to come immediately
> > after the bio_integrity_payload they're for, and there was an assignment
> > in bio_integrity_split() that didn't make any sense.
> >
> > Signed-off-by: Kent Overstreet
> > CC: Jens Axboe
> > CC: Martin K. Petersen
> > ---
> > fs/bio-integrity.c | 3 ---
> > include/linux/bio.h | 6 --
> > 2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> > index a3f28f3..c7b6b52 100644
> > --- a/fs/bio-integrity.c
> > +++ b/fs/bio-integrity.c
> > @@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct
> > bio_pair *bp, int sectors)
> > bp->iv1 = bip->bip_vec[0];
> > bp->iv2 = bip->bip_vec[0];
> >
> > - bp->bip1.bip_vec[0] = bp->iv1;
> > - bp->bip2.bip_vec[0] = bp->iv2;
> > -
> > bp->iv1.bv_len = sectors * bi->tuple_size;
> > bp->iv2.bv_offset += sectors * bi->tuple_size;
> > bp->iv2.bv_len -= sectors * bi->tuple_size;
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index b31036f..8e2d108 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -200,8 +200,10 @@ struct bio_pair {
> > struct bio bio1, bio2;
> > struct bio_vec bv1, bv2;
> > #if defined(CONFIG_BLK_DEV_INTEGRITY)
> > - struct bio_integrity_payloadbip1, bip2;
> > - struct bio_vec iv1, iv2;
> > + struct bio_integrity_payloadbip1;
> > + struct bio_vec iv1;
> > + struct bio_integrity_payloadbip2;
> > + struct bio_vec iv2;
> > #endif
>
> I think it probably is a good idea to put a comment here so that we
> know that certain elements of structure assume ordering.
>
> Also I am wondering that what's the gurantee that there are no padding
> bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
> bytes then the assumption that bio_vec is always following bip will be
> broken?
Here's the new patch:
commit e270c9ca843b5c86d59431b0d7a676b7846946d6
Author: Kent Overstreet
Date: Mon Oct 1 14:41:08 2012 -0700
block: Fix a buffer overrun in bio_integrity_split()
bio_integrity_split() seemed to be confusing pointers and arrays -
bip_vec in bio_integrity_payload is an array appended to the end of the
payload, so the bio_vecs in struct bio_pair need to come immediately
after the bio_integrity_payload they're for, and there was an assignment
in bio_integrity_split() that didn't make any sense.
Also, changed bio_integrity_split() to not refer to the bvecs embedded
in struct bio_pair, in case there's padding between them and
bip->bip_vec.
Signed-off-by: Kent Overstreet
CC: Jens Axboe
CC: Martin K. Petersen
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index a3f28f3..4ae22a8 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair
*bp, int sectors)
bp->bio1.bi_integrity = >bip1;
bp->bio2.bi_integrity = >bip2;
- bp->iv1 = bip->bip_vec[0];
- bp->iv2 = bip->bip_vec[0];
+ *bp->bip1.bip_vec = bip->bip_vec[0];
+ *bp->bip2.bip_vec = bip->bip_vec[0];
- bp->bip1.bip_vec[0] = bp->iv1;
- bp->bip2.bip_vec[0] = bp->iv2;
-
- bp->iv1.bv_len = sectors * bi->tuple_size;
- bp->iv2.bv_offset += sectors * bi->tuple_size;
- bp->iv2.bv_len -= sectors * bi->tuple_size;
+ bp->bip1.bip_vec->bv_len = sectors * bi->tuple_size;
+ bp->bip2.bip_vec->bv_offset += sectors * bi->tuple_size;
+ bp->bip2.bip_vec->bv_len -= sectors * bi->tuple_size;
bp->bip1.bip_sector = bio->bi_integrity->bip_sector;
bp->bip2.bip_sector = bio->bi_integrity->bip_sector + nr_sectors;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b31036f..8e2d108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -200,8 +200,10 @@ struct bio_pair {
struct bio bio1, bio2;
struct bio_vec bv1, bv2;
#if defined(CONFIG_BLK_DEV_INTEGRITY)
- struct bio_integrity_payloadbip1, bip2;
- struct bio_vec iv1, iv2;
+ struct bio_integrity_payloadbip1;
+ struct bio_vec iv1;
+ struct bio_integrity_payloadbip2;
+ struct bio_vec iv2;
#endif
atomic_tcnt;
int error;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 05:23:36PM -0400, Vivek Goyal wrote:
> On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
> > bio_integrity_split() seemed to be confusing pointers and arrays -
> > bip_vec in bio_integrity_payload is an array appended to the end of the
> > payload, so the bio_vecs in struct bio_pair need to come immediately
> > after the bio_integrity_payload they're for, and there was an assignment
> > in bio_integrity_split() that didn't make any sense.
> >
> > Signed-off-by: Kent Overstreet
> > CC: Jens Axboe
> > CC: Martin K. Petersen
> > ---
> > fs/bio-integrity.c | 3 ---
> > include/linux/bio.h | 6 --
> > 2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> > index a3f28f3..c7b6b52 100644
> > --- a/fs/bio-integrity.c
> > +++ b/fs/bio-integrity.c
> > @@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct
> > bio_pair *bp, int sectors)
> > bp->iv1 = bip->bip_vec[0];
> > bp->iv2 = bip->bip_vec[0];
> >
> > - bp->bip1.bip_vec[0] = bp->iv1;
> > - bp->bip2.bip_vec[0] = bp->iv2;
> > -
> > bp->iv1.bv_len = sectors * bi->tuple_size;
> > bp->iv2.bv_offset += sectors * bi->tuple_size;
> > bp->iv2.bv_len -= sectors * bi->tuple_size;
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index b31036f..8e2d108 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -200,8 +200,10 @@ struct bio_pair {
> > struct bio bio1, bio2;
> > struct bio_vec bv1, bv2;
> > #if defined(CONFIG_BLK_DEV_INTEGRITY)
> > - struct bio_integrity_payloadbip1, bip2;
> > - struct bio_vec iv1, iv2;
> > + struct bio_integrity_payloadbip1;
> > + struct bio_vec iv1;
> > + struct bio_integrity_payloadbip2;
> > + struct bio_vec iv2;
> > #endif
>
> I think it probably is a good idea to put a comment here so that we
> know that certain elements of structure assume ordering.
I'd agree, but I am getting rid of that requirement in the next patch...
> Also I am wondering that what's the gurantee that there are no padding
> bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
> bytes then the assumption that bio_vec is always following bip will be
> broken?
Feh, that is an issue. It wouldn't be an issue if we never referred to
the embedded bvecs - and only referred to bip->bip_inline_vecs - but we
don't.
I'll have to fix that.
> Also had a general question about split logic. We seem to have only one
> global pool for bio pair (bio_split_pool). So in the IO stack if we split
> a bio more than once, we have the deadlock possibility again?
Yes.
I have a fix for that in my patch queue. There's no trivial fix because
the current bio_split implementation requires its own mempool - either
we'd have to add that mempool to struct bio_set (ew, no) or we'd have to
have all the callers also allocate their own bio_pairi mempool.
My approach gets rid of the need for the bio_pair mempool by adding
generic bio chaining, which requires adding a single refcount to struct
bio - bi_remaining, and bio_endio() does an atomic_dec_and_test() on
that refcount. Chaining is also done with a flag indicating that
bi_private points to a bio, instead of a bio_chain_endio function.
A bio_chain_endio() function would be cleaner, but the problem is with
arbitrary and unlimited bio splitting, completing a bio can complete an
unlimited number of splits and use an unbounded amount of stack. (tail
call optimization would be another way of solving that, but building
with frame pointers also disables sibling call optimization so we can't
depend on that).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
> bio_integrity_split() seemed to be confusing pointers and arrays -
> bip_vec in bio_integrity_payload is an array appended to the end of the
> payload, so the bio_vecs in struct bio_pair need to come immediately
> after the bio_integrity_payload they're for, and there was an assignment
> in bio_integrity_split() that didn't make any sense.
>
> Signed-off-by: Kent Overstreet
> CC: Jens Axboe
> CC: Martin K. Petersen
> ---
> fs/bio-integrity.c | 3 ---
> include/linux/bio.h | 6 --
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> index a3f28f3..c7b6b52 100644
> --- a/fs/bio-integrity.c
> +++ b/fs/bio-integrity.c
> @@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct bio_pair
> *bp, int sectors)
> bp->iv1 = bip->bip_vec[0];
> bp->iv2 = bip->bip_vec[0];
>
> - bp->bip1.bip_vec[0] = bp->iv1;
> - bp->bip2.bip_vec[0] = bp->iv2;
> -
> bp->iv1.bv_len = sectors * bi->tuple_size;
> bp->iv2.bv_offset += sectors * bi->tuple_size;
> bp->iv2.bv_len -= sectors * bi->tuple_size;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index b31036f..8e2d108 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -200,8 +200,10 @@ struct bio_pair {
> struct bio bio1, bio2;
> struct bio_vec bv1, bv2;
> #if defined(CONFIG_BLK_DEV_INTEGRITY)
> - struct bio_integrity_payloadbip1, bip2;
> - struct bio_vec iv1, iv2;
> + struct bio_integrity_payloadbip1;
> + struct bio_vec iv1;
> + struct bio_integrity_payloadbip2;
> + struct bio_vec iv2;
> #endif
I think it probably is a good idea to put a comment here so that we
know that certain elements of structure assume ordering.
Also I am wondering that what's the gurantee that there are no padding
bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
bytes then the assumption that bio_vec is always following bip will be
broken?
Also had a general question about split logic. We seem to have only one
global pool for bio pair (bio_split_pool). So in the IO stack if we split
a bio more than once, we have the deadlock possibility again?
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
bio_integrity_split() seemed to be confusing pointers and arrays -
bip_vec in bio_integrity_payload is an array appended to the end of the
payload, so the bio_vecs in struct bio_pair need to come immediately
after the bio_integrity_payload they're for, and there was an assignment
in bio_integrity_split() that didn't make any sense.
Signed-off-by: Kent Overstreet koverstr...@google.com
CC: Jens Axboe ax...@kernel.dk
CC: Martin K. Petersen martin.peter...@oracle.com
---
fs/bio-integrity.c | 3 ---
include/linux/bio.h | 6 --
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index a3f28f3..c7b6b52 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct bio_pair
*bp, int sectors)
bp-iv1 = bip-bip_vec[0];
bp-iv2 = bip-bip_vec[0];
- bp-bip1.bip_vec[0] = bp-iv1;
- bp-bip2.bip_vec[0] = bp-iv2;
-
bp-iv1.bv_len = sectors * bi-tuple_size;
bp-iv2.bv_offset += sectors * bi-tuple_size;
bp-iv2.bv_len -= sectors * bi-tuple_size;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b31036f..8e2d108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -200,8 +200,10 @@ struct bio_pair {
struct bio bio1, bio2;
struct bio_vec bv1, bv2;
#if defined(CONFIG_BLK_DEV_INTEGRITY)
- struct bio_integrity_payloadbip1, bip2;
- struct bio_vec iv1, iv2;
+ struct bio_integrity_payloadbip1;
+ struct bio_vec iv1;
+ struct bio_integrity_payloadbip2;
+ struct bio_vec iv2;
#endif
I think it probably is a good idea to put a comment here so that we
know that certain elements of structure assume ordering.
Also I am wondering that what's the gurantee that there are no padding
bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
bytes then the assumption that bio_vec is always following bip will be
broken?
Also had a general question about split logic. We seem to have only one
global pool for bio pair (bio_split_pool). So in the IO stack if we split
a bio more than once, we have the deadlock possibility again?
Thanks
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 05:23:36PM -0400, Vivek Goyal wrote:
On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
bio_integrity_split() seemed to be confusing pointers and arrays -
bip_vec in bio_integrity_payload is an array appended to the end of the
payload, so the bio_vecs in struct bio_pair need to come immediately
after the bio_integrity_payload they're for, and there was an assignment
in bio_integrity_split() that didn't make any sense.
Signed-off-by: Kent Overstreet koverstr...@google.com
CC: Jens Axboe ax...@kernel.dk
CC: Martin K. Petersen martin.peter...@oracle.com
---
fs/bio-integrity.c | 3 ---
include/linux/bio.h | 6 --
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index a3f28f3..c7b6b52 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct
bio_pair *bp, int sectors)
bp-iv1 = bip-bip_vec[0];
bp-iv2 = bip-bip_vec[0];
- bp-bip1.bip_vec[0] = bp-iv1;
- bp-bip2.bip_vec[0] = bp-iv2;
-
bp-iv1.bv_len = sectors * bi-tuple_size;
bp-iv2.bv_offset += sectors * bi-tuple_size;
bp-iv2.bv_len -= sectors * bi-tuple_size;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b31036f..8e2d108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -200,8 +200,10 @@ struct bio_pair {
struct bio bio1, bio2;
struct bio_vec bv1, bv2;
#if defined(CONFIG_BLK_DEV_INTEGRITY)
- struct bio_integrity_payloadbip1, bip2;
- struct bio_vec iv1, iv2;
+ struct bio_integrity_payloadbip1;
+ struct bio_vec iv1;
+ struct bio_integrity_payloadbip2;
+ struct bio_vec iv2;
#endif
I think it probably is a good idea to put a comment here so that we
know that certain elements of structure assume ordering.
I'd agree, but I am getting rid of that requirement in the next patch...
Also I am wondering that what's the gurantee that there are no padding
bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
bytes then the assumption that bio_vec is always following bip will be
broken?
Feh, that is an issue. It wouldn't be an issue if we never referred to
the embedded bvecs - and only referred to bip-bip_inline_vecs - but we
don't.
I'll have to fix that.
Also had a general question about split logic. We seem to have only one
global pool for bio pair (bio_split_pool). So in the IO stack if we split
a bio more than once, we have the deadlock possibility again?
Yes.
I have a fix for that in my patch queue. There's no trivial fix because
the current bio_split implementation requires its own mempool - either
we'd have to add that mempool to struct bio_set (ew, no) or we'd have to
have all the callers also allocate their own bio_pairi mempool.
My approach gets rid of the need for the bio_pair mempool by adding
generic bio chaining, which requires adding a single refcount to struct
bio - bi_remaining, and bio_endio() does an atomic_dec_and_test() on
that refcount. Chaining is also done with a flag indicating that
bi_private points to a bio, instead of a bio_chain_endio function.
A bio_chain_endio() function would be cleaner, but the problem is with
arbitrary and unlimited bio splitting, completing a bio can complete an
unlimited number of splits and use an unbounded amount of stack. (tail
call optimization would be another way of solving that, but building
with frame pointers also disables sibling call optimization so we can't
depend on that).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
On Mon, Oct 01, 2012 at 05:23:36PM -0400, Vivek Goyal wrote:
On Mon, Sep 24, 2012 at 03:34:41PM -0700, Kent Overstreet wrote:
bio_integrity_split() seemed to be confusing pointers and arrays -
bip_vec in bio_integrity_payload is an array appended to the end of the
payload, so the bio_vecs in struct bio_pair need to come immediately
after the bio_integrity_payload they're for, and there was an assignment
in bio_integrity_split() that didn't make any sense.
Signed-off-by: Kent Overstreet koverstr...@google.com
CC: Jens Axboe ax...@kernel.dk
CC: Martin K. Petersen martin.peter...@oracle.com
---
fs/bio-integrity.c | 3 ---
include/linux/bio.h | 6 --
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index a3f28f3..c7b6b52 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -697,9 +697,6 @@ void bio_integrity_split(struct bio *bio, struct
bio_pair *bp, int sectors)
bp-iv1 = bip-bip_vec[0];
bp-iv2 = bip-bip_vec[0];
- bp-bip1.bip_vec[0] = bp-iv1;
- bp-bip2.bip_vec[0] = bp-iv2;
-
bp-iv1.bv_len = sectors * bi-tuple_size;
bp-iv2.bv_offset += sectors * bi-tuple_size;
bp-iv2.bv_len -= sectors * bi-tuple_size;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b31036f..8e2d108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -200,8 +200,10 @@ struct bio_pair {
struct bio bio1, bio2;
struct bio_vec bv1, bv2;
#if defined(CONFIG_BLK_DEV_INTEGRITY)
- struct bio_integrity_payloadbip1, bip2;
- struct bio_vec iv1, iv2;
+ struct bio_integrity_payloadbip1;
+ struct bio_vec iv1;
+ struct bio_integrity_payloadbip2;
+ struct bio_vec iv2;
#endif
I think it probably is a good idea to put a comment here so that we
know that certain elements of structure assume ordering.
Also I am wondering that what's the gurantee that there are no padding
bytes between bipi1 and iv1 (or bip2 or iv2). I think if there are padding
bytes then the assumption that bio_vec is always following bip will be
broken?
Here's the new patch:
commit e270c9ca843b5c86d59431b0d7a676b7846946d6
Author: Kent Overstreet koverstr...@google.com
Date: Mon Oct 1 14:41:08 2012 -0700
block: Fix a buffer overrun in bio_integrity_split()
bio_integrity_split() seemed to be confusing pointers and arrays -
bip_vec in bio_integrity_payload is an array appended to the end of the
payload, so the bio_vecs in struct bio_pair need to come immediately
after the bio_integrity_payload they're for, and there was an assignment
in bio_integrity_split() that didn't make any sense.
Also, changed bio_integrity_split() to not refer to the bvecs embedded
in struct bio_pair, in case there's padding between them and
bip-bip_vec.
Signed-off-by: Kent Overstreet koverstr...@google.com
CC: Jens Axboe ax...@kernel.dk
CC: Martin K. Petersen martin.peter...@oracle.com
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index a3f28f3..4ae22a8 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -694,15 +694,12 @@ void bio_integrity_split(struct bio *bio, struct bio_pair
*bp, int sectors)
bp-bio1.bi_integrity = bp-bip1;
bp-bio2.bi_integrity = bp-bip2;
- bp-iv1 = bip-bip_vec[0];
- bp-iv2 = bip-bip_vec[0];
+ *bp-bip1.bip_vec = bip-bip_vec[0];
+ *bp-bip2.bip_vec = bip-bip_vec[0];
- bp-bip1.bip_vec[0] = bp-iv1;
- bp-bip2.bip_vec[0] = bp-iv2;
-
- bp-iv1.bv_len = sectors * bi-tuple_size;
- bp-iv2.bv_offset += sectors * bi-tuple_size;
- bp-iv2.bv_len -= sectors * bi-tuple_size;
+ bp-bip1.bip_vec-bv_len = sectors * bi-tuple_size;
+ bp-bip2.bip_vec-bv_offset += sectors * bi-tuple_size;
+ bp-bip2.bip_vec-bv_len -= sectors * bi-tuple_size;
bp-bip1.bip_sector = bio-bi_integrity-bip_sector;
bp-bip2.bip_sector = bio-bi_integrity-bip_sector + nr_sectors;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b31036f..8e2d108 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -200,8 +200,10 @@ struct bio_pair {
struct bio bio1, bio2;
struct bio_vec bv1, bv2;
#if defined(CONFIG_BLK_DEV_INTEGRITY)
- struct bio_integrity_payloadbip1, bip2;
- struct bio_vec iv1, iv2;
+ struct bio_integrity_payloadbip1;
+ struct bio_vec iv1;
+ struct bio_integrity_payloadbip2;
+ struct bio_vec iv2;
#endif
atomic_tcnt;
int error;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at
