Linux Virtualization - Nov 2016 - [PATCH 1/3] tools/virtio: fix READ_ONCE()

For several reasons, it would be beneficial to kill off ACCESS_ONCE()
tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate types,
more obviously document their intended behaviour, and are necessary for tools
like KTSAN to work correctly (as otherwise reads and writes cannot be
instrumented separately).

While it's possible to script the bulk of this tree-wide conversion, some
cases
such as the virtio code, require some manual intervention. This series moves
the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process fixing a
bug in the virtio headers.

Thanks,
Mark.

Mark Rutland (3):
  tools/virtio: fix READ_ONCE()
  vringh: kill off ACCESS_ONCE()
  tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h

 drivers/vhost/vringh.c        | 5 +++--
 tools/virtio/linux/compiler.h | 2 +-
 tools/virtio/linux/uaccess.h  | 9 +++++----
 3 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.7.4

The virtio tools implementation of READ_ONCE() has a single parameter called
'var', but erroneously refers to 'val' for its cast, and thus
won't work unless
there's a variable of the correct type that happens to be called
'var'.

Fix this with s/var/val/, making READ_ONCE() work as expected regardless.

Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
Signed-off-by: Mark Rutland <mark.rutland at arm.com>
Cc: Jason Wang <jasowang at redhat.com>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: linux-kernel at vger.kernel.org
Cc: virtualization at lists.linux-foundation.org
---
 tools/virtio/linux/compiler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
index 845960e..c9ccfd4 100644
--- a/tools/virtio/linux/compiler.h
+++ b/tools/virtio/linux/compiler.h
@@ -4,6 +4,6 @@
 #define WRITE_ONCE(var, val) \
 	(*((volatile typeof(val) *)(&(var))) = (val))
 
-#define READ_ONCE(var) (*((volatile typeof(val) *)(&(var))))
+#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
 
 #endif
-- 
2.7.4

Despite living under drivers/ vringh.c is also used as part of the userspace
virtio tools. Before we can kill off the ACCESS_ONCE()definition in the tools,
we must convert vringh.c to use {READ,WRITE}_ONCE().

This patch does so, along with the required include of <linux/compiler.h>
for
the relevant definitions. The userspace tools provide their own definitions in
their own <linux/compiler.h>.

Signed-off-by: Mark Rutland <mark.rutland at arm.com>
Cc: Jason Wang <jasowang at redhat.com>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: kvm at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
Cc: netdev at vger.kernel.org
Cc: virtualization at lists.linux-foundation.org
---
 drivers/vhost/vringh.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 3bb02c6..bb8971f 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -3,6 +3,7 @@
  *
  * Since these may be in userspace, we use (inline) accessors.
  */
+#include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/vringh.h>
 #include <linux/virtio_ring.h>
@@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
 static inline int getu16_kern(const struct vringh *vrh,
 			      u16 *val, const __virtio16 *p)
 {
-	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
+	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
 	return 0;
 }
 
 static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p, u16 val)
 {
-	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
+	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
 	return 0;
 }
 
-- 
2.7.4

As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
virtio tools uaccess primitives, pulling these in from <linux/compiler.h>.

With this done, we can kill off the now-unused ACCESS_ONCE() definition.

Signed-off-by: Mark Rutland <mark.rutland at arm.com>
Cc: Jason Wang <jasowang at redhat.com>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: linux-kernel at vger.kernel.org
Cc: virtualization at lists.linux-foundation.org
---
 tools/virtio/linux/uaccess.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h
index 0a578fe..fa05d01 100644
--- a/tools/virtio/linux/uaccess.h
+++ b/tools/virtio/linux/uaccess.h
@@ -1,8 +1,9 @@
 #ifndef UACCESS_H
 #define UACCESS_H
-extern void *__user_addr_min, *__user_addr_max;
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#include <linux/compiler.h>
+
+extern void *__user_addr_min, *__user_addr_max;
 
 static inline void __chk_user_ptr(const volatile void *p, size_t size)
 {
@@ -13,7 +14,7 @@ static inline void __chk_user_ptr(const volatile void *p,
size_t size)
 ({								\
 	typeof(ptr) __pu_ptr = (ptr);				\
 	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
-	ACCESS_ONCE(*(__pu_ptr)) = x;				\
+	WRITE_ONCE(*(__pu_ptr), x);				\
 	0;							\
 })
 
@@ -21,7 +22,7 @@ static inline void __chk_user_ptr(const volatile void *p,
size_t size)
 ({								\
 	typeof(ptr) __pu_ptr = (ptr);				\
 	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
-	x = ACCESS_ONCE(*(__pu_ptr));				\
+	x = READ_ONCE(*(__pu_ptr));				\
 	0;							\
 })
 
-- 
2.7.4

On 11/24/2016 11:25 AM, Mark Rutland wrote:
> Despite living under drivers/ vringh.c is also used as part of the
userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the
tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
> 
> This patch does so, along with the required include of
<linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions
in
> their own <linux/compiler.h>.
> 
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: kvm at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: netdev at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>  drivers/vhost/vringh.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 3bb02c6..bb8971f 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -3,6 +3,7 @@
>   *
>   * Since these may be in userspace, we use (inline) accessors.
>   */
> +#include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/vringh.h>
>  #include <linux/virtio_ring.h>
> @@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
>  static inline int getu16_kern(const struct vringh *vrh,
>  			      u16 *val, const __virtio16 *p)
>  {
> -	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
> +	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
>  	return 0;
>  }
> 
>  static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p, u16
val)
>  {
> -	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
> +	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
>  	return 0;
>  }
> 
Makes sense

Reviewed-by: Christian Borntraeger <borntraeger at de.ibm.com>

On Thu, 24 Nov 2016 10:25:12 +0000
Mark Rutland <mark.rutland at arm.com> wrote:
> The virtio tools implementation of READ_ONCE() has a single parameter
called
> 'var', but erroneously refers to 'val' for its cast, and
thus won't work unless
> there's a variable of the correct type that happens to be called
'var'.
> 
> Fix this with s/var/val/, making READ_ONCE() work as expected regardless.
> 
> Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: linux-kernel at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>  tools/virtio/linux/compiler.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>

On Thu, 24 Nov 2016 10:25:13 +0000
Mark Rutland <mark.rutland at arm.com> wrote:
> Despite living under drivers/ vringh.c is also used as part of the
userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the
tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
> 
> This patch does so, along with the required include of
<linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions
in
> their own <linux/compiler.h>.
> 
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: kvm at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: netdev at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>  drivers/vhost/vringh.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>

On Thu, 24 Nov 2016 10:25:14 +0000
Mark Rutland <mark.rutland at arm.com> wrote:
> As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
> virtio tools uaccess primitives, pulling these in from
<linux/compiler.h>.
> 
> With this done, we can kill off the now-unused ACCESS_ONCE() definition.
> 
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: linux-kernel at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>  tools/virtio/linux/uaccess.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>

On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland
wrote:
> For several reasons, it would be beneficial to kill off ACCESS_ONCE()
> tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate
types,
> more obviously document their intended behaviour, and are necessary for
tools
> like KTSAN to work correctly (as otherwise reads and writes cannot be
> instrumented separately).
> 
> While it's possible to script the bulk of this tree-wide conversion,
some cases
> such as the virtio code, require some manual intervention. This series
moves
> the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process
fixing a
> bug in the virtio headers.
> 
> Thanks,
> Mark.
I don't have a problem with this specific patchset.

Though I really question the whole _ONCE APIs esp with
aggregate types - these seem to generate a memcpy and
an 8-byte read/writes sometimes, and I'm pretty sure this simply
can't be read/written at once on all architectures.

So I worry it's kind of like volatile in this respect,
too easy to overuse.

> Mark Rutland (3):
>   tools/virtio: fix READ_ONCE()
>   vringh: kill off ACCESS_ONCE()
>   tools/virtio: use {READ,WRITE}_ONCE() in uaccess.h
> 
>  drivers/vhost/vringh.c        | 5 +++--
>  tools/virtio/linux/compiler.h | 2 +-
>  tools/virtio/linux/uaccess.h  | 9 +++++----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> -- 
> 2.7.4

On 2016?11?24? 18:25, Mark Rutland wrote:
> The virtio tools implementation of READ_ONCE() has a single parameter
called
> 'var', but erroneously refers to 'val' for its cast, and
thus won't work unless
> there's a variable of the correct type that happens to be called
'var'.
>
> Fix this with s/var/val/, making READ_ONCE() work as expected regardless.
>
> Fixes: a7c490333df3cff5 ("tools/virtio: use virt_xxx barriers")
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: linux-kernel at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>   tools/virtio/linux/compiler.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h
> index 845960e..c9ccfd4 100644
> --- a/tools/virtio/linux/compiler.h
> +++ b/tools/virtio/linux/compiler.h
> @@ -4,6 +4,6 @@
>   #define WRITE_ONCE(var, val) \
>   	(*((volatile typeof(val) *)(&(var))) = (val))
>   
> -#define READ_ONCE(var) (*((volatile typeof(val) *)(&(var))))
> +#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
>   
>   #endif
Reviewed-by: Jason Wang <jasowang at redhat.com>

On 2016?11?24? 18:25, Mark Rutland wrote:
> Despite living under drivers/ vringh.c is also used as part of the
userspace
> virtio tools. Before we can kill off the ACCESS_ONCE()definition in the
tools,
> we must convert vringh.c to use {READ,WRITE}_ONCE().
>
> This patch does so, along with the required include of
<linux/compiler.h> for
> the relevant definitions. The userspace tools provide their own definitions
in
> their own <linux/compiler.h>.
>
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: kvm at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: netdev at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>   drivers/vhost/vringh.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 3bb02c6..bb8971f 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -3,6 +3,7 @@
>    *
>    * Since these may be in userspace, we use (inline) accessors.
>    */
> +#include <linux/compiler.h>
>   #include <linux/module.h>
>   #include <linux/vringh.h>
>   #include <linux/virtio_ring.h>
> @@ -820,13 +821,13 @@ EXPORT_SYMBOL(vringh_need_notify_user);
>   static inline int getu16_kern(const struct vringh *vrh,
>   			      u16 *val, const __virtio16 *p)
>   {
> -	*val = vringh16_to_cpu(vrh, ACCESS_ONCE(*p));
> +	*val = vringh16_to_cpu(vrh, READ_ONCE(*p));
>   	return 0;
>   }
>   
>   static inline int putu16_kern(const struct vringh *vrh, __virtio16 *p,
u16 val)
>   {
> -	ACCESS_ONCE(*p) = cpu_to_vringh16(vrh, val);
> +	WRITE_ONCE(*p, cpu_to_vringh16(vrh, val));
>   	return 0;
>   }
>   
Reviewed-by: Jason Wang <jasowang at redhat.com>

On 2016?11?24? 18:25, Mark Rutland wrote:
> As a step towards killing off ACCESS_ONCE, use {READ,WRITE}_ONCE() for the
> virtio tools uaccess primitives, pulling these in from
<linux/compiler.h>.
>
> With this done, we can kill off the now-unused ACCESS_ONCE() definition.
>
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: linux-kernel at vger.kernel.org
> Cc: virtualization at lists.linux-foundation.org
> ---
>   tools/virtio/linux/uaccess.h | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h
> index 0a578fe..fa05d01 100644
> --- a/tools/virtio/linux/uaccess.h
> +++ b/tools/virtio/linux/uaccess.h
> @@ -1,8 +1,9 @@
>   #ifndef UACCESS_H
>   #define UACCESS_H
> -extern void *__user_addr_min, *__user_addr_max;
>   
> -#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> +#include <linux/compiler.h>
> +
> +extern void *__user_addr_min, *__user_addr_max;
>   
>   static inline void __chk_user_ptr(const volatile void *p, size_t size)
>   {
> @@ -13,7 +14,7 @@ static inline void __chk_user_ptr(const volatile void *p,
size_t size)
>   ({								\
>   	typeof(ptr) __pu_ptr = (ptr);				\
>   	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
> -	ACCESS_ONCE(*(__pu_ptr)) = x;				\
> +	WRITE_ONCE(*(__pu_ptr), x);				\
>   	0;							\
>   })
>   
> @@ -21,7 +22,7 @@ static inline void __chk_user_ptr(const volatile void *p,
size_t size)
>   ({								\
>   	typeof(ptr) __pu_ptr = (ptr);				\
>   	__chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr));		\
> -	x = ACCESS_ONCE(*(__pu_ptr));				\
> +	x = READ_ONCE(*(__pu_ptr));				\
>   	0;							\
>   })
>   
Reviewed-by: Jason Wang <jasowang at redhat.com>

On Thu, Nov 24, 2016 at 10:36:58PM +0200, Michael S. Tsirkin
wrote:
> On Thu, Nov 24, 2016 at 10:25:11AM +0000, Mark Rutland wrote:
> > For several reasons, it would be beneficial to kill off ACCESS_ONCE()
> > tree-wide, in favour of {READ,WRITE}_ONCE(). These work with aggregate
types,
> > more obviously document their intended behaviour, and are necessary
for tools
> > like KTSAN to work correctly (as otherwise reads and writes cannot be
> > instrumented separately).
> > 
> > While it's possible to script the bulk of this tree-wide
conversion, some cases
> > such as the virtio code, require some manual intervention. This series
moves
> > the virtio and vringh code over to {READ,WRITE}_ONCE(), in the process
fixing a
> > bug in the virtio headers.
> > 
> > Thanks,
> > Mark.
> 
> I don't have a problem with this specific patchset.
Good to hear. :)

Does that mean you're happy to queue these patches? Or would you prefer
a new posting at some later point, with ack/review tags accumulated?
> Though I really question the whole _ONCE APIs esp with
> aggregate types - these seem to generate a memcpy and
> an 8-byte read/writes sometimes, and I'm pretty sure this simply
> can't be read/written at once on all architectures.
Yes, in cases where the access is larger than the machine can perform in
a single access, this will result in a memcpy.

My understanding is that this has always been the case with
ACCESS_ONCE(), where multiple accesses were silently/implicitly
generated by the compiler.

We could add some compile-time warnings for those cases. I'm not sure if
there's a reason we avoided doing that so far; perhaps Christian has a
some idea.

Thanks,
Mark.