Fix cpumap setting before passing to XEN

It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64
CPUS, it just shows below error.

[root@localhost /]# xm vcpu-pin 3 all all
Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument')

The fix make it same as in xl code.

Signed-off-by Zhenzhong Duan <[email protected]>

---
 tools/python/xen/lowlevel/xc/xc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

tools/python/xen/lowlevel/xc/xc.c +3 -1

index c40a4e9..2f4898d 100644
@@ -243,13 +243,15 @@ static PyObject *pyxc_vcpu_setaffinity(XcObject *self,
         for ( i = 0; i < PyList_Size(cpulist); i++ )
         {
             long cpu = PyInt_AsLong(PyList_GetItem(cpulist, i));
-            if ( cpu < 0 || cpu >= nr_cpus )
+            if ( cpu < 0 )
             {
                 free(cpumap);
                 errno = EINVAL;
                 PyErr_SetFromErrno(xc_error_obj);
                 return NULL;
             }
+            if ( cpu >= nr_cpus )
+                continue;
             cpumap[cpu / 8] |= 1 << (cpu % 8);
         }
     }

Signature · 7 lines

1.7.3


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 11 lines · Zhenzhong Duan

It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64
CPUS, it just shows below error.

[root@localhost /]# xm vcpu-pin 3 all all
Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument')

This error clearly shows that the upper layer is not passing down the right cpumap. So this example along won't convince me to that this is a bug in python binding. It's free to reject input that it deems wrong.

I guess the question is more or less what should it do in such situation. So ...

Quoted 2 lines

The fix make it same as in xl code.

Is there reference to this? I guess the commit message or mail thread will shed more light on the expected behaviour.

Quoted 16 lines · Wei Liu

Quoted 11 lines · Zhenzhong Duan

It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64
CPUS, it just shows below error.

[root@localhost /]# xm vcpu-pin 3 all all
Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument')

This error clearly shows that the upper layer is not passing down the right cpumap. So this example along won't convince me to that this is a bug in python binding. It's free to reject input that it deems wrong.

Quoted 7 lines

I guess the question is more or less what should it do in such situation. So ...

Quoted 2 lines

The fix make it same as in xl code.

Is there reference to this? I guess the commit message or mail thread will shed more light on the expected behaviour.

Calling path:
main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set

void libxl_bitmap_set(libxl_bitmap *bitmap, int bit)

{
    if (bit >= bitmap->size * 8)
        return;
    bitmap->map[bit / 8] |= 1 << (bit & 7);
}

I referenced above code. It just ignore the cpu number beyond the max CPU count rather than raise an error.

Quoted 16 lines · Wei Liu

Quoted 11 lines · Zhenzhong Duan

It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64
CPUS, it just shows below error.

[root@localhost /]# xm vcpu-pin 3 all all
Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument')

This error clearly shows that the upper layer is not passing down the right cpumap. So this example along won't convince me to that this is a bug in python binding. It's free to reject input that it deems wrong.

Quoted 7 lines

I guess the question is more or less what should it do in such situation. So ...

Quoted 2 lines

The fix make it same as in xl code.

Is there reference to this? I guess the commit message or mail thread will shed more light on the expected behaviour.

Calling path:
main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set

void libxl_bitmap_set(libxl_bitmap *bitmap, int bit)

{
    if (bit >= bitmap->size * 8)
        return;
    bitmap->map[bit / 8] |= 1 << (bit & 7);
}

I referenced above code. It just ignore the cpu number beyond the max CPU count rather than raise an error.

thanks
zduan

Sorry I was at the Xen hackathon in the last two days.

Quoted 28 lines · Zhenzhong Duan

Quoted 16 lines · Wei Liu

Quoted 11 lines · Zhenzhong Duan

It's tool's duty to pass a correct cpumap to XEN. On a host with less than 64
CPUS, it just shows below error.

[root@localhost /]# xm vcpu-pin 3 all all
Error: Cannot pin vcpu: 0 to cpu: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63] - (22, 'Invalid argument')

This error clearly shows that the upper layer is not passing down the right cpumap. So this example along won't convince me to that this is a bug in python binding. It's free to reject input that it deems wrong.

Quoted 7 lines

I guess the question is more or less what should it do in such situation. So ...

Quoted 2 lines

The fix make it same as in xl code.

Is there reference to this? I guess the commit message or mail thread will shed more light on the expected behaviour.

Calling path:
main_vcpupin->cpurange_parse->update_cpumap_range->libxl_bitmap_set

void libxl_bitmap_set(libxl_bitmap *bitmap, int bit)

Quoted 9 lines

{
    if (bit >= bitmap->size * 8)
        return;
    bitmap->map[bit / 8] |= 1 << (bit & 7);
}

I referenced above code. It just ignore the cpu number beyond the max CPU count rather than raise an error.

In principle I think having python binding and xl/libxl behave more or less the same is the right direction. I'm a bit nervous about the change of behaviour on the other hand.

Let's wait for a few more days to see if other people have any comment on this.

Wei.

Quoted 2 lines

thanks
zduan

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 6 lines

In principle I think having python binding and xl/libxl behave more or less the same is the right direction. I'm a bit nervous about the change of behaviour on the other hand.

Let's wait for a few more days to see if other people have any comment on this.

Reviewed-by Konrad Rzeszutek Wilk <[email protected]> ?

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"):

Quoted 8 lines · Wei Liu

Quoted 6 lines

In principle I think having python binding and xl/libxl behave more or less the same is the right direction. I'm a bit nervous about the change of behaviour on the other hand.

Let's wait for a few more days to see if other people have any comment on this.

Reviewed-by Konrad Rzeszutek Wilk <[email protected]> ?

Does this bug report mean that `xm vcpu-pin ... all' has never worked properly ? Can that really be the case ?

Also, xm exists in Xen 4.4 and earlier, only. Xen 4.4 is no longer supported upstream, so we would not apply this patch to Xen 4.4. So whatever we do, this is not going to fix any bug in `xm vcpu-pin' in 4.4.

This doesn't necessarily mean that I object to changing the behaviour of the python xc module in still-supported Xen releases. But I'm not sure the reasoning behind the behaviour of the libxl bitmap functions applies to the Python interface.

Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ?

Quoted 11 lines · Ian Jackson

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"):

Quoted 8 lines · Wei Liu

Quoted 6 lines

In principle I think having python binding and xl/libxl behave more or less the same is the right direction. I'm a bit nervous about the change of behaviour on the other hand.

Let's wait for a few more days to see if other people have any comment on this.

Reviewed-by Konrad Rzeszutek Wilk <[email protected]> ?

Does this bug report mean that `xm vcpu-pin ... all' has never worked properly ? Can that really be the case ?

Xen 4.3 doesn't work, Xen 3.4 works.
I have no Xen 4.4 around to test that, but checked code, it will not. Then I found below commit involved.

commit 41abbadef60e5fccdfd688579dd458f7f7887cf5

Author: Ian Jackson <[email protected]>
Date:   Wed May 29 15:48:11 2013 +0100

libxc: limit cpu values when setting vcpu affinity

When support for pinning more than 64 cpus was added, check for cpu out-of-range values was removed. This can lead to subsequent out-of-bounds cpumap array accesses in case the cpu number is higher than the actual count.

This patch returns the check.

This is CVE-2013-2072 / XSA-56

Signed-off-by Petr Matousek <[email protected]>

Quoted 1 line

Quoted 4 lines

Also, xm exists in Xen 4.4 and earlier, only. Xen 4.4 is no longer supported upstream, so we would not apply this patch to Xen 4.4. So whatever we do, this is not going to fix any bug in `xm vcpu-pin' in 4.4.

The only impact is upper layer or the user need to pass a correct cpumap param not beyond the real cpu map to avoid the error.
But I am not clear if python binding is still used or will be removed just as Xend.

Quoted 7 lines

This doesn't necessarily mean that I object to changing the behaviour of the python xc module in still-supported Xen releases. But I'm not sure the reasoning behind the behaviour of the libxl bitmap functions applies to the Python interface.

Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ?

I am using xen-4.3.0-55.el6.47.33 which is Xen 4.3 variant

thanks
zduan

So what is the conclusion of this discussion so far? I admit I'm a bit lost here.

Wei Liu writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN"):

Quoted 2 lines

So what is the conclusion of this discussion so far? I admit I'm a bit lost here.

Undoubtedly:

* That "xm ..." generates the reported error is definitely a bug.

* "xm" exists only in versions of Xen now no longer supported upstream.

* Applying the proposed patch to libxc in upstream supported versions of Xen will not fix "xm" for users like Zhenzhong Duan.

There are two possible views about the nature of the bug:

1. It is xm's fault for passing an invalid cpumap.

2. It is python xc's fault for failing to tolerate a cpumap with bits set which do not correspond to actual cpus.

In the case 1., the xm python code needs to be changed. But there is nothing for upstream to do because none of our supported trees contain this code any more.

In the case 2., the in-tree python xc code should be changed.

I am somewhat reluctant to go down the path implied by case 2, because we don't know what collateral damage there may be. OTOH the proposed patch is one which tolerates a wider range of inputs, which is fairly safe.

I think that the behaviour of libxl (which has to provide a C API, rather than a python one) is a poor guide to the best behaviour for python xc.

So I still don't have a clear view about whether the patch should be applied. (I think it clearly shouldn't be backported.)

Ian.