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

[Ben Hutchings]

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.

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