Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
On 08.07.21 08:52, Jan Beulich wrote:
On 08.07.2021 08:40, Juergen Gross wrote:
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(>ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
+ if (RING_RESPONSE_PROD_OVERFLOW(>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.
It doesn't eliminate the possibility of a double read, but (leaving
aside split accesses) that's not what you care about here. What you
need is a single stable value to operate on. No matter how many
(non-split) reads the compiler may issue to fill "rp", the final
read's value will be used in the subsequent calculation. Or at
least that's been my understanding; thinking about it the compiler
might issue multiple reads into distinct registers ahead of the
barrier, and use different registers for different subsequent
operations. While this would look like intentionally inefficient
code generation to me, you may indeed want to play safe and use
ACCESS_ONCE() _and_ the barrier. I guess there are more places then
which would want similar treatment, and it's not a problem that
this change introduces ...
Nevertheless I think I can change it right away. It will also help
against load tearing.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
On 08.07.2021 08:40, Juergen Gross wrote:
> 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(>ring_lock, flags);
> again:
> rp = rinfo->ring.sring->rsp_prod;
> + if (RING_RESPONSE_PROD_OVERFLOW(>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.
It doesn't eliminate the possibility of a double read, but (leaving
aside split accesses) that's not what you care about here. What you
need is a single stable value to operate on. No matter how many
(non-split) reads the compiler may issue to fill "rp", the final
read's value will be used in the subsequent calculation. Or at
least that's been my understanding; thinking about it the compiler
might issue multiple reads into distinct registers ahead of the
barrier, and use different registers for different subsequent
operations. While this would look like intentionally inefficient
code generation to me, you may indeed want to play safe and use
ACCESS_ONCE() _and_ the barrier. I guess there are more places then
which would want similar treatment, and it's not a problem that
this change introduces ...
Jan
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(>ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
+ if (RING_RESPONSE_PROD_OVERFLOW(>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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
>>> again:
>>> rp = rinfo->ring.sring->rsp_prod;
>>> + if (RING_RESPONSE_PROD_OVERFLOW(>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
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
+ if (RING_RESPONSE_PROD_OVERFLOW(>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?
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
> again:
> rp = rinfo->ring.sring->rsp_prod;
> + if (RING_RESPONSE_PROD_OVERFLOW(>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.
Jan
> 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.
>
>
> Juergen
>
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
*dev_id)
spin_lock_irqsave(>ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
+ if (RING_RESPONSE_PROD_OVERFLOW(>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.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
>>>again:
>>> rp = rinfo->ring.sring->rsp_prod;
>>> + if (RING_RESPONSE_PROD_OVERFLOW(>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
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
+ if (RING_RESPONSE_PROD_OVERFLOW(>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.
@@ -1680,6 +1707,11 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
spin_unlock_irqrestore(>ring_lock, flags);
return IRQ_HANDLED;
+
+ err:
+ info->connected = BLKIF_STATE_ERROR;
+ pr_alert("%s disabled for further use\n", info->gd->disk_name);
+ return IRQ_HANDLED;
}
Am I understanding that a suspend (and then resume) can be used to
recover from error state? If so - is this intentional? If so in turn,
would it make sense to spell this out in the description?
I'd call it a nice side effect rather than intention. I can add a remark
to the commit message if you want.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
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(>ring_lock, flags);
> again:
> rp = rinfo->ring.sring->rsp_prod;
> + if (RING_RESPONSE_PROD_OVERFLOW(>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.
> @@ -1680,6 +1707,11 @@ static irqreturn_t blkif_interrupt(int irq, void
> *dev_id)
> spin_unlock_irqrestore(>ring_lock, flags);
>
> return IRQ_HANDLED;
> +
> + err:
> + info->connected = BLKIF_STATE_ERROR;
> + pr_alert("%s disabled for further use\n", info->gd->disk_name);
> + return IRQ_HANDLED;
> }
Am I understanding that a suspend (and then resume) can be used to
recover from error state? If so - is this intentional? If so in turn,
would it make sense to spell this out in the description?
Jan
