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(-)
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
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
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
>>> 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
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
© 2016 - 2024 Red Hat, Inc.