Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Wed, Apr 03, 2013 at 12:38:28PM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Apr 03, 2013 at 09:01:06AM -0700, Greg Kroah-Hartman wrote: > > On Wed, Apr 03, 2013 at 04:01:54PM +0200, William Dauchy wrote: > > > On Tue, Mar 12, 2013 at 11:10 PM, Greg Kroah-Hartman > > > wrote: > > > >> > >> IOW I don't see why this got proposed for stable at all. > > > >> > > > > > >> > > So, you suggest to just drop this patch for v3.8.3, don't you? > > > >> > > > > >> > I do, yes. But I'd suggest to get Konrad to agree. > > > >> > > > >> Yes. Lets drop it. > > > > > > > > Now reverted, thanks. > > > > > > Seems like still present in 3.4.x branch. Is that a mistake? > > > > It showed up in 3.4.35, if that's a mistake, and I should revert it, > > please, someone let me know. > > Yes. It is a mistake. Please revert it. Now reverted, thanks. greg k-h -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Wed, Apr 03, 2013 at 09:01:06AM -0700, Greg Kroah-Hartman wrote: > On Wed, Apr 03, 2013 at 04:01:54PM +0200, William Dauchy wrote: > > On Tue, Mar 12, 2013 at 11:10 PM, Greg Kroah-Hartman > > wrote: > > >> > >> IOW I don't see why this got proposed for stable at all. > > >> > > > > >> > > So, you suggest to just drop this patch for v3.8.3, don't you? > > >> > > > >> > I do, yes. But I'd suggest to get Konrad to agree. > > >> > > >> Yes. Lets drop it. > > > > > > Now reverted, thanks. > > > > Seems like still present in 3.4.x branch. Is that a mistake? > > It showed up in 3.4.35, if that's a mistake, and I should revert it, > please, someone let me know. Yes. It is a mistake. Please revert it. > > thanks, > > greg k-h -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Wed, Apr 03, 2013 at 04:01:54PM +0200, William Dauchy wrote: > On Tue, Mar 12, 2013 at 11:10 PM, Greg Kroah-Hartman > wrote: > >> > >> IOW I don't see why this got proposed for stable at all. > >> > > > >> > > So, you suggest to just drop this patch for v3.8.3, don't you? > >> > > >> > I do, yes. But I'd suggest to get Konrad to agree. > >> > >> Yes. Lets drop it. > > > > Now reverted, thanks. > > Seems like still present in 3.4.x branch. Is that a mistake? It showed up in 3.4.35, if that's a mistake, and I should revert it, please, someone let me know. thanks, greg k-h -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Tue, Mar 12, 2013 at 11:10 PM, Greg Kroah-Hartman wrote: >> > >> IOW I don't see why this got proposed for stable at all. >> > > >> > > So, you suggest to just drop this patch for v3.8.3, don't you? >> > >> > I do, yes. But I'd suggest to get Konrad to agree. >> >> Yes. Lets drop it. > > Now reverted, thanks. Seems like still present in 3.4.x branch. Is that a mistake? Regards, -- William -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Mon, Mar 04, 2013 at 10:02:46AM -0500, Konrad Rzeszutek Wilk wrote: > On Mon, Mar 04, 2013 at 09:14:47AM +, Jan Beulich wrote: > > >>> On 04.03.13 at 10:11, Paul Bolle wrote: > > > On Mon, 2013-03-04 at 07:55 +, Jan Beulich wrote: > > >> >>> On 03.03.13 at 11:20, Paul Bolle wrote: > > >> For one, a fix for the (indeed valid) compiler warning has been in > > >> Konrad's tree for several days > > >> > > > (http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/commit/drivers/bl > > > > > > ock/xen-blkback/blkback.c?id=a72d9002f80bffd7e4c7d60e5a9caa0cddffe894). > > > > > > Thanks. (For the record: that commit is titled "xen/xen-blkback: > > > preq.dev is used without initialized".) > > > > > >> And second, there's nothing really being fixed with the patch in > > >> question here. The title is kind of misleading, as the handle > > >> coming from the frontend is - without said patch - in the worst > > >> case being used for the very message that triggered the > > >> compiler warning. Nothing else is affected, the code just gave > > >> the impression that the handle was used. > > Which it actually is not, unless somebody compiles the kernel > with #define DEBUG 1 > at the top of the file. > > > >> > > >> IOW I don't see why this got proposed for stable at all. > > > > > > So, you suggest to just drop this patch for v3.8.3, don't you? > > > > I do, yes. But I'd suggest to get Konrad to agree. > > Yes. Lets drop it. Now reverted, thanks. greg k-h -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Mon, Mar 04, 2013 at 09:14:47AM +, Jan Beulich wrote: > >>> On 04.03.13 at 10:11, Paul Bolle wrote: > > On Mon, 2013-03-04 at 07:55 +, Jan Beulich wrote: > >> >>> On 03.03.13 at 11:20, Paul Bolle wrote: > >> For one, a fix for the (indeed valid) compiler warning has been in > >> Konrad's tree for several days > >> > > (http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/commit/drivers/bl > > > > ock/xen-blkback/blkback.c?id=a72d9002f80bffd7e4c7d60e5a9caa0cddffe894). > > > > Thanks. (For the record: that commit is titled "xen/xen-blkback: > > preq.dev is used without initialized".) > > > >> And second, there's nothing really being fixed with the patch in > >> question here. The title is kind of misleading, as the handle > >> coming from the frontend is - without said patch - in the worst > >> case being used for the very message that triggered the > >> compiler warning. Nothing else is affected, the code just gave > >> the impression that the handle was used. Which it actually is not, unless somebody compiles the kernel with #define DEBUG 1 at the top of the file. > >> > >> IOW I don't see why this got proposed for stable at all. > > > > So, you suggest to just drop this patch for v3.8.3, don't you? > > I do, yes. But I'd suggest to get Konrad to agree. Yes. Lets drop it. -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
>>> On 04.03.13 at 10:11, Paul Bolle wrote: > On Mon, 2013-03-04 at 07:55 +, Jan Beulich wrote: >> >>> On 03.03.13 at 11:20, Paul Bolle wrote: >> For one, a fix for the (indeed valid) compiler warning has been in >> Konrad's tree for several days >> > (http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/commit/drivers/bl > ock/xen-blkback/blkback.c?id=a72d9002f80bffd7e4c7d60e5a9caa0cddffe894). > > Thanks. (For the record: that commit is titled "xen/xen-blkback: > preq.dev is used without initialized".) > >> And second, there's nothing really being fixed with the patch in >> question here. The title is kind of misleading, as the handle >> coming from the frontend is - without said patch - in the worst >> case being used for the very message that triggered the >> compiler warning. Nothing else is affected, the code just gave >> the impression that the handle was used. >> >> IOW I don't see why this got proposed for stable at all. > > So, you suggest to just drop this patch for v3.8.3, don't you? I do, yes. But I'd suggest to get Konrad to agree. Jan -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Mon, 2013-03-04 at 07:55 +, Jan Beulich wrote: > >>> On 03.03.13 at 11:20, Paul Bolle wrote: > For one, a fix for the (indeed valid) compiler warning has been in > Konrad's tree for several days > (http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/commit/drivers/block/xen-blkback/blkback.c?id=a72d9002f80bffd7e4c7d60e5a9caa0cddffe894). Thanks. (For the record: that commit is titled "xen/xen-blkback: preq.dev is used without initialized".) > And second, there's nothing really being fixed with the patch in > question here. The title is kind of misleading, as the handle > coming from the frontend is - without said patch - in the worst > case being used for the very message that triggered the > compiler warning. Nothing else is affected, the code just gave > the impression that the handle was used. > > IOW I don't see why this got proposed for stable at all. So, you suggest to just drop this patch for v3.8.3, don't you? Paul Bolle -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
>>> On 03.03.13 at 11:20, Paul Bolle wrote: > Perhaps Konrad, Jan, Or Ian can tell whether the patch still needs to go > in stable as is, because the problem it fixes is more severe than the > problem it apparently creates. Maybe a mainline fix is needed before > this can go in, or perhaps even a stable specific fix (if context > changes are to blame). For one, a fix for the (indeed valid) compiler warning has been in Konrad's tree for several days (http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/commit/drivers/block/xen-blkback/blkback.c?id=a72d9002f80bffd7e4c7d60e5a9caa0cddffe894). And second, there's nothing really being fixed with the patch in question here. The title is kind of misleading, as the handle coming from the frontend is - without said patch - in the worst case being used for the very message that triggered the compiler warning. Nothing else is affected, the code just gave the impression that the handle was used. IOW I don't see why this got proposed for stable at all. Jan -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Sun, Mar 03, 2013 at 11:20:02AM +0100, Paul Bolle wrote: > On Sat, 2013-03-02 at 23:10 +, Ben Hutchings wrote: > > On Sat, 2013-03-02 at 23:35 +0100, Paul Bolle wrote: > > > 1) So if xen_vbd_translate() fails, it can return before setting > > > preq.dev. That makes the call of pr_debug() use an uninitialized value, > > > doesn't it? > > > > Oh yes, so it's a completely valid warning in this case! > > Thanks. > > Perhaps Konrad, Jan, Or Ian can tell whether the patch still needs to go > in stable as is, because the problem it fixes is more severe than the > problem it apparently creates. Maybe a mainline fix is needed before > this can go in, or perhaps even a stable specific fix (if context > changes are to blame). I've left it in, as-is. If this is a problem, please fix it in Linus's tree, and then I'll be glad to accept the same fix here in the stable kernel releases as well. Just be sure to let me know if that happens. thanks, greg k-h -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Sat, 2013-03-02 at 23:10 +, Ben Hutchings wrote: > On Sat, 2013-03-02 at 23:35 +0100, Paul Bolle wrote: > > 1) So if xen_vbd_translate() fails, it can return before setting > > preq.dev. That makes the call of pr_debug() use an uninitialized value, > > doesn't it? > > Oh yes, so it's a completely valid warning in this case! Thanks. Perhaps Konrad, Jan, Or Ian can tell whether the patch still needs to go in stable as is, because the problem it fixes is more severe than the problem it apparently creates. Maybe a mainline fix is needed before this can go in, or perhaps even a stable specific fix (if context changes are to blame). Paul Bolle -- 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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Sat, 2013-03-02 at 23:35 +0100, Paul Bolle wrote:
[...]
> 0) I've had another look at the relevant code in v3.8.2-rc1. It can be
> summarized like this:
>
> static int xen_vbd_translate()
> {
> [...]
> int rc = -EACCES;
>
> if ([...])
> goto out;
> [...]
>
> [p]req->dev = vbd->pdevice;
> [p]req->bdev = vbd->bdev;
> [...]
>
> out:
> return rc;
> }
>
> static int dispatch_rw_block_io()
> {
> struct phys_req preq;
> [...]
>
> preq.sector_number = req->u.rw.sector_number;
> preq.nr_sects = 0;
> [...]
>
> for ([...]) {
> [...]
> preq.nr_sects += seg[i].nsec;
> }
>
> if (xen_vbd_translate(&preq, blkif, operation) != 0) {
> pr_debug(DRV_PFX "access denied: %s of [%llu,%llu] on
> dev=%04x\n",
>operation == READ ? "read" : "write",
>preq.sector_number,
>preq.sector_number + preq.nr_sects, preq.dev);
> goto [...];
> }
> [...]
> }
>
> 1) So if xen_vbd_translate() fails, it can return before setting
> preq.dev. That makes the call of pr_debug() use an uninitialized value,
> doesn't it?
Oh yes, so it's a completely valid warning in this case!
> Does inlining xen_vbd_translate() and/or
> dispatch_rw_block_io() generate code were that can't happen anymore?
> (Both functions being static they probably are inlined.)
>
> 2) And even if inlining does generate code where this can't happen,
> isn't it enough that preq.dev can be used uninitialized if no code were
> inlined?
If gcc inlines a function call, it analyses data flow between the two
functions. Otherwise it assumes that the called function will
initialise any variable it's given a pointer to, and the warning doesn't
appear. (That's my experience, anyway.)
Ben.
--
Ben Hutchings
Computers are not intelligent. They only think they are.
signature.asc
Description: This is a digitally signed message part
Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Sat, 2013-03-02 at 19:48 +, Ben Hutchings wrote:
> When gcc compiles something like this:
>
> static int foo(int *p)
> {
> if (rand() & 1)
> return -1;
> *p = 0;
> return 0;
> }
>
> int bar(void)
> {
> int i;
> if (foo(&i) < 0)
> return 1;
> return i;
> }
>
> and inlines foo() into bar(), sometimes it fails to recognise that i
> will definitely be initialised before use. This simple example seems to
> be OK but more complex functions such as these will often trigger this
> warning. The warning is really quite useless now.
0) I've had another look at the relevant code in v3.8.2-rc1. It can be
summarized like this:
static int xen_vbd_translate()
{
[...]
int rc = -EACCES;
if ([...])
goto out;
[...]
[p]req->dev = vbd->pdevice;
[p]req->bdev = vbd->bdev;
[...]
out:
return rc;
}
static int dispatch_rw_block_io()
{
struct phys_req preq;
[...]
preq.sector_number = req->u.rw.sector_number;
preq.nr_sects = 0;
[...]
for ([...]) {
[...]
preq.nr_sects += seg[i].nsec;
}
if (xen_vbd_translate(&preq, blkif, operation) != 0) {
pr_debug(DRV_PFX "access denied: %s of [%llu,%llu] on
dev=%04x\n",
operation == READ ? "read" : "write",
preq.sector_number,
preq.sector_number + preq.nr_sects, preq.dev);
goto [...];
}
[...]
}
1) So if xen_vbd_translate() fails, it can return before setting
preq.dev. That makes the call of pr_debug() use an uninitialized value,
doesn't it? Does inlining xen_vbd_translate() and/or
dispatch_rw_block_io() generate code were that can't happen anymore?
(Both functions being static they probably are inlined.)
2) And even if inlining does generate code where this can't happen,
isn't it enough that preq.dev can be used uninitialized if no code were
inlined?
Paul Bolle
--
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: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Fri, 2013-03-01 at 22:12 +0100, Paul Bolle wrote:
> On Fri, 2013-03-01 at 11:44 -0800, Greg Kroah-Hartman wrote:
> > 3.8-stable review patch. If anyone has any objections, please let me know.
> >
> > --
> >
> > From: Konrad Rzeszutek Wilk
> >
> > commit 01c681d4c70d64cb72142a2823f27c4146a02e63 upstream.
> >
> > The 'handle' is the device that the request is from. For the life-time
> > of the ring we copy it from a request to a response so that the frontend
> > is not surprised by it. But we do not need it - when we start processing
> > I/Os we have our own 'struct phys_req' which has only most essential
> > information about the request. In fact the 'vbd_translate' ends up
> > over-writing the preq.dev with a value from the backend.
>
> Unless that call to vb_translate() fails, doesn't it? Wouldn't preq.dev
> still contain random data in that case?
>
> > This assignment of preq.dev with the 'handle' value is superfluous
> > so lets not do it.
> >
> > Acked-by: Jan Beulich
> > Acked-by: Ian Campbell
> > Signed-off-by: Konrad Rzeszutek Wilk
> > Signed-off-by: Greg Kroah-Hartman
> >
> > ---
> > drivers/block/xen-blkback/blkback.c |1 -
> > 1 file changed, 1 deletion(-)
> >
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> > @@ -879,7 +879,6 @@ static int dispatch_rw_block_io(struct x
> > goto fail_response;
> > }
> >
> > - preq.dev = req->u.rw.handle;
> > preq.sector_number = req->u.rw.sector_number;
> > preq.nr_sects = 0;
> >
>
> This introduces a new GCC warning in the stable 3.8.y tree:
> drivers/block/xen-blkback/blkback.c: In function 'dispatch_rw_block_io':
> drivers/block/xen-blkback/blkback.c:904:3: warning: 'preq.dev' may be
> used uninitialized in this function [-Wuninitialized]
>
> It does look GCC is right here. But I'm totally new to the code in
> question, so I'll just ask whether this can really go in stable as is.
When gcc compiles something like this:
static int foo(int *p)
{
if (rand() & 1)
return -1;
*p = 0;
return 0;
}
int bar(void)
{
int i;
if (foo(&i) < 0)
return 1;
return i;
}
and inlines foo() into bar(), sometimes it fails to recognise that i
will definitely be initialised before use. This simple example seems to
be OK but more complex functions such as these will often trigger this
warning. The warning is really quite useless now.
Ben.
--
Ben Hutchings
Computers are not intelligent. They only think they are.
signature.asc
Description: This is a digitally signed message part
Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.
On Fri, 2013-03-01 at 11:44 -0800, Greg Kroah-Hartman wrote: > 3.8-stable review patch. If anyone has any objections, please let me know. > > -- > > From: Konrad Rzeszutek Wilk > > commit 01c681d4c70d64cb72142a2823f27c4146a02e63 upstream. > > The 'handle' is the device that the request is from. For the life-time > of the ring we copy it from a request to a response so that the frontend > is not surprised by it. But we do not need it - when we start processing > I/Os we have our own 'struct phys_req' which has only most essential > information about the request. In fact the 'vbd_translate' ends up > over-writing the preq.dev with a value from the backend. Unless that call to vb_translate() fails, doesn't it? Wouldn't preq.dev still contain random data in that case? > This assignment of preq.dev with the 'handle' value is superfluous > so lets not do it. > > Acked-by: Jan Beulich > Acked-by: Ian Campbell > Signed-off-by: Konrad Rzeszutek Wilk > Signed-off-by: Greg Kroah-Hartman > > --- > drivers/block/xen-blkback/blkback.c |1 - > 1 file changed, 1 deletion(-) > > --- a/drivers/block/xen-blkback/blkback.c > +++ b/drivers/block/xen-blkback/blkback.c > @@ -879,7 +879,6 @@ static int dispatch_rw_block_io(struct x > goto fail_response; > } > > - preq.dev = req->u.rw.handle; > preq.sector_number = req->u.rw.sector_number; > preq.nr_sects = 0; > This introduces a new GCC warning in the stable 3.8.y tree: drivers/block/xen-blkback/blkback.c: In function 'dispatch_rw_block_io': drivers/block/xen-blkback/blkback.c:904:3: warning: 'preq.dev' may be used uninitialized in this function [-Wuninitialized] It does look GCC is right here. But I'm totally new to the code in question, so I'll just ask whether this can really go in stable as is. Paul Bolle -- 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/
[ 34/77] xen/blkback: Dont trust the handle from the frontend.
3.8-stable review patch. If anyone has any objections, please let me know. -- From: Konrad Rzeszutek Wilk commit 01c681d4c70d64cb72142a2823f27c4146a02e63 upstream. The 'handle' is the device that the request is from. For the life-time of the ring we copy it from a request to a response so that the frontend is not surprised by it. But we do not need it - when we start processing I/Os we have our own 'struct phys_req' which has only most essential information about the request. In fact the 'vbd_translate' ends up over-writing the preq.dev with a value from the backend. This assignment of preq.dev with the 'handle' value is superfluous so lets not do it. Acked-by: Jan Beulich Acked-by: Ian Campbell Signed-off-by: Konrad Rzeszutek Wilk Signed-off-by: Greg Kroah-Hartman --- drivers/block/xen-blkback/blkback.c |1 - 1 file changed, 1 deletion(-) --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -879,7 +879,6 @@ static int dispatch_rw_block_io(struct x goto fail_response; } - preq.dev = req->u.rw.handle; preq.sector_number = req->u.rw.sector_number; preq.nr_sects = 0; -- 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/
