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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2021 07:47, Juergen Gross wrote:
> On 17.05.21 17:33, Jan Beulich wrote:
>> On 17.05.2021 17:22, Juergen Gross wrote:
>>> On 17.05.21 17:12, Jan Beulich wrote:
>>>> 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.
>>>
>>> Yes and no.
>>>
>>> rsp_cons should never be written by the other side, and additionally
>>> it would be read multiple times anyway.
>>
>> But I'm talking about rsp_prod, as that's what rp gets loaded from.
> 
> Oh, now I get your problem.
> 
> But shouldn't that better be solved by using READ_ONCE() for reading rp
> instead?

Not sure - the rmb() is needed anyway aiui, and hence you could as well
move your code addition.

Jan