Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Longpeng (Mike)]

在 2020/3/10 16:23, Michael S. Tsirkin 写道:
> On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud 
> Infrastructure Service Product Dept.) wrote:
>>
>>
>> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
>>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
>>>> From: Longpeng <address@hidden>
>>>>
>>>> vhost_log_alloc() may fails and returned pointer of log is null.
>>>> However there're two places derefernce the return pointer without
>>>> check.
>>>>
[...]

>>>>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t 
>>>> size)
>>>>  {
>>>> -    struct vhost_log *log = vhost_log_get(size, 
>>>> vhost_dev_log_is_shared(dev));
>>>> -    uint64_t log_base = (uintptr_t)log->log;
>>>> +    struct vhost_log *log;
>>>> +    uint64_t log_base;
>>>>      int r;
>>>>  
>>>> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>>>> +    if (!log) {
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> I'm not sure silently failing like this is safe. Callers assume
>>> log can be resized. What can be done? I suspect not much
>>> beside exiting ...
>>> Speaking of which, lots of other failures in log resizing
>>> path seem to be silently ignored.
>>> I guess we should propagate them, and fix callers to check
>>> the return code?
>>>
>> How about to let the callers treat the failure of log_resize as a fatal 
>> error ?
>>
[...]

>>
>> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
>>  #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
>>      /* To log more, must increase log size before table update. */
>>      if (dev->log_size < log_size) {
>> -        vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
>> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> +            abort();
>> +        }
>>      }
>>      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
>>      if (r < 0) {
>> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
>>      }
>>      /* To log less, can only decrease log size after table update. */
>>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
>> -        vhost_dev_log_resize(dev, log_size);
>> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> +            abort();
>> +        }
>>      }
>>
>>  out:
> 
> 
> I think the suggested handling is
>       error_report() and exit().
> we also need to propagate errno. So how about passing in Error then?
> 
vhost_dev_log_resize
  vhost_log_get
    vhost_log_alloc
      error_report_err (fail path, errno is in the errp)
  VHOST_OPS_DEBUG (if ->vhost_set_log_base fail)
      error_report (errno)

Um, it seems log_resize will report error with errno internal, do we need
error_report once more ?

> 
>> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
>> int enable)
>>          }
>>          vhost_log_put(dev, false);
>>      } else {
>> -        vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> +        r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> +        if (r < 0) {
>> +            return r;
>> +        }
>> +
>>          r = vhost_dev_set_log(dev, true);
>>          if (r < 0) {
>>              return r;
>>
>>
>>>
>>> .
>>>
>>
>> -- 
>> ---
>> Regards,
>> Longpeng(Mike)
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)