UBSAN reports due to ATTR_XVATTR definition

[Bronek]

The definition of ATTR_XVATTR in vnode.h is technically incorrect 1 << 31 because it invokes overflow, which is undefined behaviour for signed int. It should be replaced with 1u << 31 . However this may in turn force conversions to unsigned int in locations were ATTR_XVATTR is used.

This problem has caused following "bogus" UBSAN reports on my test system:

2017-12-16T22:04:14,494937+0000 ZFS: Loaded module v0.7.4-1, ZFS pool version 5000, ZFS filesystem version 5
2017-12-16T22:04:15,263529+0000 ip_tables: (C) 2000-2006 Netfilter Core Team
2017-12-16T22:04:15,547801+0000 ================================================================================
2017-12-16T22:04:15,548399+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/include/sys/xvattr.h:292:32
2017-12-16T22:04:15,548993+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,552801+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/include/sys/xvattr.h:280:28
2017-12-16T22:04:15,553386+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,555060+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_vnops.c:2796:15
2017-12-16T22:04:15,555648+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,563582+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_vnops.c:2859:13
2017-12-16T22:04:15,564170+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,568493+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_log.c:604:21
2017-12-16T22:04:15,568494+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,568666+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_log.c:629:21
2017-12-16T22:04:15,568667+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,568983+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_vnops.c:3051:15
2017-12-16T22:04:15,568983+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,618932+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_vnops.c:1341:21
2017-12-16T22:04:15,619529+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . . ================================================================================
2017-12-16T22:04:15,623045+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_log.c:73:33
2017-12-16T22:04:15,623638+0000 left shift of 1 by 31 places cannot be represented in type 'int'

. . . ================================================================================
2017-12-16T22:04:15,625398+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_log.c:291:21
2017-12-16T22:04:15,626008+0000 left shift of 1 by 31 places cannot be represented in type 'int'
. . .
================================================================================
2017-12-16T22:04:15,628174+0000 UBSAN: Undefined behaviour in /build/zfs-linux/src/zfs-0.7.4/module/zfs/zfs_log.c:336:21
2017-12-16T22:04:15,628759+0000 left shift of 1 by 31 places cannot be represented in type 'int'

[behlendorf]

@Bronek thanks for catching this. The only place ATTR_XVATTR is used is with the va_mask which is unsigned so it's safe to cast this. Can you verify that the tweak you suggested and changing the va_mask type does resolve the bogus warnings.

diff --git a/include/sys/vnode.h b/include/sys/vnode.h
index 9ae48c7..9eb91e5 100644
--- a/include/sys/vnode.h
+++ b/include/sys/vnode.h
@@ -87,7 +87,7 @@
 #define AT_MTIME       ATTR_MTIME
 #define AT_CTIME       ATTR_CTIME

-#define ATTR_XVATTR    (1 << 31)
+#define ATTR_XVATTR    (1U << 31)
 #define AT_XVATTR      ATTR_XVATTR

 #define ATTR_IATTR_MASK        (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_SIZE | \
@@ -121,7 +121,7 @@ typedef enum vtype {

 typedef struct vattr {
        enum vtype      va_type;        /* vnode type */
-       u_int           va_mask;        /* attribute bit-mask */
+       uint32_t        va_mask;        /* attribute bit-mask */
        u_short         va_mode;        /* acc mode */
        uid_t           va_uid;         /* owner uid */
        gid_t           va_gid;         /* owner gid */

[Bronek]

@behlendorf Thank you, this works (just tested)

[ryao]

@behlendorf Your fix looks good to me.

[behlendorf]

This fix was merged.

[rincebrain]

@tonyhutter could we possibly get this in 0.7.12, if one gets cut? I'm trying to help someone debug problems and it causes a lot of UBSAN noise.

[behlendorf]

I've added this issue to the 0.7.12 project so we can track it for possible inclusion.