Re: [PATCH v3 2/2] vhost: return bool from *_access_ok() functions

[Michael S. Tsirkin]

On Wed, Apr 11, 2018 at 10:35:41AM +0800, Stefan Hajnoczi wrote:
> Currently vhost *_access_ok() functions return int.  This is error-prone
> because there are two popular conventions:
> 
> 1. 0 means failure, 1 means success
> 2. -errno means failure, 0 means success
> 
> Although vhost mostly uses #1, it does not do so consistently.
> umem_access_ok() uses #2.
> 
> This patch changes the return type from int to bool so that false means
> failure and true means success.  This eliminates a potential source of
> errors.
> 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Stefan Hajnoczi 

Acked-by: Michael S. Tsirkin 

> ---
>  drivers/vhost/vhost.h |  4 ++--
>  drivers/vhost/vhost.c | 66 
> +--
>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d8ee85ae8fdc..6c844b90a168 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
>  void vhost_dev_stop(struct vhost_dev *);
>  long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user 
> *argp);
>  long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user 
> *argp);
> -int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> -int vhost_log_access_ok(struct vhost_dev *);
> +bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
> +bool vhost_log_access_ok(struct vhost_dev *);
>  
>  int vhost_get_vq_desc(struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fc805b7fad9d..0fcb51a9940c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
>  
> -static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
> +static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
>  {
>   u64 a = addr / VHOST_PAGE_SIZE / 8;
>  
>   /* Make sure 64 bit math will not overflow. */
>   if (a > ULONG_MAX - (unsigned long)log_base ||
>   a + (unsigned long)log_base > ULONG_MAX)
> - return 0;
> + return false;
>  
>   return access_ok(VERIFY_WRITE, log_base + a,
>(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
> @@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
>  }
>  
>  /* Caller should have vq mutex and device mutex. */
> -static int vq_memory_access_ok(void __user *log_base, struct vhost_umem 
> *umem,
> -int log_all)
> +static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem 
> *umem,
> + int log_all)
>  {
>   struct vhost_umem_node *node;
>  
>   if (!umem)
> - return 0;
> + return false;
>  
>   list_for_each_entry(node, >umem_list, link) {
>   unsigned long a = node->userspace_addr;
>  
>   if (vhost_overflow(node->userspace_addr, node->size))
> - return 0;
> + return false;
>  
>  
>   if (!access_ok(VERIFY_WRITE, (void __user *)a,
>   node->size))
> - return 0;
> + return false;
>   else if (log_all && !log_access_ok(log_base,
>  node->start,
>  node->size))
> - return 0;
> + return false;
>   }
> - return 1;
> + return true;
>  }
>  
>  static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
> @@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct 
> vhost_virtqueue *vq,
>  
>  /* Can we switch to this memory table? */
>  /* Caller should have device mutex but not vq mutex */
> -static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> - int log_all)
> +static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> +  int log_all)
>  {
>   int i;
>  
>   for (i = 0; i < d->nvqs; ++i) {
> - int ok;
> + bool ok;
>   bool log;
>  
>   mutex_lock(>vqs[i]->mutex);
> @@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct 
> vhost_umem *umem,
>   ok = vq_memory_access_ok(d->vqs[i]->log_base,
>umem, log);
>   else
> - ok = 1;
> + ok = true;
>   mutex_unlock(>vqs[i]->mutex);
>   if (!ok)
> - return 0;
> + return false;
>   }
> - return 

Re: [PATCH v3 2/2] vhost: return bool from *_access_ok() functions

[Michael S. Tsirkin]

On Wed, Apr 11, 2018 at 10:35:41AM +0800, Stefan Hajnoczi wrote:
> Currently vhost *_access_ok() functions return int.  This is error-prone
> because there are two popular conventions:
> 
> 1. 0 means failure, 1 means success
> 2. -errno means failure, 0 means success
> 
> Although vhost mostly uses #1, it does not do so consistently.
> umem_access_ok() uses #2.
> 
> This patch changes the return type from int to bool so that false means
> failure and true means success.  This eliminates a potential source of
> errors.
> 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Stefan Hajnoczi 

Acked-by: Michael S. Tsirkin 

> ---
>  drivers/vhost/vhost.h |  4 ++--
>  drivers/vhost/vhost.c | 66 
> +--
>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d8ee85ae8fdc..6c844b90a168 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
>  void vhost_dev_stop(struct vhost_dev *);
>  long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user 
> *argp);
>  long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user 
> *argp);
> -int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> -int vhost_log_access_ok(struct vhost_dev *);
> +bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
> +bool vhost_log_access_ok(struct vhost_dev *);
>  
>  int vhost_get_vq_desc(struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fc805b7fad9d..0fcb51a9940c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
>  
> -static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
> +static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
>  {
>   u64 a = addr / VHOST_PAGE_SIZE / 8;
>  
>   /* Make sure 64 bit math will not overflow. */
>   if (a > ULONG_MAX - (unsigned long)log_base ||
>   a + (unsigned long)log_base > ULONG_MAX)
> - return 0;
> + return false;
>  
>   return access_ok(VERIFY_WRITE, log_base + a,
>(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
> @@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
>  }
>  
>  /* Caller should have vq mutex and device mutex. */
> -static int vq_memory_access_ok(void __user *log_base, struct vhost_umem 
> *umem,
> -int log_all)
> +static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem 
> *umem,
> + int log_all)
>  {
>   struct vhost_umem_node *node;
>  
>   if (!umem)
> - return 0;
> + return false;
>  
>   list_for_each_entry(node, >umem_list, link) {
>   unsigned long a = node->userspace_addr;
>  
>   if (vhost_overflow(node->userspace_addr, node->size))
> - return 0;
> + return false;
>  
>  
>   if (!access_ok(VERIFY_WRITE, (void __user *)a,
>   node->size))
> - return 0;
> + return false;
>   else if (log_all && !log_access_ok(log_base,
>  node->start,
>  node->size))
> - return 0;
> + return false;
>   }
> - return 1;
> + return true;
>  }
>  
>  static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
> @@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct 
> vhost_virtqueue *vq,
>  
>  /* Can we switch to this memory table? */
>  /* Caller should have device mutex but not vq mutex */
> -static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> - int log_all)
> +static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> +  int log_all)
>  {
>   int i;
>  
>   for (i = 0; i < d->nvqs; ++i) {
> - int ok;
> + bool ok;
>   bool log;
>  
>   mutex_lock(>vqs[i]->mutex);
> @@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct 
> vhost_umem *umem,
>   ok = vq_memory_access_ok(d->vqs[i]->log_base,
>umem, log);
>   else
> - ok = 1;
> + ok = true;
>   mutex_unlock(>vqs[i]->mutex);
>   if (!ok)
> - return 0;
> + return false;
>   }
> - return 1;
> + return true;
>  }
>  
>  static int translate_desc(struct