[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
On 08.07.21 08:37, Jan Beulich wrote:
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.
Sure.
My question was rather: does the rmb() really eliminate the possibility
of a double read introduced by the compiler? If yes, moving the code is
the correct solution.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
