[Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring

Juergen Gross posted 1 patch 4 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190524131522.29170-1-jgross@suse.com
There is a newer version of this series
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(-)
[Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
Posted by Juergen Gross 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
Posted by Razvan Cojocaru 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
Posted by Juergen Gross 4 years, 10 months ago
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
Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH] xen/vm_event: fix rc check for uninitialized ring
Posted by Juergen Gross 4 years, 10 months ago
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