Re: lsattr: incorrect size for ioctl result

On 6/24/21 5:07 AM, Denys Vlasenko wrote:

On Thu, Jun 24, 2021 at 10:53 AM Rob Landley [off-list ref] wrote:

quoted

On 6/23/21 6:41 AM, Denys Vlasenko wrote:

quoted

  unsigned long flag = 0, version = 0;
  int fd;
  struct stat sb;
...
  if (FLAG(v)) {
    if (ioctl(fd, FS_IOC_GETVERSION, (void*)&version) < 0) goto LABEL2;
    xprintf("%-10lu ", version);
  }

if (ext2_getflag(fd, &sb, &flag) < 0) perror_msg("reading flags '%s'", path);
...

The above only works on little-endian.
(The hint is in zeroing assignments at the beginning).

...

quoted

These ioctls return an int-sized result, not long.
(Check the kernel source to verify).

Looks to me like the kernel source is taking a long?

include/uapi/linux/fs.h:#define FS_IOC_GETVERSION   _IOR('v', 1, long)

No. The above is a lie.

If so that's a kernel bug. That was out of current git.

quoted

Has since 2006: https://github.com/torvalds/linux/commit/36695673b012

There's a FS_IOC32_GETVERSION but that's not what we're using here?

fs/ext2/ioctl.c:
...
        case EXT2_IOC_GETFLAGS:
                flags = ei->i_flags & EXT2_FL_USER_VISIBLE;
                return put_user(flags, (int __user *) arg);
...
        case EXT2_IOC_GETVERSION:
                return put_user(inode->i_generation, (int __user *) arg);


To verify, replace

- unsigned long flag = 0, version = 0;
+ unsigned long flag = -1L, version = -1L;

and see what x86-64 version of the lsattr -v shows.

Hmmm, btrfs is also using int __user *arg, xfs is using int... you're right.
That's a bug in the kernel headers.

Except int isn't right either, it's gotta be unsigned or else it's negative half
the time (which is not what lsattr -v shows). But fs/xfs/xfs_ioctl.c
fs/btrfs/ioctl.c fs/ext2/ioctl.c are all using signed "int *". Not that it
matters quite as much what they do internally, but I think the right patch is
probably:

--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h

@@ -203,8 +203,8 @@ struct fsxattr {

 #define        FS_IOC_GETFLAGS                 _IOR('f', 1, long)
 #define        FS_IOC_SETFLAGS                 _IOW('f', 2, long)
-#define        FS_IOC_GETVERSION               _IOR('v', 1, long)
-#define        FS_IOC_SETVERSION               _IOW('v', 2, long)
+#define        FS_IOC_GETVERSION               _IOR('v', 1, unsigned int)
+#define        FS_IOC_SETVERSION               _IOW('v', 2, unsigned int)
 #define FS_IOC_FIEMAP                  _IOWR('f', 11, struct fiemap)
 #define FS_IOC32_GETFLAGS              _IOR('f', 1, int)
 #define FS_IOC32_SETFLAGS              _IOW('f', 2, int)

Which raises the question "why is there an IOC32 version of this when it was
never NOT 32 bit" and "does GETFLAGS have the same problem"? (Haven't looked...)

David: opinion?

Rob