| Message ID | 20190524131522.29170-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/vm_event: fix rc check for uninitialized ring | expand |
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
>>> 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
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
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
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 *
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(-)