Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.

[Ben Hutchings]

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 <konrad.wilk@xxxxxxxxxx>
> > 
> > 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 <jbeulich@xxxxxxxx>
> > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > 
> > ---
> >  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.

Attachment: signature.asc
Description: This is a digitally signed message part