[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 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 
        spin_lock_irqsave(&rinfo->ring_lock, flags);
        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.

So if the other side is writing it, the write could always happen after
the test and before the loop is started. This is no real issue here as
the frontend would very soon stumble over an illegal response (either
no request pending, or some other inconsistency). The test is meant to
have a more detailed error message in case it hits.

In the end it doesn't really matter, so I can change it. I just wanted
to point out that IMO both variants are equally valid.


Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.