Re: [Qemu-devel] [PATCH v4] vl: exit if maxcpus is negative

[seeteena <s1seetee@linux.vnet.ibm.com>]

On 08/29/2017 07:15 PM, Philippe Mathieu-Daudé wrote:
> Hi Seeteena,
>
> On 08/29/2017 02:45 AM, Seeteena Thoufeek wrote:
>> ---Steps to Reproduce---
>>
>> When passed a negative number to 'maxcpus' parameter, Qemu aborts
>> with a core dump.
>>
>> Run the following command with maxcpus argument as negative number
>>
>> ppc64-softmmu/qemu-system-ppc64 --nographic -vga none -machine
>> pseries,accel=kvm,kvm-type=HV -m size=200g -device virtio-blk-pci,
>> drive=rootdisk -drive file=/home/images/pegas-1.0-ppc64le.qcow2,
>> if=none,cache=none,id=rootdisk,format=qcow2 -monitor telnet
>> :127.0.0.1:1234,server,nowait -net nic,model=virtio -net
>> user -redir tcp:2000::22 -device nec-usb-xhci -smp 8,cores=1,
>> threads=1,maxcpus=-12
>>
>> (process:12149): GLib-ERROR **: gmem.c:130: failed to allocate
>>   18446744073709550568 bytes
>>
>> Trace/breakpoint trap
>>
>> Reported-by: R.Nageswara Sastry <rnsastry@linux.vnet.ibm.com>
>> Signed-off-by: Seeteena Thoufeek <s1seetee@linux.vnet.ibm.com>
>> ---
>> v1 -> v2:
>>    - Fix the error check in vl.c to make it generic.
>> v2 -> v3:
>>    - Fix coding style pointed out by patchew.
>>    - Fix check for "<= 0" instead of just "< 0".
>> v3 -> v4:
>>    - Fix subject line.
>>    - Removed space before ":" from vl.c:1248
>>    - Removed Reviewed-by: flag.
>> ---
>>   vl.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 8e247cc..2d9e73d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1244,7 +1244,10 @@ static void smp_parse(QemuOpts *opts)
>>           }
>>             max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
>> -
>> +        if (max_cpus <= 0) {
>> +            error_report("Invalid max_cpus: %d", max_cpus);
>
> I disagree with this patch, I think the correct fix is to declare 
> max_cpus as unsigned.
> Looking at the codebase I can't find any signed use of it.
    if I declare max_cpus as unsigned, the error check is no more valid 
as the value max_cpus fetches is of unsigned type and hence we cannot do 
this below check.
                                                     if (max_cpus <= 0) {
error_report("Invalid max_cpus: %d", max_cpus);

When I remove the error check with max_cpus defined as unsigned, the 
code behaves as follows when negetive value is passed for maxcpus
    ppc64-softmmu/qemu-system-ppc64 --nographic -vga none -machine 
pseries,accel=kvm,kvm-type=HV -m size=20g -device 
virtio-blk-pci,drive=rootdisk -drive 
file=/var/lib/libvirt/images/avocado-fvt-wrapper/data/avocado-vt/images/ubuntu-17.10-ppc64le.qcow2,if=none,cache=none,id=rootdisk,format=qcow2 
-monitor telnet:127.0.0.1:1234,server,nowait -net nic,model=virtio -net 
user -device nec-usb-xhci -smp 8,cores=1,threads=1,maxcpus=-12


SLOF **********************************************************************
QEMU Starting
  Build Date = Jan  4 2017 05:15:48
  FW Version = buildd@ release 20161019
  Press "s" to enter Open Firmware.

>
>> +            exit(1);
>> +        }
>>           if (max_cpus < cpus) {
>>               error_report("maxcpus must be equal to or greater than 
>> smp");
>>               exit(1);
>>
>
> Regards,
>
> Phil.
>