* [PATCH] xen/vm_event: fix rc check for uninitialized ring
@ 2019-05-24 13:15 Juergen Gross
2019-05-24 13:15 ` [Xen-devel] " Juergen Gross
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 13:15 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
George Dunlap, Andrew Cooper, Jan Beulich, Roger Pau Monné
vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
lists on domain creation"), but the callers test for -ENOSYS.
Correct the callers.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/mm/p2m.c | 2 +-
xen/common/monitor.c | 2 +-
xen/common/vm_event.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 57c5eeda91..8a9a1153a8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
/* We're paging. There should be a ring */
int rc = vm_event_claim_slot(d, d->vm_event_paging);
- if ( rc == -ENOSYS )
+ if ( rc == -EOPNOTSUPP )
{
gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
"in place\n", d->domain_id, gfn_l);
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index cb5f37fdb2..d5c9ff1cbf 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req)
{
case 0:
break;
- case -ENOSYS:
+ case -EOPNOTSUPP:
/*
* If there was no ring to handle the event, then
* simply continue executing normally.
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 6e68be47bc..7b4ebced16 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
* this function will always return 0 for a guest. For a non-guest, we check
* for space and return -EBUSY if the ring is not available.
*
- * Return codes: -ENOSYS: the ring is not yet configured
+ * Return codes: -EOPNOTSUPP: the ring is not yet configured
* -EBUSY: the ring is busy
* 0: a spot has been reserved
*
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 13:15 [PATCH] xen/vm_event: fix rc check for uninitialized ring Juergen Gross
@ 2019-05-24 13:15 ` Juergen Gross
2019-05-24 14:05 ` Razvan Cojocaru
2019-05-24 14:11 ` Jan Beulich
2 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 13:15 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
George Dunlap, Andrew Cooper, Jan Beulich, Roger Pau Monné
vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
lists on domain creation"), but the callers test for -ENOSYS.
Correct the callers.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/mm/p2m.c | 2 +-
xen/common/monitor.c | 2 +-
xen/common/vm_event.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 57c5eeda91..8a9a1153a8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
/* We're paging. There should be a ring */
int rc = vm_event_claim_slot(d, d->vm_event_paging);
- if ( rc == -ENOSYS )
+ if ( rc == -EOPNOTSUPP )
{
gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
"in place\n", d->domain_id, gfn_l);
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index cb5f37fdb2..d5c9ff1cbf 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req)
{
case 0:
break;
- case -ENOSYS:
+ case -EOPNOTSUPP:
/*
* If there was no ring to handle the event, then
* simply continue executing normally.
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 6e68be47bc..7b4ebced16 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
* this function will always return 0 for a guest. For a non-guest, we check
* for space and return -EBUSY if the ring is not available.
*
- * Return codes: -ENOSYS: the ring is not yet configured
+ * Return codes: -EOPNOTSUPP: the ring is not yet configured
* -EBUSY: the ring is busy
* 0: a spot has been reserved
*
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 13:15 [PATCH] xen/vm_event: fix rc check for uninitialized ring Juergen Gross
2019-05-24 13:15 ` [Xen-devel] " Juergen Gross
@ 2019-05-24 14:05 ` Razvan Cojocaru
2019-05-24 14:05 ` [Xen-devel] " Razvan Cojocaru
2019-05-24 14:54 ` Juergen Gross
2019-05-24 14:11 ` Jan Beulich
2 siblings, 2 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2019-05-24 14:05 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Tamas K Lengyel, Wei Liu, George Dunlap, Andrew Cooper,
Jan Beulich, Roger Pau Monné
On 5/24/19 4:15 PM, Juergen Gross wrote:
> vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
> since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
> lists on domain creation"), but the callers test for -ENOSYS.
>
> Correct the callers.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> xen/arch/x86/mm/p2m.c | 2 +-
> xen/common/monitor.c | 2 +-
> xen/common/vm_event.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 57c5eeda91..8a9a1153a8 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>
> /* We're paging. There should be a ring */
> int rc = vm_event_claim_slot(d, d->vm_event_paging);
> - if ( rc == -ENOSYS )
> + if ( rc == -EOPNOTSUPP )
> {
> gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
> "in place\n", d->domain_id, gfn_l);
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index cb5f37fdb2..d5c9ff1cbf 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req)
> {
> case 0:
> break;
> - case -ENOSYS:
> + case -EOPNOTSUPP:
> /*
> * If there was no ring to handle the event, then
> * simply continue executing normally.
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 6e68be47bc..7b4ebced16 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
> * this function will always return 0 for a guest. For a non-guest, we check
> * for space and return -EBUSY if the ring is not available.
> *
> - * Return codes: -ENOSYS: the ring is not yet configured
> + * Return codes: -EOPNOTSUPP: the ring is not yet configured
> * -EBUSY: the ring is busy
> * 0: a spot has been reserved
> *
>
But vm_event_grab_slot() (which ends up being what vm_event_wait_slot()
returns), still returns -ENOSYS:
463 static int vm_event_grab_slot(struct vm_event_domain *ved, int foreign)
464 {
465 unsigned int avail_req;
466
467 if ( !ved->ring_page )
468 return -ENOSYS;
Although we can't get to that part if vm_event_check_ring() returns
false, we should probably return -EOPNOTSUPP from there as well. In
fact, I wonder if any of the -ENOSYS returns in vm_event.c should not be
replaced with return -EOPNOTSUPPs.
Anyway, the change does clearly improve the code even without settling
the above questions, so:
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:05 ` Razvan Cojocaru
@ 2019-05-24 14:05 ` Razvan Cojocaru
2019-05-24 14:54 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2019-05-24 14:05 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Tamas K Lengyel, Wei Liu, George Dunlap, Andrew Cooper,
Jan Beulich, Roger Pau Monné
On 5/24/19 4:15 PM, Juergen Gross wrote:
> vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
> since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
> lists on domain creation"), but the callers test for -ENOSYS.
>
> Correct the callers.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> xen/arch/x86/mm/p2m.c | 2 +-
> xen/common/monitor.c | 2 +-
> xen/common/vm_event.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 57c5eeda91..8a9a1153a8 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>
> /* We're paging. There should be a ring */
> int rc = vm_event_claim_slot(d, d->vm_event_paging);
> - if ( rc == -ENOSYS )
> + if ( rc == -EOPNOTSUPP )
> {
> gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
> "in place\n", d->domain_id, gfn_l);
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index cb5f37fdb2..d5c9ff1cbf 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req)
> {
> case 0:
> break;
> - case -ENOSYS:
> + case -EOPNOTSUPP:
> /*
> * If there was no ring to handle the event, then
> * simply continue executing normally.
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 6e68be47bc..7b4ebced16 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
> * this function will always return 0 for a guest. For a non-guest, we check
> * for space and return -EBUSY if the ring is not available.
> *
> - * Return codes: -ENOSYS: the ring is not yet configured
> + * Return codes: -EOPNOTSUPP: the ring is not yet configured
> * -EBUSY: the ring is busy
> * 0: a spot has been reserved
> *
>
But vm_event_grab_slot() (which ends up being what vm_event_wait_slot()
returns), still returns -ENOSYS:
463 static int vm_event_grab_slot(struct vm_event_domain *ved, int foreign)
464 {
465 unsigned int avail_req;
466
467 if ( !ved->ring_page )
468 return -ENOSYS;
Although we can't get to that part if vm_event_check_ring() returns
false, we should probably return -EOPNOTSUPP from there as well. In
fact, I wonder if any of the -ENOSYS returns in vm_event.c should not be
replaced with return -EOPNOTSUPPs.
Anyway, the change does clearly improve the code even without settling
the above questions, so:
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 13:15 [PATCH] xen/vm_event: fix rc check for uninitialized ring Juergen Gross
2019-05-24 13:15 ` [Xen-devel] " Juergen Gross
2019-05-24 14:05 ` Razvan Cojocaru
@ 2019-05-24 14:11 ` Jan Beulich
2019-05-24 14:11 ` [Xen-devel] " Jan Beulich
2019-05-24 14:57 ` Juergen Gross
2 siblings, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2019-05-24 14:11 UTC (permalink / raw)
To: Juergen Gross
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, xen-devel, Roger Pau Monne
>>> On 24.05.19 at 15:15, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>
> /* We're paging. There should be a ring */
> int rc = vm_event_claim_slot(d, d->vm_event_paging);
> - if ( rc == -ENOSYS )
> + if ( rc == -EOPNOTSUPP )
Perhaps while committing (or if a v2 should become necessary)
the missing blank line could be added here at the same time.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:11 ` Jan Beulich
@ 2019-05-24 14:11 ` Jan Beulich
2019-05-24 14:57 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-05-24 14:11 UTC (permalink / raw)
To: Juergen Gross
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, xen-devel, Roger Pau Monne
>>> On 24.05.19 at 15:15, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>
> /* We're paging. There should be a ring */
> int rc = vm_event_claim_slot(d, d->vm_event_paging);
> - if ( rc == -ENOSYS )
> + if ( rc == -EOPNOTSUPP )
Perhaps while committing (or if a v2 should become necessary)
the missing blank line could be added here at the same time.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:05 ` Razvan Cojocaru
2019-05-24 14:05 ` [Xen-devel] " Razvan Cojocaru
@ 2019-05-24 14:54 ` Juergen Gross
2019-05-24 14:54 ` [Xen-devel] " Juergen Gross
1 sibling, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 14:54 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel
Cc: Tamas K Lengyel, Wei Liu, George Dunlap, Andrew Cooper,
Jan Beulich, Roger Pau Monné
On 24/05/2019 16:05, Razvan Cojocaru wrote:
> On 5/24/19 4:15 PM, Juergen Gross wrote:
>> vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
>> since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
>> lists on domain creation"), but the callers test for -ENOSYS.
>>
>> Correct the callers.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> xen/arch/x86/mm/p2m.c | 2 +-
>> xen/common/monitor.c | 2 +-
>> xen/common/vm_event.c | 2 +-
>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 57c5eeda91..8a9a1153a8 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d,
>> unsigned long gfn_l)
>> /* We're paging. There should be a ring */
>> int rc = vm_event_claim_slot(d, d->vm_event_paging);
>> - if ( rc == -ENOSYS )
>> + if ( rc == -EOPNOTSUPP )
>> {
>> gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
>> "in place\n", d->domain_id, gfn_l);
>> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
>> index cb5f37fdb2..d5c9ff1cbf 100644
>> --- a/xen/common/monitor.c
>> +++ b/xen/common/monitor.c
>> @@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync,
>> vm_event_request_t *req)
>> {
>> case 0:
>> break;
>> - case -ENOSYS:
>> + case -EOPNOTSUPP:
>> /*
>> * If there was no ring to handle the event, then
>> * simply continue executing normally.
>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>> index 6e68be47bc..7b4ebced16 100644
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
>> * this function will always return 0 for a guest. For a non-guest,
>> we check
>> * for space and return -EBUSY if the ring is not available.
>> *
>> - * Return codes: -ENOSYS: the ring is not yet configured
>> + * Return codes: -EOPNOTSUPP: the ring is not yet configured
>> * -EBUSY: the ring is busy
>> * 0: a spot has been reserved
>> *
>>
>
> But vm_event_grab_slot() (which ends up being what vm_event_wait_slot()
> returns), still returns -ENOSYS:
>
> 463 static int vm_event_grab_slot(struct vm_event_domain *ved, int foreign)
> 464 {
> 465 unsigned int avail_req;
> 466
> 467 if ( !ved->ring_page )
> 468 return -ENOSYS;
>
> Although we can't get to that part if vm_event_check_ring() returns
> false, we should probably return -EOPNOTSUPP from there as well. In
Hmm, in case the page is removed while a vcpu is waiting for a slot
to become free ENOSYS should still be possible. There is, however, a
race in vm_event_grab_slot(): ved->ring_page is tested outside the
lock, so it could be zeroed just after the test occurred leading to
a "use after free" when calling vm_event_ring_available().
> fact, I wonder if any of the -ENOSYS returns in vm_event.c should not be
> replaced with return -EOPNOTSUPPs.
I believe the ones for the ring page not existing should be replaced,
while the ones for wrong hypercall sub-commands should either remain
or be replaced by EINVAL.
> Anyway, the change does clearly improve the code even without settling
> the above questions, so:
>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:54 ` Juergen Gross
@ 2019-05-24 14:54 ` Juergen Gross
0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 14:54 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel
Cc: Tamas K Lengyel, Wei Liu, George Dunlap, Andrew Cooper,
Jan Beulich, Roger Pau Monné
On 24/05/2019 16:05, Razvan Cojocaru wrote:
> On 5/24/19 4:15 PM, Juergen Gross wrote:
>> vm_event_claim_slot() returns -EOPNOTSUPP for an uninitialized ring
>> since commit 15e4dd5e866b43bbc ("common/vm_event: Initialize vm_event
>> lists on domain creation"), but the callers test for -ENOSYS.
>>
>> Correct the callers.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> xen/arch/x86/mm/p2m.c | 2 +-
>> xen/common/monitor.c | 2 +-
>> xen/common/vm_event.c | 2 +-
>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 57c5eeda91..8a9a1153a8 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d,
>> unsigned long gfn_l)
>> /* We're paging. There should be a ring */
>> int rc = vm_event_claim_slot(d, d->vm_event_paging);
>> - if ( rc == -ENOSYS )
>> + if ( rc == -EOPNOTSUPP )
>> {
>> gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
>> "in place\n", d->domain_id, gfn_l);
>> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
>> index cb5f37fdb2..d5c9ff1cbf 100644
>> --- a/xen/common/monitor.c
>> +++ b/xen/common/monitor.c
>> @@ -98,7 +98,7 @@ int monitor_traps(struct vcpu *v, bool sync,
>> vm_event_request_t *req)
>> {
>> case 0:
>> break;
>> - case -ENOSYS:
>> + case -EOPNOTSUPP:
>> /*
>> * If there was no ring to handle the event, then
>> * simply continue executing normally.
>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>> index 6e68be47bc..7b4ebced16 100644
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -513,7 +513,7 @@ bool vm_event_check_ring(struct vm_event_domain *ved)
>> * this function will always return 0 for a guest. For a non-guest,
>> we check
>> * for space and return -EBUSY if the ring is not available.
>> *
>> - * Return codes: -ENOSYS: the ring is not yet configured
>> + * Return codes: -EOPNOTSUPP: the ring is not yet configured
>> * -EBUSY: the ring is busy
>> * 0: a spot has been reserved
>> *
>>
>
> But vm_event_grab_slot() (which ends up being what vm_event_wait_slot()
> returns), still returns -ENOSYS:
>
> 463 static int vm_event_grab_slot(struct vm_event_domain *ved, int foreign)
> 464 {
> 465 unsigned int avail_req;
> 466
> 467 if ( !ved->ring_page )
> 468 return -ENOSYS;
>
> Although we can't get to that part if vm_event_check_ring() returns
> false, we should probably return -EOPNOTSUPP from there as well. In
Hmm, in case the page is removed while a vcpu is waiting for a slot
to become free ENOSYS should still be possible. There is, however, a
race in vm_event_grab_slot(): ved->ring_page is tested outside the
lock, so it could be zeroed just after the test occurred leading to
a "use after free" when calling vm_event_ring_available().
> fact, I wonder if any of the -ENOSYS returns in vm_event.c should not be
> replaced with return -EOPNOTSUPPs.
I believe the ones for the ring page not existing should be replaced,
while the ones for wrong hypercall sub-commands should either remain
or be replaced by EINVAL.
> Anyway, the change does clearly improve the code even without settling
> the above questions, so:
>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:11 ` Jan Beulich
2019-05-24 14:11 ` [Xen-devel] " Jan Beulich
@ 2019-05-24 14:57 ` Juergen Gross
2019-05-24 14:57 ` [Xen-devel] " Juergen Gross
1 sibling, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 14:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, xen-devel, Roger Pau Monne
On 24/05/2019 16:11, Jan Beulich wrote:
>>>> On 24.05.19 at 15:15, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>>
>> /* We're paging. There should be a ring */
>> int rc = vm_event_claim_slot(d, d->vm_event_paging);
>> - if ( rc == -ENOSYS )
>> + if ( rc == -EOPNOTSUPP )
>
> Perhaps while committing (or if a v2 should become necessary)
> the missing blank line could be added here at the same time.
I'll send a V2 with the two missing ENOSYS replacements.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
2019-05-24 14:57 ` Juergen Gross
@ 2019-05-24 14:57 ` Juergen Gross
0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2019-05-24 14:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, George Dunlap,
Andrew Cooper, xen-devel, Roger Pau Monne
On 24/05/2019 16:11, Jan Beulich wrote:
>>>> On 24.05.19 at 15:15, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1705,7 +1705,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
>>
>> /* We're paging. There should be a ring */
>> int rc = vm_event_claim_slot(d, d->vm_event_paging);
>> - if ( rc == -ENOSYS )
>> + if ( rc == -EOPNOTSUPP )
>
> Perhaps while committing (or if a v2 should become necessary)
> the missing blank line could be added here at the same time.
I'll send a V2 with the two missing ENOSYS replacements.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-24 14:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 13:15 [PATCH] xen/vm_event: fix rc check for uninitialized ring Juergen Gross
2019-05-24 13:15 ` [Xen-devel] " Juergen Gross
2019-05-24 14:05 ` Razvan Cojocaru
2019-05-24 14:05 ` [Xen-devel] " Razvan Cojocaru
2019-05-24 14:54 ` Juergen Gross
2019-05-24 14:54 ` [Xen-devel] " Juergen Gross
2019-05-24 14:11 ` Jan Beulich
2019-05-24 14:11 ` [Xen-devel] " Jan Beulich
2019-05-24 14:57 ` Juergen Gross
2019-05-24 14:57 ` [Xen-devel] " Juergen Gross
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).