On 6/24/21 5:07 AM, Denys Vlasenko wrote:
> On Thu, Jun 24, 2021 at 10:53 AM Rob Landley <rob@xxxxxxxxxxx> wrote:
>>
>> On 6/23/21 6:41 AM, Denys Vlasenko wrote:
>> > 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).
>> ...
>> > 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.
>> 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
