Re: [PATCH 04/15] nvdimm: support nvdimm label

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 03/17/2016 06:28 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 17, 2016 at 04:32:50PM +0800, Xiao Guangrong wrote:
>> +static void nvdimm_init(Object *obj)
>> +{
>> +    object_property_add_bool(obj, "reserve-label", nvdimm_get_reserve_label,
>> +                             nvdimm_set_reserve_label, NULL);
>
> In the future users may wish for larger namespace label sizes.  This
> bool option will not allow that.
>
> Perhaps the option should be an integer called "label-size"?

Yes, good to me.

>
>> +static void nvdimm_assert_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size,
>> +                                        uint64_t offset)
>> +{
>> +    assert(nvdimm->reserve_label &&
>> +           (nvdimm->label_size >= size + offset) && (offset + size > offset));
>> +}
>
> It's not clear from this patch alone, but QEMU is not allowed to assert
> due to invalid inputs from the guest.  So if input validation is
> necessary here because the values may be invalid, please write if
> statements and error returns.

The caller should check it before calling these callbacks, in our case, we did
it in nvdimm_rw_label_data_check() in patch 13.

So if that happen, it is really a QEMU internal BUG.

>
> This is important so guests cannot cause QEMU to core dump (SIGABRT
> default behavior) and so that nested virtualization doesn't allow a
> nested guest to DoS its parent guest.


Yes, i understood it, but it is not the case in this patch as the assert()
can not be triggered by guest.

Maybe i should mention it in the changelog to make this fact more clean.