* [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
@ 2017-06-21 9:30 ` Jan Beulich
2017-06-21 11:26 ` Andrew Cooper
2017-06-21 15:44 ` George Dunlap
2017-06-21 9:31 ` [PATCH 02/11] gnttab: remove redundant xenheap check from gnttab_transfer() Jan Beulich
` (9 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:30 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
/*
- * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
- * foreign domain has previously registered its interest in the transfer via
- * <domid, ref>.
+ * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
+ * has previously registered its interest in the transfer via <domid, ref>.
*
* Note that, even if the transfer fails, the specified page no longer belongs
* to the calling domain *unless* the error is GNTST_bad_page.
+ *
+ * Note further that only PV guests can use this operation.
*/
struct gnttab_transfer {
/* IN parameters. */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
* Returns zero on complete success, otherwise a negative error code.
* On complete success then always @nr_exchanged == @in.nr_extents.
* On partial success @nr_exchanged indicates how much work was done.
+ *
+ * Note that only PV guests can use this operation.
*/
#define XENMEM_exchange 11
struct xen_memory_exchange {
[-- Attachment #2: xsa217-doc.patch --]
[-- Type: text/plain, Size: 1394 bytes --]
public: adjust documentation following XSA-217
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
/*
- * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
- * foreign domain has previously registered its interest in the transfer via
- * <domid, ref>.
+ * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
+ * has previously registered its interest in the transfer via <domid, ref>.
*
* Note that, even if the transfer fails, the specified page no longer belongs
* to the calling domain *unless* the error is GNTST_bad_page.
+ *
+ * Note further that only PV guests can use this operation.
*/
struct gnttab_transfer {
/* IN parameters. */
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
* Returns zero on complete success, otherwise a negative error code.
* On complete success then always @nr_exchanged == @in.nr_extents.
* On partial success @nr_exchanged indicates how much work was done.
+ *
+ * Note that only PV guests can use this operation.
*/
#define XENMEM_exchange 11
struct xen_memory_exchange {
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 9:30 ` [PATCH 01/11] public: adjust documentation following XSA-217 Jan Beulich
@ 2017-06-21 11:26 ` Andrew Cooper
2017-06-21 15:44 ` George Dunlap
1 sibling, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 11:26 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson
On 21/06/17 10:30, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 9:30 ` [PATCH 01/11] public: adjust documentation following XSA-217 Jan Beulich
2017-06-21 11:26 ` Andrew Cooper
@ 2017-06-21 15:44 ` George Dunlap
2017-06-21 15:54 ` Jan Beulich
1 sibling, 1 reply; 31+ messages in thread
From: George Dunlap @ 2017-06-21 15:44 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
Jan,
When you have a long series like this, could you name the files in a way
that makes it easier for a shell script to get their order? e.g.,
01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
Having to manually save-and-apply the name of each patch of an
eleven-patch series separately is fairly annoying. If they started with
a number, I could save them all to the same directory and then use "for
patch in *.patch ; do..." to apply them.
(Using `git send-email` would also make things a lot easier...)
-George
On 21/06/17 10:30, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -411,12 +411,13 @@ typedef struct gnttab_dump_table gnttab_
> DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t);
>
> /*
> - * GNTTABOP_transfer_grant_ref: Transfer <frame> to a foreign domain. The
> - * foreign domain has previously registered its interest in the transfer via
> - * <domid, ref>.
> + * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain
> + * has previously registered its interest in the transfer via <domid, ref>.
> *
> * Note that, even if the transfer fails, the specified page no longer belongs
> * to the calling domain *unless* the error is GNTST_bad_page.
> + *
> + * Note further that only PV guests can use this operation.
> */
> struct gnttab_transfer {
> /* IN parameters. */
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -102,6 +102,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_reser
> * Returns zero on complete success, otherwise a negative error code.
> * On complete success then always @nr_exchanged == @in.nr_extents.
> * On partial success @nr_exchanged indicates how much work was done.
> + *
> + * Note that only PV guests can use this operation.
> */
> #define XENMEM_exchange 11
> struct xen_memory_exchange {
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 15:44 ` George Dunlap
@ 2017-06-21 15:54 ` Jan Beulich
2017-06-21 16:53 ` George Dunlap
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 15:54 UTC (permalink / raw)
To: George Dunlap
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
> When you have a long series like this, could you name the files in a way
> that makes it easier for a shell script to get their order? e.g.,
> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
>
> Having to manually save-and-apply the name of each patch of an
> eleven-patch series separately is fairly annoying. If they started with
> a number, I could save them all to the same directory and then use "for
> patch in *.patch ; do..." to apply them.
>
> (Using `git send-email` would also make things a lot easier...)
Well, that's kind of difficult without using git for development work.
I can try to remember to name patches suitably, but that's
another manual step for me then, and while I continue to attach
files I would have hoped that the mail bodies nowadays come
through uncorrupted (and hence I'd expect file names to be
chosen by your mail client based on subject, which includes
numbering - that's at least how mine behaves - and there being
no actual need to use the attachments).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 15:54 ` Jan Beulich
@ 2017-06-21 16:53 ` George Dunlap
2017-06-21 17:55 ` Stefano Stabellini
2017-06-22 6:59 ` Jan Beulich
0 siblings, 2 replies; 31+ messages in thread
From: George Dunlap @ 2017-06-21 16:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
On 21/06/17 16:54, Jan Beulich wrote:
>>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
>> When you have a long series like this, could you name the files in a way
>> that makes it easier for a shell script to get their order? e.g.,
>> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
>>
>> Having to manually save-and-apply the name of each patch of an
>> eleven-patch series separately is fairly annoying. If they started with
>> a number, I could save them all to the same directory and then use "for
>> patch in *.patch ; do..." to apply them.
>>
>> (Using `git send-email` would also make things a lot easier...)
>
> Well, that's kind of difficult without using git for development work.
> I can try to remember to name patches suitably, but that's
> another manual step for me then
Well, it's either an extra manual step for you, or an extra manual step
for every person who reviews your code. You don't need to justify to me
why you don't want to use git for development. However, it's not fair
to externalize the cost of your development preferences to everybody else.
When it's just one or two patches I'm willing to make some
accommodation, but a series like this it's too much.
> , and while I continue to attach
> files I would have hoped that the mail bodies nowadays come
> through uncorrupted (and hence I'd expect file names to be
> chosen by your mail client based on subject, which includes
> numbering - that's at least how mine behaves - and there being
> no actual need to use the attachments).
I don't have a simple way of saving the text of the mail message without
saving the attachment in-line in the file; so without writing a special
tool just for your messages, I can't easily apply the patches into the
git tree (so I can see the change in-situ).
It looks like the mail bodies might actually be uncorrupted now; so it's
possible that if you send the mail without attachments anymore I might
be able to use the same workflow for you as I use for everyone else
(which presumably would extend to other people who might want to review
your patches). If you send a test mail (or perhaps a series) I can give
it a try.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 16:53 ` George Dunlap
@ 2017-06-21 17:55 ` Stefano Stabellini
2017-06-22 6:59 ` Jan Beulich
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-06-21 17:55 UTC (permalink / raw)
To: George Dunlap
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Jan Beulich, xen-devel
On Wed, 21 Jun 2017, George Dunlap wrote:
> On 21/06/17 16:54, Jan Beulich wrote:
> >>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
> >> When you have a long series like this, could you name the files in a way
> >> that makes it easier for a shell script to get their order? e.g.,
> >> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
> >>
> >> Having to manually save-and-apply the name of each patch of an
> >> eleven-patch series separately is fairly annoying. If they started with
> >> a number, I could save them all to the same directory and then use "for
> >> patch in *.patch ; do..." to apply them.
> >>
> >> (Using `git send-email` would also make things a lot easier...)
> >
> > Well, that's kind of difficult without using git for development work.
> > I can try to remember to name patches suitably, but that's
> > another manual step for me then
>
> Well, it's either an extra manual step for you, or an extra manual step
> for every person who reviews your code. You don't need to justify to me
> why you don't want to use git for development. However, it's not fair
> to externalize the cost of your development preferences to everybody else.
>
> When it's just one or two patches I'm willing to make some
> accommodation, but a series like this it's too much.
I don't use git either (I use guilt). My solution to this problem is to
`guilt push' all my patches, then export them back as patches from the
commits using `git format-patch'.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-21 16:53 ` George Dunlap
2017-06-21 17:55 ` Stefano Stabellini
@ 2017-06-22 6:59 ` Jan Beulich
2017-06-22 9:52 ` George Dunlap
1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-22 6:59 UTC (permalink / raw)
To: George Dunlap
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
>>> On 21.06.17 at 18:53, <george.dunlap@citrix.com> wrote:
> On 21/06/17 16:54, Jan Beulich wrote:
>>>>> On 21.06.17 at 17:44, <george.dunlap@citrix.com> wrote:
>>> When you have a long series like this, could you name the files in a way
>>> that makes it easier for a shell script to get their order? e.g.,
>>> 01-xsa217-doc.patch, 02-gnttab-xfer-xenheap.patch, &c?
>>>
>>> Having to manually save-and-apply the name of each patch of an
>>> eleven-patch series separately is fairly annoying. If they started with
>>> a number, I could save them all to the same directory and then use "for
>>> patch in *.patch ; do..." to apply them.
>>>
>>> (Using `git send-email` would also make things a lot easier...)
>>
>> Well, that's kind of difficult without using git for development work.
>> I can try to remember to name patches suitably, but that's
>> another manual step for me then
>
> Well, it's either an extra manual step for you, or an extra manual step
> for every person who reviews your code.
That's a good point (which in fact I did think of while replying),
yet with one questionable aspect: Patch reviewing doesn't
generally mean patch application. Afaic, it's very rare that I
find it necessary to apply a patch in order to review it. Hence
I'm not convinced this is an extra manual step for everyone
looking at the patch.
> You don't need to justify to me
> why you don't want to use git for development. However, it's not fair
> to externalize the cost of your development preferences to everybody else.
>
> When it's just one or two patches I'm willing to make some
> accommodation, but a series like this it's too much.
As said - I'll try to remember next time.
>> , and while I continue to attach
>> files I would have hoped that the mail bodies nowadays come
>> through uncorrupted (and hence I'd expect file names to be
>> chosen by your mail client based on subject, which includes
>> numbering - that's at least how mine behaves - and there being
>> no actual need to use the attachments).
>
> I don't have a simple way of saving the text of the mail message without
> saving the attachment in-line in the file; so without writing a special
> tool just for your messages, I can't easily apply the patches into the
> git tree (so I can see the change in-situ).
Oh, okay. Mail programs seem to be even more different than so
far I did think they would be.
> It looks like the mail bodies might actually be uncorrupted now; so it's
> possible that if you send the mail without attachments anymore I might
> be able to use the same workflow for you as I use for everyone else
> (which presumably would extend to other people who might want to review
> your patches). If you send a test mail (or perhaps a series) I can give
> it a try.
Perhaps best if I try to remember to do this next time round when
Cc-ing you on (not to small) a patch (I don't think it really needs to
be a series for that purpose). Not having to attach patches would,
in the end, remove one manual step for me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/11] public: adjust documentation following XSA-217
2017-06-22 6:59 ` Jan Beulich
@ 2017-06-22 9:52 ` George Dunlap
0 siblings, 0 replies; 31+ messages in thread
From: George Dunlap @ 2017-06-22 9:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, xen-devel
On 22/06/17 07:59, Jan Beulich wrote:
>>> , and while I continue to attach
>>> files I would have hoped that the mail bodies nowadays come
>>> through uncorrupted (and hence I'd expect file names to be
>>> chosen by your mail client based on subject, which includes
>>> numbering - that's at least how mine behaves - and there being
>>> no actual need to use the attachments).
>>
>> I don't have a simple way of saving the text of the mail message without
>> saving the attachment in-line in the file; so without writing a special
>> tool just for your messages, I can't easily apply the patches into the
>> git tree (so I can see the change in-situ).
>
> Oh, okay. Mail programs seem to be even more different than so
> far I did think they would be.
Yes, I actually was surprised that I don't seem to be able to save the
messages without the attachments in my mailer.
>> It looks like the mail bodies might actually be uncorrupted now; so it's
>> possible that if you send the mail without attachments anymore I might
>> be able to use the same workflow for you as I use for everyone else
>> (which presumably would extend to other people who might want to review
>> your patches). If you send a test mail (or perhaps a series) I can give
>> it a try.
>
> Perhaps best if I try to remember to do this next time round when
> Cc-ing you on (not to small) a patch (I don't think it really needs to
> be a series for that purpose). Not having to attach patches would,
> in the end, remove one manual step for me.
Thanks.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 02/11] gnttab: remove redundant xenheap check from gnttab_transfer()
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
2017-06-21 9:30 ` [PATCH 01/11] public: adjust documentation following XSA-217 Jan Beulich
@ 2017-06-21 9:31 ` Jan Beulich
2017-06-21 11:28 ` Andrew Cooper
2017-06-21 9:32 ` [PATCH 03/11] make steal_page() return a proper error value Jan Beulich
` (8 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:31 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 693 bytes --]
The message isn't very useful, and the check is being done by
steal_page() anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,15 +1843,6 @@ gnttab_transfer(
}
page = mfn_to_page(mfn);
- if ( unlikely(is_xen_heap_page(page)) )
- {
- put_gfn(d, gop.mfn);
- gdprintk(XENLOG_INFO, "gnttab_transfer: xen frame %lx\n",
- (unsigned long)gop.mfn);
- gop.status = GNTST_bad_page;
- goto copyback;
- }
-
if ( steal_page(d, page, 0) < 0 )
{
put_gfn(d, gop.mfn);
[-- Attachment #2: gnttab-xfer-xenheap.patch --]
[-- Type: text/plain, Size: 752 bytes --]
gnttab: remove redundant xenheap check from gnttab_transfer()
The message isn't very useful, and the check is being done by
steal_page() anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,15 +1843,6 @@ gnttab_transfer(
}
page = mfn_to_page(mfn);
- if ( unlikely(is_xen_heap_page(page)) )
- {
- put_gfn(d, gop.mfn);
- gdprintk(XENLOG_INFO, "gnttab_transfer: xen frame %lx\n",
- (unsigned long)gop.mfn);
- gop.status = GNTST_bad_page;
- goto copyback;
- }
-
if ( steal_page(d, page, 0) < 0 )
{
put_gfn(d, gop.mfn);
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 03/11] make steal_page() return a proper error value
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
2017-06-21 9:30 ` [PATCH 01/11] public: adjust documentation following XSA-217 Jan Beulich
2017-06-21 9:31 ` [PATCH 02/11] gnttab: remove redundant xenheap check from gnttab_transfer() Jan Beulich
@ 2017-06-21 9:32 ` Jan Beulich
2017-06-21 11:39 ` Andrew Cooper
2017-06-22 10:01 ` Julien Grall
2017-06-21 9:33 ` [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains Jan Beulich
` (7 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:32 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall
[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]
... and use it where suitable (the tmem caller doesn't propagate an
error code). While it doesn't matter as much, also make donate_page()
follow suit on x86 (on ARM it already returns -ENOSYS).
Also move their declarations to common code and add __must_check.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1090,7 +1090,7 @@ int donate_page(struct domain *d, struct
int steal_page(
struct domain *d, struct page_info *page, unsigned int memflags)
{
- return -1;
+ return -EOPNOTSUPP;
}
int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4424,7 +4424,7 @@ int donate_page(
page_to_mfn(page), d->domain_id,
owner ? owner->domain_id : DOMID_INVALID,
page->count_info, page->u.inuse.type_info);
- return -1;
+ return -EINVAL;
}
int steal_page(
@@ -4435,7 +4435,7 @@ int steal_page(
const struct domain *owner = dom_xen;
if ( paging_mode_external(d) )
- return -1;
+ return -EOPNOTSUPP;
spin_lock(&d->page_alloc_lock);
@@ -4490,7 +4490,7 @@ int steal_page(
page_to_mfn(page), d->domain_id,
owner ? owner->domain_id : DOMID_INVALID,
page->count_info, page->u.inuse.type_info);
- return -1;
+ return -EINVAL;
}
static int __do_update_va_mapping(
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,10 +1843,10 @@ gnttab_transfer(
}
page = mfn_to_page(mfn);
- if ( steal_page(d, page, 0) < 0 )
+ if ( (rc = steal_page(d, page, 0)) < 0 )
{
put_gfn(d, gop.mfn);
- gop.status = GNTST_bad_page;
+ gop.status = rc == -EINVAL ? GNTST_bad_page : GNTST_general_error;
goto copyback;
}
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -566,10 +566,10 @@ static long memory_exchange(XEN_GUEST_HA
page = mfn_to_page(mfn);
- if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+ rc = steal_page(d, page, MEMF_no_refcount);
+ if ( unlikely(rc) )
{
put_gfn(d, gmfn + k);
- rc = -EINVAL;
goto fail;
}
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -322,11 +322,6 @@ static inline int relinquish_shared_page
/* Arch-specific portion of memory_op hypercall. */
long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-int steal_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-
#define domain_set_alloc_bitsize(d) ((void)0)
#define domain_clamp_alloc_bitsize(d, b) (b)
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -550,11 +550,6 @@ long subarch_memory_op(unsigned long cmd
int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
-int steal_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-
int map_ldt_shadow_page(unsigned int);
#define NIL(type) ((type *)-sizeof(type))
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,8 +567,12 @@ int xenmem_add_to_physmap_one(struct dom
union xen_add_to_physmap_batch_extra extra,
unsigned long idx, gfn_t gfn);
-/* Returns 0 on success, or negative on error. */
+/* Return 0 on success, or negative on error. */
int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
+int __must_check steal_page(struct domain *d, struct page_info *page,
+ unsigned int memflags);
+int __must_check donate_page(struct domain *d, struct page_info *page,
+ unsigned int memflags);
#define RAM_TYPE_CONVENTIONAL 0x00000001
#define RAM_TYPE_RESERVED 0x00000002
[-- Attachment #2: steal_page-retval.patch --]
[-- Type: text/plain, Size: 4338 bytes --]
make steal_page() return a proper error value
... and use it where suitable (the tmem caller doesn't propagate an
error code). While it doesn't matter as much, also make donate_page()
follow suit on x86 (on ARM it already returns -ENOSYS).
Also move their declarations to common code and add __must_check.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1090,7 +1090,7 @@ int donate_page(struct domain *d, struct
int steal_page(
struct domain *d, struct page_info *page, unsigned int memflags)
{
- return -1;
+ return -EOPNOTSUPP;
}
int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4424,7 +4424,7 @@ int donate_page(
page_to_mfn(page), d->domain_id,
owner ? owner->domain_id : DOMID_INVALID,
page->count_info, page->u.inuse.type_info);
- return -1;
+ return -EINVAL;
}
int steal_page(
@@ -4435,7 +4435,7 @@ int steal_page(
const struct domain *owner = dom_xen;
if ( paging_mode_external(d) )
- return -1;
+ return -EOPNOTSUPP;
spin_lock(&d->page_alloc_lock);
@@ -4490,7 +4490,7 @@ int steal_page(
page_to_mfn(page), d->domain_id,
owner ? owner->domain_id : DOMID_INVALID,
page->count_info, page->u.inuse.type_info);
- return -1;
+ return -EINVAL;
}
static int __do_update_va_mapping(
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1843,10 +1843,10 @@ gnttab_transfer(
}
page = mfn_to_page(mfn);
- if ( steal_page(d, page, 0) < 0 )
+ if ( (rc = steal_page(d, page, 0)) < 0 )
{
put_gfn(d, gop.mfn);
- gop.status = GNTST_bad_page;
+ gop.status = rc == -EINVAL ? GNTST_bad_page : GNTST_general_error;
goto copyback;
}
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -566,10 +566,10 @@ static long memory_exchange(XEN_GUEST_HA
page = mfn_to_page(mfn);
- if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+ rc = steal_page(d, page, MEMF_no_refcount);
+ if ( unlikely(rc) )
{
put_gfn(d, gmfn + k);
- rc = -EINVAL;
goto fail;
}
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -322,11 +322,6 @@ static inline int relinquish_shared_page
/* Arch-specific portion of memory_op hypercall. */
long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-int steal_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-
#define domain_set_alloc_bitsize(d) ((void)0)
#define domain_clamp_alloc_bitsize(d, b) (b)
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -550,11 +550,6 @@ long subarch_memory_op(unsigned long cmd
int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void));
int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void));
-int steal_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-int donate_page(
- struct domain *d, struct page_info *page, unsigned int memflags);
-
int map_ldt_shadow_page(unsigned int);
#define NIL(type) ((type *)-sizeof(type))
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,8 +567,12 @@ int xenmem_add_to_physmap_one(struct dom
union xen_add_to_physmap_batch_extra extra,
unsigned long idx, gfn_t gfn);
-/* Returns 0 on success, or negative on error. */
+/* Return 0 on success, or negative on error. */
int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
+int __must_check steal_page(struct domain *d, struct page_info *page,
+ unsigned int memflags);
+int __must_check donate_page(struct domain *d, struct page_info *page,
+ unsigned int memflags);
#define RAM_TYPE_CONVENTIONAL 0x00000001
#define RAM_TYPE_RESERVED 0x00000002
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 03/11] make steal_page() return a proper error value
2017-06-21 9:32 ` [PATCH 03/11] make steal_page() return a proper error value Jan Beulich
@ 2017-06-21 11:39 ` Andrew Cooper
2017-06-22 10:01 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 11:39 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
Ian Jackson, Julien Grall
On 21/06/17 10:32, Jan Beulich wrote:
> ... and use it where suitable (the tmem caller doesn't propagate an
> error code). While it doesn't matter as much, also make donate_page()
> follow suit on x86 (on ARM it already returns -ENOSYS).
>
> Also move their declarations to common code and add __must_check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 03/11] make steal_page() return a proper error value
2017-06-21 9:32 ` [PATCH 03/11] make steal_page() return a proper error value Jan Beulich
2017-06-21 11:39 ` Andrew Cooper
@ 2017-06-22 10:01 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-06-22 10:01 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
Hi,
On 21/06/17 10:32, Jan Beulich wrote:
> ... and use it where suitable (the tmem caller doesn't propagate an
> error code). While it doesn't matter as much, also make donate_page()
> follow suit on x86 (on ARM it already returns -ENOSYS).
>
> Also move their declarations to common code and add __must_check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
For the ARM bits:
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (2 preceding siblings ...)
2017-06-21 9:32 ` [PATCH 03/11] make steal_page() return a proper error value Jan Beulich
@ 2017-06-21 9:33 ` Jan Beulich
2017-06-21 11:41 ` Andrew Cooper
2017-06-21 9:34 ` [PATCH 05/11] evtchn: convert evtchn_port_is_*() to plain bool Jan Beulich
` (6 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:33 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 586 bytes --]
Both the XSA-217 fix and
lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
make this assumption, so let's enforce it.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
break;
}
- ret = xsm_set_target(XSM_HOOK, d, e);
+ ret = -EOPNOTSUPP;
+ if ( is_hvm_domain(e) )
+ ret = xsm_set_target(XSM_HOOK, d, e);
if ( ret ) {
put_domain(e);
break;
[-- Attachment #2: domctl-set-target-HVM.patch --]
[-- Type: text/plain, Size: 633 bytes --]
domctl: restrict DOMCTL_set_target to HVM domains
Both the XSA-217 fix and
lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
make this assumption, so let's enforce it.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
break;
}
- ret = xsm_set_target(XSM_HOOK, d, e);
+ ret = -EOPNOTSUPP;
+ if ( is_hvm_domain(e) )
+ ret = xsm_set_target(XSM_HOOK, d, e);
if ( ret ) {
put_domain(e);
break;
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains
2017-06-21 9:33 ` [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains Jan Beulich
@ 2017-06-21 11:41 ` Andrew Cooper
0 siblings, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 11:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Tim Deegan
On 21/06/17 10:33, Jan Beulich wrote:
> Both the XSA-217 fix and
> lists.xenproject.org/archives/html/xen-devel/2017-04/msg02945.html
> make this assumption, so let's enforce it.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>, although...
>
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1071,7 +1071,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
> break;
> }
>
> - ret = xsm_set_target(XSM_HOOK, d, e);
> + ret = -EOPNOTSUPP;
> + if ( is_hvm_domain(e) )
> + ret = xsm_set_target(XSM_HOOK, d, e);
> if ( ret ) {
... do you mind fixing this style while you are here?
~Andrew
> put_domain(e);
> break;
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 05/11] evtchn: convert evtchn_port_is_*() to plain bool
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (3 preceding siblings ...)
2017-06-21 9:33 ` [PATCH 04/11] domctl: restrict DOMCTL_set_target to HVM domains Jan Beulich
@ 2017-06-21 9:34 ` Jan Beulich
2017-06-21 11:46 ` Andrew Cooper
2017-06-21 9:35 ` [PATCH 06/11] ARM: simplify page type handling Jan Beulich
` (5 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:34 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 5203 bytes --]
... at once reducing overall source size by combining some statements
and constifying a few pointers.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/event_2l.c
+++ b/xen/common/event_2l.c
@@ -61,7 +61,7 @@ static void evtchn_2l_unmask(struct doma
}
}
-static bool_t evtchn_2l_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_pending(const struct domain *d, evtchn_port_t port)
{
unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
@@ -69,7 +69,7 @@ static bool_t evtchn_2l_is_pending(struc
return port < max_ports && test_bit(port, &shared_info(d, evtchn_pending));
}
-static bool_t evtchn_2l_is_masked(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_masked(const struct domain *d, evtchn_port_t port)
{
unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -19,7 +19,7 @@
#include <public/event_channel.h>
-static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
+static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
unsigned int port)
{
unsigned int p, w;
@@ -293,37 +293,25 @@ static void evtchn_fifo_unmask(struct do
evtchn_fifo_set_pending(v, evtchn);
}
-static bool_t evtchn_fifo_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_fifo_is_pending(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
-
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 0;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- return test_bit(EVTCHN_FIFO_PENDING, word);
+ return word && test_bit(EVTCHN_FIFO_PENDING, word);
}
-static bool_t evtchn_fifo_is_masked(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_masked(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 1;
-
- return test_bit(EVTCHN_FIFO_MASKED, word);
+ return !word || test_bit(EVTCHN_FIFO_MASKED, word);
}
-static bool_t evtchn_fifo_is_busy(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_busy(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
-
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 0;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- return test_bit(EVTCHN_FIFO_LINKED, word);
+ return word && test_bit(EVTCHN_FIFO_LINKED, word);
}
static int evtchn_fifo_set_priority(struct domain *d, struct evtchn *evtchn,
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -137,13 +137,13 @@ struct evtchn_port_ops {
void (*set_pending)(struct vcpu *v, struct evtchn *evtchn);
void (*clear_pending)(struct domain *d, struct evtchn *evtchn);
void (*unmask)(struct domain *d, struct evtchn *evtchn);
- bool_t (*is_pending)(struct domain *d, evtchn_port_t port);
- bool_t (*is_masked)(struct domain *d, evtchn_port_t port);
+ bool (*is_pending)(const struct domain *d, evtchn_port_t port);
+ bool (*is_masked)(const struct domain *d, evtchn_port_t port);
/*
* Is the port unavailable because it's still being cleaned up
* after being closed?
*/
- bool_t (*is_busy)(struct domain *d, evtchn_port_t port);
+ bool (*is_busy)(const struct domain *d, evtchn_port_t port);
int (*set_priority)(struct domain *d, struct evtchn *evtchn,
unsigned int priority);
void (*print_state)(struct domain *d, const struct evtchn *evtchn);
@@ -174,23 +174,23 @@ static inline void evtchn_port_unmask(st
d->evtchn_port_ops->unmask(d, evtchn);
}
-static inline bool_t evtchn_port_is_pending(struct domain *d,
- evtchn_port_t port)
+static inline bool evtchn_port_is_pending(const struct domain *d,
+ evtchn_port_t port)
{
return d->evtchn_port_ops->is_pending(d, port);
}
-static inline bool_t evtchn_port_is_masked(struct domain *d,
- evtchn_port_t port)
+static inline bool evtchn_port_is_masked(const struct domain *d,
+ evtchn_port_t port)
{
return d->evtchn_port_ops->is_masked(d, port);
}
-static inline bool_t evtchn_port_is_busy(struct domain *d, evtchn_port_t port)
+static inline bool evtchn_port_is_busy(const struct domain *d,
+ evtchn_port_t port)
{
- if ( d->evtchn_port_ops->is_busy )
- return d->evtchn_port_ops->is_busy(d, port);
- return 0;
+ return d->evtchn_port_ops->is_busy &&
+ d->evtchn_port_ops->is_busy(d, port);
}
static inline int evtchn_port_set_priority(struct domain *d,
[-- Attachment #2: evtchn-port_is-bool.patch --]
[-- Type: text/plain, Size: 5251 bytes --]
evtchn: convert evtchn_port_is_*() to plain bool
... at once reducing overall source size by combining some statements
and constifying a few pointers.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/event_2l.c
+++ b/xen/common/event_2l.c
@@ -61,7 +61,7 @@ static void evtchn_2l_unmask(struct doma
}
}
-static bool_t evtchn_2l_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_pending(const struct domain *d, evtchn_port_t port)
{
unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
@@ -69,7 +69,7 @@ static bool_t evtchn_2l_is_pending(struc
return port < max_ports && test_bit(port, &shared_info(d, evtchn_pending));
}
-static bool_t evtchn_2l_is_masked(struct domain *d, evtchn_port_t port)
+static bool evtchn_2l_is_masked(const struct domain *d, evtchn_port_t port)
{
unsigned int max_ports = BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d);
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -19,7 +19,7 @@
#include <public/event_channel.h>
-static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d,
+static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
unsigned int port)
{
unsigned int p, w;
@@ -293,37 +293,25 @@ static void evtchn_fifo_unmask(struct do
evtchn_fifo_set_pending(v, evtchn);
}
-static bool_t evtchn_fifo_is_pending(struct domain *d, evtchn_port_t port)
+static bool evtchn_fifo_is_pending(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
-
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 0;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- return test_bit(EVTCHN_FIFO_PENDING, word);
+ return word && test_bit(EVTCHN_FIFO_PENDING, word);
}
-static bool_t evtchn_fifo_is_masked(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_masked(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 1;
-
- return test_bit(EVTCHN_FIFO_MASKED, word);
+ return !word || test_bit(EVTCHN_FIFO_MASKED, word);
}
-static bool_t evtchn_fifo_is_busy(struct domain *d, evtchn_port_t port)
+static bool_t evtchn_fifo_is_busy(const struct domain *d, evtchn_port_t port)
{
- event_word_t *word;
-
- word = evtchn_fifo_word_from_port(d, port);
- if ( unlikely(!word) )
- return 0;
+ const event_word_t *word = evtchn_fifo_word_from_port(d, port);
- return test_bit(EVTCHN_FIFO_LINKED, word);
+ return word && test_bit(EVTCHN_FIFO_LINKED, word);
}
static int evtchn_fifo_set_priority(struct domain *d, struct evtchn *evtchn,
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -137,13 +137,13 @@ struct evtchn_port_ops {
void (*set_pending)(struct vcpu *v, struct evtchn *evtchn);
void (*clear_pending)(struct domain *d, struct evtchn *evtchn);
void (*unmask)(struct domain *d, struct evtchn *evtchn);
- bool_t (*is_pending)(struct domain *d, evtchn_port_t port);
- bool_t (*is_masked)(struct domain *d, evtchn_port_t port);
+ bool (*is_pending)(const struct domain *d, evtchn_port_t port);
+ bool (*is_masked)(const struct domain *d, evtchn_port_t port);
/*
* Is the port unavailable because it's still being cleaned up
* after being closed?
*/
- bool_t (*is_busy)(struct domain *d, evtchn_port_t port);
+ bool (*is_busy)(const struct domain *d, evtchn_port_t port);
int (*set_priority)(struct domain *d, struct evtchn *evtchn,
unsigned int priority);
void (*print_state)(struct domain *d, const struct evtchn *evtchn);
@@ -174,23 +174,23 @@ static inline void evtchn_port_unmask(st
d->evtchn_port_ops->unmask(d, evtchn);
}
-static inline bool_t evtchn_port_is_pending(struct domain *d,
- evtchn_port_t port)
+static inline bool evtchn_port_is_pending(const struct domain *d,
+ evtchn_port_t port)
{
return d->evtchn_port_ops->is_pending(d, port);
}
-static inline bool_t evtchn_port_is_masked(struct domain *d,
- evtchn_port_t port)
+static inline bool evtchn_port_is_masked(const struct domain *d,
+ evtchn_port_t port)
{
return d->evtchn_port_ops->is_masked(d, port);
}
-static inline bool_t evtchn_port_is_busy(struct domain *d, evtchn_port_t port)
+static inline bool evtchn_port_is_busy(const struct domain *d,
+ evtchn_port_t port)
{
- if ( d->evtchn_port_ops->is_busy )
- return d->evtchn_port_ops->is_busy(d, port);
- return 0;
+ return d->evtchn_port_ops->is_busy &&
+ d->evtchn_port_ops->is_busy(d, port);
}
static inline int evtchn_port_set_priority(struct domain *d,
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 06/11] ARM: simplify page type handling
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (4 preceding siblings ...)
2017-06-21 9:34 ` [PATCH 05/11] evtchn: convert evtchn_port_is_*() to plain bool Jan Beulich
@ 2017-06-21 9:35 ` Jan Beulich
2017-06-21 23:53 ` Stefano Stabellini
2017-06-21 9:36 ` [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one() Jan Beulich
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:35 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall
[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]
There's no need to have anything here on ARM other than the distinction
between writable and non-writable pages (and even that could likely be
eliminated, but with a more intrusive change). Limit type to a single
bit and drop pinned and validated flags altogether.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Compile tested only.
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
spin_lock(&d->page_alloc_lock);
/* The incremented type count pins as writable or read-only. */
- page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page);
- page->u.inuse.type_info |= PGT_validated | 1;
+ page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
page_set_owner(page, d);
smp_wmb(); /* install valid domain ptr before updating refcnt. */
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
+#ifdef _PGT_pinned
if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
put_page_and_type(page);
+#endif
/*
* With the lack of an IOMMU on some platforms, domains with DMA-capable
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -77,20 +77,12 @@ struct page_info
#define PG_shift(idx) (BITS_PER_LONG - (idx))
#define PG_mask(x, idx) (x ## UL << PG_shift(idx))
-#define PGT_none PG_mask(0, 4) /* no special uses of this page */
-#define PGT_writable_page PG_mask(7, 4) /* has writable mappings? */
-#define PGT_type_mask PG_mask(15, 4) /* Bits 28-31 or 60-63. */
-
- /* Owning guest has pinned this page to its current type? */
-#define _PGT_pinned PG_shift(5)
-#define PGT_pinned PG_mask(1, 5)
-
- /* Has this page been validated for use as its current type? */
-#define _PGT_validated PG_shift(6)
-#define PGT_validated PG_mask(1, 6)
+#define PGT_none PG_mask(0, 1) /* no special uses of this page */
+#define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
+#define PGT_type_mask PG_mask(1, 1) /* Bits 31 or 63. */
/* Count of uses of this frame as its current type. */
-#define PGT_count_width PG_shift(9)
+#define PGT_count_width PG_shift(2)
#define PGT_count_mask ((1UL<<PGT_count_width)-1)
/* Cleared when the owning guest 'frees' this page. */
[-- Attachment #2: ARM-page-types.patch --]
[-- Type: text/plain, Size: 2591 bytes --]
ARM: simplify page type handling
There's no need to have anything here on ARM other than the distinction
between writable and non-writable pages (and even that could likely be
eliminated, but with a more intrusive change). Limit type to a single
bit and drop pinned and validated flags altogether.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note: Compile tested only.
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
spin_lock(&d->page_alloc_lock);
/* The incremented type count pins as writable or read-only. */
- page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page);
- page->u.inuse.type_info |= PGT_validated | 1;
+ page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
page_set_owner(page, d);
smp_wmb(); /* install valid domain ptr before updating refcnt. */
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
+#ifdef _PGT_pinned
if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
put_page_and_type(page);
+#endif
/*
* With the lack of an IOMMU on some platforms, domains with DMA-capable
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -77,20 +77,12 @@ struct page_info
#define PG_shift(idx) (BITS_PER_LONG - (idx))
#define PG_mask(x, idx) (x ## UL << PG_shift(idx))
-#define PGT_none PG_mask(0, 4) /* no special uses of this page */
-#define PGT_writable_page PG_mask(7, 4) /* has writable mappings? */
-#define PGT_type_mask PG_mask(15, 4) /* Bits 28-31 or 60-63. */
-
- /* Owning guest has pinned this page to its current type? */
-#define _PGT_pinned PG_shift(5)
-#define PGT_pinned PG_mask(1, 5)
-
- /* Has this page been validated for use as its current type? */
-#define _PGT_validated PG_shift(6)
-#define PGT_validated PG_mask(1, 6)
+#define PGT_none PG_mask(0, 1) /* no special uses of this page */
+#define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
+#define PGT_type_mask PG_mask(1, 1) /* Bits 31 or 63. */
/* Count of uses of this frame as its current type. */
-#define PGT_count_width PG_shift(9)
+#define PGT_count_width PG_shift(2)
#define PGT_count_mask ((1UL<<PGT_count_width)-1)
/* Cleared when the owning guest 'frees' this page. */
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/11] ARM: simplify page type handling
2017-06-21 9:35 ` [PATCH 06/11] ARM: simplify page type handling Jan Beulich
@ 2017-06-21 23:53 ` Stefano Stabellini
0 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-06-21 23:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall, xen-devel
On Wed, 21 Jun 2017, Jan Beulich wrote:
> There's no need to have anything here on ARM other than the distinction
> between writable and non-writable pages (and even that could likely be
> eliminated, but with a more intrusive change). Limit type to a single
> bit and drop pinned and validated flags altogether.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Note: Compile tested only.
>
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1113,8 +1113,7 @@ void share_xen_page_with_guest(struct pa
> spin_lock(&d->page_alloc_lock);
>
> /* The incremented type count pins as writable or read-only. */
> - page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page);
> - page->u.inuse.type_info |= PGT_validated | 1;
> + page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
>
> page_set_owner(page, d);
> smp_wmb(); /* install valid domain ptr before updating refcnt. */
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -354,8 +354,10 @@ int guest_remove_page(struct domain *d,
>
> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>
> +#ifdef _PGT_pinned
> if ( !rc && test_and_clear_bit(_PGT_pinned, &page->u.inuse.type_info) )
> put_page_and_type(page);
> +#endif
>
> /*
> * With the lack of an IOMMU on some platforms, domains with DMA-capable
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -77,20 +77,12 @@ struct page_info
> #define PG_shift(idx) (BITS_PER_LONG - (idx))
> #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>
> -#define PGT_none PG_mask(0, 4) /* no special uses of this page */
> -#define PGT_writable_page PG_mask(7, 4) /* has writable mappings? */
> -#define PGT_type_mask PG_mask(15, 4) /* Bits 28-31 or 60-63. */
> -
> - /* Owning guest has pinned this page to its current type? */
> -#define _PGT_pinned PG_shift(5)
> -#define PGT_pinned PG_mask(1, 5)
> -
> - /* Has this page been validated for use as its current type? */
> -#define _PGT_validated PG_shift(6)
> -#define PGT_validated PG_mask(1, 6)
> +#define PGT_none PG_mask(0, 1) /* no special uses of this page */
> +#define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
> +#define PGT_type_mask PG_mask(1, 1) /* Bits 31 or 63. */
>
> /* Count of uses of this frame as its current type. */
> -#define PGT_count_width PG_shift(9)
> +#define PGT_count_width PG_shift(2)
> #define PGT_count_mask ((1UL<<PGT_count_width)-1)
>
> /* Cleared when the owning guest 'frees' this page. */
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one()
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (5 preceding siblings ...)
2017-06-21 9:35 ` [PATCH 06/11] ARM: simplify page type handling Jan Beulich
@ 2017-06-21 9:36 ` Jan Beulich
2017-06-21 11:53 ` Andrew Cooper
2017-06-21 9:36 ` [PATCH 08/11] gnttab: remove host map in the event of a grant_map failure Jan Beulich
` (3 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:36 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 533 bytes --]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
if ( !paging_mode_translate(d) || (mfn == 0) )
{
- if ( page )
- put_page(page);
- if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
- put_gfn(d, gfn);
- return -EINVAL;
+ rc = -EINVAL;
+ goto put_both;
}
/* Remove previously mapped page if it was present. */
[-- Attachment #2: x86-xatp1-error-paths.patch --]
[-- Type: text/plain, Size: 593 bytes --]
x86: fold identical error paths in xenmem_add_to_physmap_one()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
if ( !paging_mode_translate(d) || (mfn == 0) )
{
- if ( page )
- put_page(page);
- if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
- put_gfn(d, gfn);
- return -EINVAL;
+ rc = -EINVAL;
+ goto put_both;
}
/* Remove previously mapped page if it was present. */
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one()
2017-06-21 9:36 ` [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one() Jan Beulich
@ 2017-06-21 11:53 ` Andrew Cooper
0 siblings, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 11:53 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap
On 21/06/17 10:36, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
I think this function is another one which could benefit from explicitly
counting the number of references it collects, as per XSA-224.
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4899,11 +4899,8 @@ int xenmem_add_to_physmap_one(
>
> if ( !paging_mode_translate(d) || (mfn == 0) )
> {
> - if ( page )
> - put_page(page);
> - if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> - put_gfn(d, gfn);
> - return -EINVAL;
> + rc = -EINVAL;
> + goto put_both;
> }
>
> /* Remove previously mapped page if it was present. */
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 08/11] gnttab: remove host map in the event of a grant_map failure
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (6 preceding siblings ...)
2017-06-21 9:36 ` [PATCH 07/11] x86: fold identical error paths in xenmem_add_to_physmap_one() Jan Beulich
@ 2017-06-21 9:36 ` Jan Beulich
2017-06-21 9:37 ` [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
` (2 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]
From: George Dunlap <george.dunlap@citrix.com>
The current code appropriately removes the reference and type counts
on failure, but leaves the mapping set up. As the only path which can
trigger this is failure from IOMMU manipulation, and as unprivileged
domains are being crashed in that case, this is not by itself a
security issue.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -764,6 +764,7 @@ __gnttab_map_grant_ref(
u32 old_pin;
u32 act_pin;
unsigned int cache_flags, refcnt = 0, typecnt = 0;
+ bool host_map_created = false;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -923,6 +924,8 @@ __gnttab_map_grant_ref(
cache_flags);
if ( rc != GNTST_okay )
goto undo_out;
+
+ host_map_created = true;
}
}
else if ( owner == rd || owner == dom_cow )
@@ -960,6 +963,8 @@ __gnttab_map_grant_ref(
rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
if ( rc != GNTST_okay )
goto undo_out;
+
+ host_map_created = true;
}
}
else
@@ -1030,6 +1035,12 @@ __gnttab_map_grant_ref(
return;
undo_out:
+ if ( host_map_created )
+ {
+ replace_grant_host_mapping(op->host_addr, frame, 0, op->flags);
+ gnttab_flush_tlb(ld);
+ }
+
while ( typecnt-- )
put_page_type(pg);
[-- Attachment #2: gnttab-remove-host-map.patch --]
[-- Type: text/plain, Size: 1794 bytes --]
gnttab: remove host map in the event of a grant_map failure
From: George Dunlap <george.dunlap@citrix.com>
The current code appropriately removes the reference and type counts
on failure, but leaves the mapping set up. As the only path which can
trigger this is failure from IOMMU manipulation, and as unprivileged
domains are being crashed in that case, this is not by itself a
security issue.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -764,6 +764,7 @@ __gnttab_map_grant_ref(
u32 old_pin;
u32 act_pin;
unsigned int cache_flags, refcnt = 0, typecnt = 0;
+ bool host_map_created = false;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -923,6 +924,8 @@ __gnttab_map_grant_ref(
cache_flags);
if ( rc != GNTST_okay )
goto undo_out;
+
+ host_map_created = true;
}
}
else if ( owner == rd || owner == dom_cow )
@@ -960,6 +963,8 @@ __gnttab_map_grant_ref(
rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
if ( rc != GNTST_okay )
goto undo_out;
+
+ host_map_created = true;
}
}
else
@@ -1030,6 +1035,12 @@ __gnttab_map_grant_ref(
return;
undo_out:
+ if ( host_map_created )
+ {
+ replace_grant_host_mapping(op->host_addr, frame, 0, op->flags);
+ gnttab_flush_tlb(ld);
+ }
+
while ( typecnt-- )
put_page_type(pg);
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (7 preceding siblings ...)
2017-06-21 9:36 ` [PATCH 08/11] gnttab: remove host map in the event of a grant_map failure Jan Beulich
@ 2017-06-21 9:37 ` Jan Beulich
2017-06-21 12:02 ` Andrew Cooper
2017-06-21 9:38 ` [PATCH 10/11] gnttab: limit mapkind()'s iteration count Jan Beulich
2017-06-21 9:38 ` [PATCH 11/11] gnttab: drop useless locking Jan Beulich
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:37 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
When no memory is available in the hypervisor, rather than immediately
failing the request try to steal a handle from another vCPU.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -397,7 +397,7 @@ get_maptrack_handle(
struct vcpu *curr = current;
unsigned int i, head;
grant_handle_t handle;
- struct grant_mapping *new_mt;
+ struct grant_mapping *new_mt = NULL;
handle = __get_maptrack_handle(lgt, curr);
if ( likely(handle != -1) )
@@ -408,8 +408,13 @@ get_maptrack_handle(
/*
* If we've run out of frames, try stealing an entry from another
* VCPU (in case the guest isn't mapping across its VCPUs evenly).
+ * Also use this path in case we're out of memory, to avoid spurious
+ * failures.
*/
- if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
+ if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+ new_mt = alloc_xenheap_page();
+
+ if ( !new_mt )
{
/*
* Can drop the lock since no other VCPU can be adding a new
@@ -432,12 +437,6 @@ get_maptrack_handle(
return steal_maptrack_handle(lgt, curr);
}
- new_mt = alloc_xenheap_page();
- if ( !new_mt )
- {
- spin_unlock(&lgt->maptrack_lock);
- return -1;
- }
clear_page(new_mt);
/*
[-- Attachment #2: gnttab-spurious-mt-handle-fail.patch --]
[-- Type: text/plain, Size: 1550 bytes --]
gnttab: avoid spurious maptrack handle allocation failures
When no memory is available in the hypervisor, rather than immediately
failing the request try to steal a handle from another vCPU.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -397,7 +397,7 @@ get_maptrack_handle(
struct vcpu *curr = current;
unsigned int i, head;
grant_handle_t handle;
- struct grant_mapping *new_mt;
+ struct grant_mapping *new_mt = NULL;
handle = __get_maptrack_handle(lgt, curr);
if ( likely(handle != -1) )
@@ -408,8 +408,13 @@ get_maptrack_handle(
/*
* If we've run out of frames, try stealing an entry from another
* VCPU (in case the guest isn't mapping across its VCPUs evenly).
+ * Also use this path in case we're out of memory, to avoid spurious
+ * failures.
*/
- if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
+ if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
+ new_mt = alloc_xenheap_page();
+
+ if ( !new_mt )
{
/*
* Can drop the lock since no other VCPU can be adding a new
@@ -432,12 +437,6 @@ get_maptrack_handle(
return steal_maptrack_handle(lgt, curr);
}
- new_mt = alloc_xenheap_page();
- if ( !new_mt )
- {
- spin_unlock(&lgt->maptrack_lock);
- return -1;
- }
clear_page(new_mt);
/*
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures
2017-06-21 9:37 ` [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
@ 2017-06-21 12:02 ` Andrew Cooper
2017-06-21 12:19 ` Jan Beulich
2017-06-22 14:16 ` Jan Beulich
0 siblings, 2 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 12:02 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson
On 21/06/17 10:37, Jan Beulich wrote:
> When no memory is available in the hypervisor, rather than immediately
> failing the request try to steal a handle from another vCPU.
"request, try"
>
> Reported-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -397,7 +397,7 @@ get_maptrack_handle(
> struct vcpu *curr = current;
> unsigned int i, head;
> grant_handle_t handle;
> - struct grant_mapping *new_mt;
> + struct grant_mapping *new_mt = NULL;
>
> handle = __get_maptrack_handle(lgt, curr);
> if ( likely(handle != -1) )
> @@ -408,8 +408,13 @@ get_maptrack_handle(
> /*
> * If we've run out of frames, try stealing an entry from another
> * VCPU (in case the guest isn't mapping across its VCPUs evenly).
> + * Also use this path in case we're out of memory, to avoid spurious
> + * failures.
> */
> - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
> + if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
> + new_mt = alloc_xenheap_page();
> +
> + if ( !new_mt )
> {
> /*
> * Can drop the lock since no other VCPU can be adding a new
* frame once they've run out.
*/
It doesn't look like this comment is true any more, which brings the
locking correctness into question.
~Andrew
> @@ -432,12 +437,6 @@ get_maptrack_handle(
> return steal_maptrack_handle(lgt, curr);
> }
>
> - new_mt = alloc_xenheap_page();
> - if ( !new_mt )
> - {
> - spin_unlock(&lgt->maptrack_lock);
> - return -1;
> - }
> clear_page(new_mt);
>
> /*
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures
2017-06-21 12:02 ` Andrew Cooper
@ 2017-06-21 12:19 ` Jan Beulich
2017-06-22 14:16 ` Jan Beulich
1 sibling, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 12:19 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
Ian Jackson, xen-devel
>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 21/06/17 10:37, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -397,7 +397,7 @@ get_maptrack_handle(
>> struct vcpu *curr = current;
>> unsigned int i, head;
>> grant_handle_t handle;
>> - struct grant_mapping *new_mt;
>> + struct grant_mapping *new_mt = NULL;
>>
>> handle = __get_maptrack_handle(lgt, curr);
>> if ( likely(handle != -1) )
>> @@ -408,8 +408,13 @@ get_maptrack_handle(
>> /*
>> * If we've run out of frames, try stealing an entry from another
>> * VCPU (in case the guest isn't mapping across its VCPUs evenly).
>> + * Also use this path in case we're out of memory, to avoid spurious
>> + * failures.
>> */
>> - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
>> + if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
>> + new_mt = alloc_xenheap_page();
>> +
>> + if ( !new_mt )
>> {
>> /*
>> * Can drop the lock since no other VCPU can be adding a new
>
> * frame once they've run out.
> */
>
> It doesn't look like this comment is true any more, which brings the
> locking correctness into question.
Oh, indeed. I'll need to revive the locking change I had done here
and then dropped because we did realize we didn't need it for
XSA-218.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures
2017-06-21 12:02 ` Andrew Cooper
2017-06-21 12:19 ` Jan Beulich
@ 2017-06-22 14:16 ` Jan Beulich
1 sibling, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-06-22 14:16 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
Ian Jackson, xen-devel
>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 21/06/17 10:37, Jan Beulich wrote:
>> @@ -408,8 +408,13 @@ get_maptrack_handle(
>> /*
>> * If we've run out of frames, try stealing an entry from another
>> * VCPU (in case the guest isn't mapping across its VCPUs evenly).
>> + * Also use this path in case we're out of memory, to avoid spurious
>> + * failures.
>> */
>> - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames )
>> + if ( nr_maptrack_frames(lgt) < max_maptrack_frames )
>> + new_mt = alloc_xenheap_page();
>> +
>> + if ( !new_mt )
>> {
>> /*
>> * Can drop the lock since no other VCPU can be adding a new
>
> * frame once they've run out.
> */
>
> It doesn't look like this comment is true any more, which brings the
> locking correctness into question.
The whole function acts on current only, so races aren't possible
at all. The comment therefore is misleading, and I now think the
maptrack_lock is pointless altogether.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 10/11] gnttab: limit mapkind()'s iteration count
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (8 preceding siblings ...)
2017-06-21 9:37 ` [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures Jan Beulich
@ 2017-06-21 9:38 ` Jan Beulich
2017-06-21 12:13 ` Andrew Cooper
2017-06-21 9:38 ` [PATCH 11/11] gnttab: drop useless locking Jan Beulich
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:38 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]
There's no need for the function to observe increases of the maptrack
table (which can occur as the maptrack lock isn't being held) - actual
population of maptrack entries is excluded while we're here (by way of
holding the respective grant table lock for writing, while code
populating entries acquires it for reading). Latch the limit ahead of
the loop, allowing for the barrier to move out, too.
Signed-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -712,7 +712,7 @@ static unsigned int mapkind(
struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
{
struct grant_mapping *map;
- grant_handle_t handle;
+ grant_handle_t handle, limit = lgt->maptrack_limit;
unsigned int kind = 0;
/*
@@ -726,10 +726,10 @@ static unsigned int mapkind(
*/
ASSERT(percpu_rw_is_write_locked(&rd->grant_table->lock));
- for ( handle = 0; !(kind & MAPKIND_WRITE) &&
- handle < lgt->maptrack_limit; handle++ )
+ smp_rmb();
+
+ for ( handle = 0; !(kind & MAPKIND_WRITE) && handle < limit; handle++ )
{
- smp_rmb();
map = &maptrack_entry(lgt, handle);
if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
map->domid != rd->domain_id )
[-- Attachment #2: gnttab-mapkind-limit.patch --]
[-- Type: text/plain, Size: 1386 bytes --]
gnttab: limit mapkind()'s iteration count
There's no need for the function to observe increases of the maptrack
table (which can occur as the maptrack lock isn't being held) - actual
population of maptrack entries is excluded while we're here (by way of
holding the respective grant table lock for writing, while code
populating entries acquires it for reading). Latch the limit ahead of
the loop, allowing for the barrier to move out, too.
Signed-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -712,7 +712,7 @@ static unsigned int mapkind(
struct grant_table *lgt, const struct domain *rd, unsigned long mfn)
{
struct grant_mapping *map;
- grant_handle_t handle;
+ grant_handle_t handle, limit = lgt->maptrack_limit;
unsigned int kind = 0;
/*
@@ -726,10 +726,10 @@ static unsigned int mapkind(
*/
ASSERT(percpu_rw_is_write_locked(&rd->grant_table->lock));
- for ( handle = 0; !(kind & MAPKIND_WRITE) &&
- handle < lgt->maptrack_limit; handle++ )
+ smp_rmb();
+
+ for ( handle = 0; !(kind & MAPKIND_WRITE) && handle < limit; handle++ )
{
- smp_rmb();
map = &maptrack_entry(lgt, handle);
if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
map->domid != rd->domain_id )
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 10/11] gnttab: limit mapkind()'s iteration count
2017-06-21 9:38 ` [PATCH 10/11] gnttab: limit mapkind()'s iteration count Jan Beulich
@ 2017-06-21 12:13 ` Andrew Cooper
0 siblings, 0 replies; 31+ messages in thread
From: Andrew Cooper @ 2017-06-21 12:13 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson
On 21/06/17 10:38, Jan Beulich wrote:
> There's no need for the function to observe increases of the maptrack
> table (which can occur as the maptrack lock isn't being held) - actual
> population of maptrack entries is excluded while we're here (by way of
> holding the respective grant table lock for writing, while code
> populating entries acquires it for reading). Latch the limit ahead of
> the loop, allowing for the barrier to move out, too.
>
> Signed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 11/11] gnttab: drop useless locking
2017-06-21 9:25 [PATCH 00/11] assorted follow-ups to recent XSAs Jan Beulich
` (9 preceding siblings ...)
2017-06-21 9:38 ` [PATCH 10/11] gnttab: limit mapkind()'s iteration count Jan Beulich
@ 2017-06-21 9:38 ` Jan Beulich
2017-07-14 12:34 ` Ping: " Jan Beulich
10 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-06-21 9:38 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 945 bytes --]
Holding any lock while accessing the maptrack entry fields is
pointless, as these entries are protected by their associated active
entry lock (which is being acquired later, before re-validating the
fields read without holding the lock).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
smp_rmb();
map = &maptrack_entry(lgt, op->handle);
- grant_read_lock(lgt);
-
if ( unlikely(!read_atomic(&map->flags)) )
{
- grant_read_unlock(lgt);
gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
op->status = GNTST_bad_handle;
return;
}
dom = map->domid;
- grant_read_unlock(lgt);
-
if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
{
/* This can happen when a grant is implicitly unmapped. */
[-- Attachment #2: gnttab-useless-locking.patch --]
[-- Type: text/plain, Size: 971 bytes --]
gnttab: drop useless locking
Holding any lock while accessing the maptrack entry fields is
pointless, as these entries are protected by their associated active
entry lock (which is being acquired later, before re-validating the
fields read without holding the lock).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
smp_rmb();
map = &maptrack_entry(lgt, op->handle);
- grant_read_lock(lgt);
-
if ( unlikely(!read_atomic(&map->flags)) )
{
- grant_read_unlock(lgt);
gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
op->status = GNTST_bad_handle;
return;
}
dom = map->domid;
- grant_read_unlock(lgt);
-
if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
{
/* This can happen when a grant is implicitly unmapped. */
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Ping: [PATCH 11/11] gnttab: drop useless locking
2017-06-21 9:38 ` [PATCH 11/11] gnttab: drop useless locking Jan Beulich
@ 2017-07-14 12:34 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-07-14 12:34 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson,
Stefano Stabellini, Konrad Rzeszutek Wilk, Tim Deegan
Cc: xen-devel
>>> On 21.06.17 at 11:38, <JBeulich@suse.com> wrote:
> Holding any lock while accessing the maptrack entry fields is
> pointless, as these entries are protected by their associated active
> entry lock (which is being acquired later, before re-validating the
> fields read without holding the lock).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1122,19 +1122,14 @@ __gnttab_unmap_common(
> smp_rmb();
> map = &maptrack_entry(lgt, op->handle);
>
> - grant_read_lock(lgt);
> -
> if ( unlikely(!read_atomic(&map->flags)) )
> {
> - grant_read_unlock(lgt);
> gdprintk(XENLOG_INFO, "Zero flags for handle %#x\n", op->handle);
> op->status = GNTST_bad_handle;
> return;
> }
>
> dom = map->domid;
> - grant_read_unlock(lgt);
> -
> if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
> {
> /* This can happen when a grant is implicitly unmapped. */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread