From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>,
Julien Grall <julien.grall.oss@gmail.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
hongyxia@amazon.co.uk, Ian Jackson <iwj@xenproject.org>,
Julien Grall <jgrall@amazon.com>, Paul Durrant <paul@xen.org>
Subject: Re: [for-4.15][PATCH v2 3/5] xen/iommu: iommu_map: Don't crash the domain if it is dying
Date: Wed, 10 Feb 2021 14:58:48 +0000 [thread overview]
Message-ID: <04755ab0-94fe-f797-1cfd-cf8aa22ceba0@xen.org> (raw)
In-Reply-To: <6fb54306-20e6-516f-cdcf-c7d8dd430b96@suse.com>
Hi Jan,
On 10/02/2021 14:14, Jan Beulich wrote:
> On 09.02.2021 22:14, Julien Grall wrote:
>> On Tue, 9 Feb 2021 at 20:28, Paul Durrant <xadimgnik@gmail.com> wrote:
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: 09 February 2021 15:28
>>>>
>>>> It is a bit pointless to crash a domain that is already dying. This will
>>>> become more an annoyance with a follow-up change where page-table
>>>> allocation will be forbidden when the domain is dying.
>>>>
>>>> Security wise, there is no change as the devices would still have access
>>>> to the IOMMU page-tables even if the domain has crashed until Xen
>>>> start to relinquish the resources.
>>>>
>>>> For x86, we rely on dom_iommu(d)->arch.mapping.lock to ensure
>>>> d->is_dying is correctly observed (a follow-up patch will held it in the
>>>> relinquish path).
>
> Am I to understand this to mean that at this point of the series
> things aren't really correct yet in this regard? If so, wouldn't
> it be better to re-order?
You asked this specific order... So are you saying you want me to use
the original ordering?
>
>>>> For Arm, there is still a small race possible. But there is so far no
>>>> failure specific to a domain dying.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>
>>>> ---
>>>>
>>>> This was spotted when trying to destroy IOREQ servers while the domain
>>>> is dying. The code will try to add the entry back in the P2M and
>>>> therefore update the P2M (see arch_ioreq_server_disable() ->
>>>> hvm_add_ioreq_gfn()).
>>>>
>>>> It should be possible to skip the mappin in hvm_add_ioreq_gfn(), however
>>>> I didn't try a patch yet because checking d->is_dying can be racy (I
>>>> can't find a proper lock).
>
> I understand the concern. I find it odd though that we permit
> iommu_map() to do anything at all when the domain is already
> dying. So irrespective of the remark below, how about bailing
> from iommu_map() earlier when the domain is dying?
I felt this was potentially too racy to use it. But it should be fine if
keep the !d->is_dying below.
>
>>>> --- a/xen/drivers/passthrough/iommu.c
>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>> @@ -272,7 +272,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
>>>> flush_flags) )
>>>> continue;
>>>>
>>>> - if ( !is_hardware_domain(d) )
>>>> + if ( !is_hardware_domain(d) && !d->is_dying )
>>>> domain_crash(d);
>>>
>>> Would it make more sense to check is_dying inside domain_crash() (and turn it into a no-op in that case)?
>>
>> Jan also suggested moving the check in domain_crash(). However, I felt
>> it is potentially a too risky change for 4.15 as there are quite a few
>> callers.
>
> This is a fair point. However, in such a case I'd prefer symmetry
> at least throughout this one source file (there are three more
> places), unless there are strong reasons against doing so.
I can have a look and see if the decision is easy to make.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-02-10 14:59 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-09 15:28 [for-4.15][PATCH v2 0/5] xen/iommu: Collection of bug fixes for IOMMU teadorwn Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 1/5] xen/x86: p2m: Don't map the special pages in the IOMMU page-tables Julien Grall
2021-02-09 20:19 ` Paul Durrant
2021-02-10 8:29 ` Roger Pau Monné
2021-02-10 8:50 ` Julien Grall
2021-02-10 9:34 ` Julien Grall
2021-02-10 11:10 ` Jan Beulich
2021-02-10 11:34 ` Roger Pau Monné
2021-02-10 11:38 ` Jan Beulich
2021-02-10 11:40 ` Julien Grall
2021-02-10 11:45 ` Jan Beulich
2021-02-10 11:48 ` Julien Grall
2021-02-10 11:54 ` Roger Pau Monné
2021-02-10 13:12 ` Jan Beulich
2021-02-10 15:24 ` Roger Pau Monné
2021-02-10 15:53 ` Jan Beulich
2021-02-10 13:08 ` Jan Beulich
2021-02-10 11:26 ` Jan Beulich
2021-02-15 11:38 ` Julien Grall
2021-02-15 12:36 ` Jan Beulich
2021-02-15 12:54 ` Julien Grall
2021-02-15 13:14 ` Jan Beulich
2021-02-17 11:21 ` Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 2/5] xen/iommu: Check if the IOMMU was initialized before tearing down Julien Grall
2021-02-09 20:22 ` Paul Durrant
2021-02-17 11:25 ` Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 3/5] xen/iommu: iommu_map: Don't crash the domain if it is dying Julien Grall
2021-02-09 20:28 ` Paul Durrant
2021-02-09 21:14 ` Julien Grall
2021-02-10 14:14 ` Jan Beulich
2021-02-10 14:58 ` Julien Grall [this message]
2021-02-10 15:56 ` Jan Beulich
2021-02-09 15:28 ` [for-4.15][PATCH v2 4/5] xen/iommu: x86: Don't leak the IOMMU page-tables Julien Grall
2021-02-09 20:33 ` Paul Durrant
2021-02-10 14:32 ` Jan Beulich
2021-02-10 15:04 ` Julien Grall
2021-02-10 16:12 ` Jan Beulich
2021-02-17 11:49 ` Julien Grall
2021-02-17 12:57 ` Jan Beulich
2021-02-10 14:44 ` Jan Beulich
2021-02-09 15:28 ` [for-4.15][PATCH v2 5/5] xen/iommu: x86: Clear the root page-table before freeing the page-tables Julien Grall
2021-02-09 20:36 ` Paul Durrant
2021-02-10 2:21 ` Tian, Kevin
2021-02-17 13:54 ` Julien Grall
2021-02-10 14:39 ` Jan Beulich
2021-02-09 16:47 ` [for-4.15][PATCH v2 0/5] xen/iommu: Collection of bug fixes for IOMMU teadorwn Ian Jackson
2021-02-17 11:33 ` Julien Grall
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=04755ab0-94fe-f797-1cfd-cf8aa22ceba0@xen.org \
--to=julien@xen.org \
--cc=hongyxia@amazon.co.uk \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=jgrall@amazon.com \
--cc=julien.grall.oss@gmail.com \
--cc=paul@xen.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).