Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2021 16:23, Juergen Gross wrote:
> On 17.05.21 16:11, Jan Beulich wrote:
>> On 13.05.2021 12:02, Juergen Gross wrote:
>>> @@ -1574,10 +1580,16 @@ static irqreturn_t blkif_interrupt(int irq, void 
>>> *dev_id)
>>>     spin_lock_irqsave(&rinfo->ring_lock, flags);
>>>    again:
>>>     rp = rinfo->ring.sring->rsp_prod;
>>> +   if (RING_RESPONSE_PROD_OVERFLOW(&rinfo->ring, rp)) {
>>> +           pr_alert("%s: illegal number of responses %u\n",
>>> +                    info->gd->disk_name, rp - rinfo->ring.rsp_cons);
>>> +           goto err;
>>> +   }
>>>     rmb(); /* Ensure we see queued responses up to 'rp'. */
>>
>> I think you want to insert after the barrier.
> 
> Why? The relevant variable which is checked is "rp". The result of the
> check is in no way depending on the responses themselves. And any change
> of rsp_cons is protected by ring_lock, so there is no possibility of
> reading an old value here.

But this is a standard double read situation: You might check a value
and then (via a separate read) use a different one past the barrier.

Jan