| Message ID | 20200224064219.1434-3-longpeng2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix some warnings by static code scan tool | expand |
Hi Michael, ping... On 2020/2/24 14:42, Longpeng(Mike) wrote: > From: Longpeng <longpeng2@huawei.com> > > vhost_log_alloc() may fails and returned pointer of log is null. > However there're two places derefernce the return pointer without > check. > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9edfadc..c7ad6e5 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > > if (!log || log->size != size) { > log = vhost_log_alloc(size, share); > + if (!log) { > + return NULL; > + } > + > if (share) { > vhost_log_shm = log; > } else { > @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > > 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; > + } > + > + log_base = (uintptr_t)log->log; > + > /* inform backend of log switching, this must be done before > releasing the current log, to ensure no logging is lost */ > r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); > @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > hdev->log_size = vhost_get_log_size(hdev); > hdev->log = vhost_log_get(hdev->log_size, > vhost_dev_log_is_shared(hdev)); > + if (!hdev->log) { > + goto fail_vq; > + } > + > log_base = (uintptr_t)hdev->log->log; > r = hdev->vhost_ops->vhost_set_log_base(hdev, > hdev->log_size ? log_base : 0, >
On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote: > From: Longpeng <longpeng2@huawei.com> > > vhost_log_alloc() may fails and returned pointer of log is null. > However there're two places derefernce the return pointer without > check. > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9edfadc..c7ad6e5 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > > if (!log || log->size != size) { > log = vhost_log_alloc(size, share); > + if (!log) { > + return NULL; > + } > + > if (share) { > vhost_log_shm = log; > } else { > @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > > 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?
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 <longpeng2@huawei.com> >> >> vhost_log_alloc() may fails and returned pointer of log is null. >> However there're two places derefernce the return pointer without >> check. >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> hw/virtio/vhost.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 9edfadc..c7ad6e5 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) >> >> if (!log || log->size != size) { >> log = vhost_log_alloc(size, share); >> + if (!log) { >> + return NULL; >> + } >> + >> if (share) { >> vhost_log_shm = log; >> } else { >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) >> >> 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 ? -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) +static inline int 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) { + r = -1; + goto out; + } + + log_base = (uintptr_t)log->log; + /* inform backend of log switching, this must be done before releasing the current log, to ensure no logging is lost */ r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) vhost_log_put(dev, true); dev->log = log; dev->log_size = size; + +out: + return 0; } @@ -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: @@ -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; > > . >
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 <longpeng2@huawei.com> > >> > >> vhost_log_alloc() may fails and returned pointer of log is null. > >> However there're two places derefernce the return pointer without > >> check. > >> > >> Signed-off-by: Longpeng <longpeng2@huawei.com> > >> --- > >> hw/virtio/vhost.c | 19 +++++++++++++++++-- > >> 1 file changed, 17 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >> index 9edfadc..c7ad6e5 100644 > >> --- a/hw/virtio/vhost.c > >> +++ b/hw/virtio/vhost.c > >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > >> > >> if (!log || log->size != size) { > >> log = vhost_log_alloc(size, share); > >> + if (!log) { > >> + return NULL; > >> + } > >> + > >> if (share) { > >> vhost_log_shm = log; > >> } else { > >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > >> > >> 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 ? > > -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > +static inline int 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) { > + r = -1; > + goto out; > + } > + > + log_base = (uintptr_t)log->log; > + > /* inform backend of log switching, this must be done before > releasing the current log, to ensure no logging is lost */ > r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); > @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev > *dev, uint64_t size) > vhost_log_put(dev, true); > dev->log = log; > dev->log_size = size; > + > +out: > + return 0; > } > > > @@ -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? > @@ -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)
在 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 <longpeng2@huawei.com> >>>> >>>> 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) > > > . >
On Tue, Mar 10, 2020 at 08:02:30PM +0800, Longpeng (Mike) wrote: > 在 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 <longpeng2@huawei.com> > >>>> > >>>> 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 ? Well we need to convert this over to something other than VHOST_OPS_DEBUG, that will go away at some point. > > > >> @@ -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)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 9edfadc..c7ad6e5 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) if (!log || log->size != size) { log = vhost_log_alloc(size, share); + if (!log) { + return NULL; + } + if (share) { vhost_log_shm = log; } else { @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) 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; + } + + log_base = (uintptr_t)log->log; + /* inform backend of log switching, this must be done before releasing the current log, to ensure no logging is lost */ r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log); @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) hdev->log_size = vhost_get_log_size(hdev); hdev->log = vhost_log_get(hdev->log_size, vhost_dev_log_is_shared(hdev)); + if (!hdev->log) { + goto fail_vq; + } + log_base = (uintptr_t)hdev->log->log; r = hdev->vhost_ops->vhost_set_log_base(hdev, hdev->log_size ? log_base : 0,