Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal withmisbehaving backends.

[Konrad Rzeszutek Wilk]

> >> @@ -145,6 +145,7 @@ static void add_id_to_freelist(struct blkfront_info 
> > *info,
> >>  			       unsigned long id)
> >>  {
> >>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> >> +	BUG_ON(info->shadow[id].request == NULL);
> 
> This only catches a small sub-portion of possible bad backend
> behavior. Checking (as the very first thing in the function) e.g.
> 
> info->shadow[id].req.u.rw.id == id
> 
> would seem to cover a broader set (based on my recent looking
> at similar mismatches apparently resulting from the qdisk
> backend occasionally sending bad/duplicate responses).
> 
> But take this with the below applied here too.
> 
> >>  	info->shadow[id].request = NULL;
> >>  	info->shadow_free = id;
> >>  }
> >> @@ -746,6 +747,12 @@ static irqreturn_t blkif_interrupt(int irq, void 
> > *dev_id)
> >>  
> >>  		bret = RING_GET_RESPONSE(&info->ring, i);
> >>  		id   = bret->id;
> >> +		/*
> >> +		 * The backend has messed up and given us an id that we would
> >> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> >> +		 * look in get_id_from_freelist.
> >> +		 */
> >> +		BUG_ON(id >= BLK_RING_SIZE);
> >>  		req  = info->shadow[id].request;
> >>  
> >>  		if (bret->operation != BLKIF_OP_DISCARD)
> > 
> > While we should certainly check whether bret->id is valid before
> > using it, is it actually correct that the frontend BUGs in response of a
> > backend bug?

The 'id' is used to get the 'struct request' and to do do the grant unmaps.
Since it would be outside the shadow structure it would fetch garbage as
'struct request'.

> > 
> > If the IO doesn't involve the root disk, the guest might be able to
> > function correctly without communicating with the backend at all.
> > I think we should WARN and return error. Maybe also call blkfront_remove
> > if we can.
> 
> I very much agree to this.

The blkfront_remove part is .. that is going to take some surgery to do
and I don't think I am going to be able to attempt that within the next couple
of weeks. So lets put that on the TODO list and just do this one?