On 6/28/21 11:33 AM, Theodore Ts'o wrote:
> On Fri, Jun 25, 2021 at 04:01:27AM -0500, Rob Landley wrote:
>> > No. The above is a lie.
>>
>> --- 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)
>
> The problem is that there are a large number of userspace programs
> which are using _IOR('v', 1, long) (the codepoint for
> FS_IOC_GETVERSION for decades), but are expecting the kernel to fill
> in an int.
Because if we don't any 64 bit version breaks on big endian. (And possibly also
if userspace didn't prezero the long.)
I agree changing the behavior of FS_IOC_* to do what the export says it does
would break binary compatibility, which is why my patch went the other way.
There's already FS_IOC32_* and I thought the suggested change would export their
ioctl numbers for both sets of names. (Does that not work?)
Maybe for compatibility/cleanup/pruning internal codepaths something like:
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -1144,13 +1144,13 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned
int, cmd,
#endif
/*
- * These access 32-bit values anyway so no further handling is
- * necessary.
+ * These are defined "long" but access int values, so force to IOC32
*/
- case FS_IOC32_GETFLAGS:
- case FS_IOC32_SETFLAGS:
- cmd = (cmd == FS_IOC32_GETFLAGS) ?
- FS_IOC_GETFLAGS : FS_IOC_SETFLAGS;
+ case FS_IOC_GETVERSION:
+ case FS_IOC_SETVERSION:
+ case FS_IOC_GETFLAGS:
+ case FS_IOC_SETFLAGS:
+ if (sizeof(long) == 8) cmd ^= 12 << _IOC_SIZESHIFT;
fallthrough;
/*
* everything else in do_vfs_ioctl() takes either a compatible
... would mean all the filesystems only ever see IOC32 and if they want to add
64 at some point probably use a new number for it?
At the moment I'm just trying to go "can the next person not hit the same issue
I hit, which Denys presumably notified me about because HE hit it".
> We also need to be a bit careful when we make these sorts of changes
> of #defines, so we don't break kernel code like this:
*shrug* I dunno how it should be properly fixed, just heads up that it's borked.
> long ext2_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> /* These are just misnamed, they actually get/put from/to user an int */
Of course given that comment I guess they already knew. The comment was added 18
years ago:
https://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux-fullhistory.git/commit/?id=835a765ed1e1ef5814c9e8c6e1b64b63f8a335c9
And yet the uapi headers still don't have any comments next to the ioctl export
warning that the long is wrong:
-- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -201,15 +201,19 @@ struct fsxattr {
#define FSLABEL_MAX 256 /* Max chars for the interface; each fs may
differ */
+/* Warning: these actually read/write an unsigned int, not long */
#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_FIEMAP _IOWR('f', 11, struct fiemap)
+
+/* Functionally equivalent to the FS_IOC_{GET,SET}{FLAGS,VERSION} above */
#define FS_IOC32_GETFLAGS _IOR('f', 1, int)
#define FS_IOC32_SETFLAGS _IOW('f', 2, int)
#define FS_IOC32_GETVERSION _IOR('v', 1, int)
#define FS_IOC32_SETVERSION _IOW('v', 2, int)
+
+#define FS_IOC_FIEMAP _IOWR('f', 11, struct fiemap)
#define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
#define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
#define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX])
> (This is from 4.4's fs/ext2/ioct.c; the point is if we want to "fix"
> the definition of *_IOC_GETFLAGS because of a pearl clutching fit that
...
>> 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...)
>
> Probably because the people who added the IOC32 versions didn't
> understand this at the time? :-)
Let me see if I understand:
1) The API the kernel exports is not what the kernel is doing.
2) Userspace can't reliably use the API the way it's currently exported.
3) Even other kernel devs "didn't understand" it.
4) Fixing it would involve scare quotes and result from a pearl clutching fit.
... no, I'm pretty sure I don't understand.
*shrug* I've cc'd Michael Kerrisk in hopes he can update the man pages. "man
ioctl_list" already documents these ioctls correctishly (modulo the
signed/unsigned part) but might benefit from some sort of "warning, do not trust
the kernel headers here, they are wrong" comment.
Rob
