xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Jens Axboe" <axboe@kernel.dk>,
	xen-devel@lists.xenproject.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly
Date: Mon, 17 May 2021 17:22:34 +0200	[thread overview]
Message-ID: <030ef85e-b5af-f46e-c8dc-88b8d195c4e1@suse.com> (raw)
In-Reply-To: <a19a13ba-a386-2808-ad85-338d47085fa6@suse.com>


[-- Attachment #1.1.1: Type: text/plain, Size: 1730 bytes --]

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.

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

  reply	other threads:[~2021-05-17 15:22 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-13 10:02 [PATCH 0/8] xen: harden frontends against malicious backends Juergen Gross
2021-05-13 10:02 ` [PATCH 1/8] xen: sync include/xen/interface/io/ring.h with Xen's newest version Juergen Gross
2021-05-13 10:02 ` [PATCH 2/8] xen/blkfront: read response from backend only once Juergen Gross
2021-05-17 13:50   ` Jan Beulich
2021-05-13 10:02 ` [PATCH 3/8] xen/blkfront: don't take local copy of a request from the ring page Juergen Gross
2021-05-17 14:01   ` Jan Beulich
2021-05-17 14:11     ` Juergen Gross
2021-05-13 10:02 ` [PATCH 4/8] xen/blkfront: don't trust the backend response data blindly Juergen Gross
2021-05-17 14:11   ` Jan Beulich
2021-05-17 14:23     ` Juergen Gross
2021-05-17 15:12       ` Jan Beulich
2021-05-17 15:22         ` Juergen Gross [this message]
2021-05-17 15:33           ` Jan Beulich
2021-07-08  5:47             ` Juergen Gross
2021-07-08  6:37               ` Jan Beulich
2021-07-08  6:40                 ` Juergen Gross
2021-07-08  6:52                   ` Jan Beulich
2021-07-08  6:56                     ` Juergen Gross
2021-05-13 10:02 ` [PATCH 5/8] xen/netfront: read response from backend only once Juergen Gross
2021-05-17 14:20   ` Jan Beulich
2021-05-17 14:24     ` Juergen Gross
2021-05-13 10:03 ` [PATCH 6/8] xen/netfront: don't read data from request on the ring page Juergen Gross
2021-05-17 15:08   ` Jan Beulich
2021-05-13 10:03 ` [PATCH 7/8] xen/netfront: don't trust the backend response data blindly Juergen Gross
2021-05-17 15:31   ` Jan Beulich
2021-05-13 10:03 ` [PATCH 8/8] xen/hvc: replace BUG_ON() with negative return value Juergen Gross
2021-05-13 10:16   ` Christophe Leroy
2021-05-13 10:20     ` Juergen Gross
2021-05-13 10:25   ` Greg Kroah-Hartman
2021-05-13 10:35     ` Juergen Gross
2021-05-21 10:43 ` [PATCH 0/8] xen: harden frontends against malicious backends Marek Marczykowski-Górecki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=030ef85e-b5af-f46e-c8dc-88b8d195c4e1@suse.com \
    --to=jgross@suse.com \
    --cc=axboe@kernel.dk \
    --cc=boris.ostrovsky@oracle.com \
    --cc=jbeulich@suse.com \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=roger.pau@citrix.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).