Re: lsattr: incorrect size for ioctl result

Re: lsattr: incorrect size for ioctl result

[Rob Landley]

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

Re: lsattr: incorrect size for ioctl result

[Denys Vlasenko]

On Fri, Jun 25, 2021 at 10:44 AM Rob Landley <rob@landley.net> wrote:
> 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...)

Yes, GET/SETFLAGS also read and store int-sized value.

Re: lsattr: incorrect size for ioctl result

[Theodore Ts'o]

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.

We could do something like this:

#define        FS_IOC_GETVERSION               _IOR('v', 1, int)
#define        FS_IOC_GETVERSION_OLD           _IOR('v', 1, long)

But the key is that we keep support for the codepoint of _IOR('v', 1,
long) essentially forever, or we will break userspace binary
compatibility, which is verboten.

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: 

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 */
	switch (cmd) {
	case EXT2_IOC32_GETFLAGS:
		cmd = EXT2_IOC_GETFLAGS;
		break;
	case EXT2_IOC32_SETFLAGS:
		cmd = EXT2_IOC_SETFLAGS;
		break;
	case EXT2_IOC32_GETVERSION:
		cmd = EXT2_IOC_GETVERSION;
		break;
	case EXT2_IOC32_SETVERSION:
		cmd = EXT2_IOC_SETVERSION;
		break;
	default:
		return -ENOIOCTLCMD;
	}
	return ext2_ioctl(file, cmd, (unsigned long) compat_ptr(arg));
}

(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
even though the code point is _IOR('v', 1, long), we're reading and
writing an int, we need to be careful and check all of the kernel
codepaths that refer to IOC_{GET,SET}{FLAGS,VERSION}.

> 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?  :-)

					- Ted

Re: lsattr: incorrect size for ioctl result

[Rob Landley]

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

Re: lsattr: incorrect size for ioctl result

[Theodore Ts'o]

On Mon, Jun 28, 2021 at 02:35:24PM -0500, Rob Landley wrote:
> 
> 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.

The philosophical question is whether the encoding of _IO* is actually
part of an exported "API", or just a way of encoding codepoints such
that when data structures change size, the codepoint automatically
changes/breaks.

We have historically speaking, a non-trivial number of ioctl's which
don't follow the _IO[RW] convention.  For example, most of the block
ioctls predate the _IO[RW] convention, and they set or get memory
without specifying the size of the type that is set or get.  Oh, noos!
The kernel is (clutchign pearls) *violating* an API "promise".
(Although, in reality, these code points existed for long before we
imposed this _IO[RW] "API".)  Should we break userspace to "fix" this
supposed API violation?  Or should we go through a huge amount of
effort to fix them all?

At one point, I had made an attempt to define the "correct" codepoint
via a definition of EXT4_IOC_GETVERSION and EXT4_IOC_GETVERSION_OLD,
so the kernel would support the "correct" and "wrong" ioctl codepoint.
Unfortunate, this got broken when other folks tried to unify
everything to use FS_IOC_GETVERSION defined in
include/uapi/linux/fs.h.  And given that we would have to support the
"wrong" codepoint for a decade or more, personally, I've stopped
caring about it, especially since we've lived with it for a decade or
more, and very few people been harmed.

If someone wants to fix up all of the ioctl code points, perhaps so it
would make life easier for strace, or some such, it's not a *terrible*
idea.  (At the very least, it's more useful than KPI-hacking folks
submitting whitespace fixes that don't even fix all of the
checkpatch.pl "violations".)  But forcing all of the ioctl codepoints
into the procrustean bed of the _IO[RW] covention is a huge amount of
work, and would take years before userspace could depend on this
"API".

Cheers,

					- Ted

Re: lsattr: incorrect size for ioctl result

[Darrick J. Wong]

On Tue, Jun 29, 2021 at 11:24:26AM -0400, Theodore Ts'o wrote:
> On Mon, Jun 28, 2021 at 02:35:24PM -0500, Rob Landley wrote:
> > 
> > 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.
> 
> The philosophical question is whether the encoding of _IO* is actually
> part of an exported "API", or just a way of encoding codepoints such
> that when data structures change size, the codepoint automatically
> changes/breaks.
> 
> We have historically speaking, a non-trivial number of ioctl's which
> don't follow the _IO[RW] convention.  For example, most of the block
> ioctls predate the _IO[RW] convention, and they set or get memory
> without specifying the size of the type that is set or get.  Oh, noos!
> The kernel is (clutchign pearls) *violating* an API "promise".
> (Although, in reality, these code points existed for long before we
> imposed this _IO[RW] "API".)  Should we break userspace to "fix" this
> supposed API violation?  Or should we go through a huge amount of
> effort to fix them all?
> 
> At one point, I had made an attempt to define the "correct" codepoint
> via a definition of EXT4_IOC_GETVERSION and EXT4_IOC_GETVERSION_OLD,
> so the kernel would support the "correct" and "wrong" ioctl codepoint.
> Unfortunate, this got broken when other folks tried to unify
> everything to use FS_IOC_GETVERSION defined in
> include/uapi/linux/fs.h.  And given that we would have to support the
> "wrong" codepoint for a decade or more, personally, I've stopped
> caring about it, especially since we've lived with it for a decade or
> more, and very few people been harmed.
> 
> If someone wants to fix up all of the ioctl code points, perhaps so it
> would make life easier for strace, or some such, it's not a *terrible*
> idea.  (At the very least, it's more useful than KPI-hacking folks
> submitting whitespace fixes that don't even fix all of the
> checkpatch.pl "violations".)  But forcing all of the ioctl codepoints
> into the procrustean bed of the _IO[RW] covention is a huge amount of
> work, and would take years before userspace could depend on this
> "API".

Why don't we deprecate FS_IOC_[GS]ETFLAGS and tell everyone to use
FS[GS]ETXATTR?  They use the same code paths and vfs helpers now.

--D

> 
> Cheers,
> 
> 					- Ted

Re: lsattr: incorrect size for ioctl result

[Theodore Ts'o]

On Tue, Jun 29, 2021 at 02:04:22PM -0700, Darrick J. Wong wrote:
> 
> Why don't we deprecate FS_IOC_[GS]ETFLAGS and tell everyone to use
> FS[GS]ETXATTR?  They use the same code paths and vfs helpers now.

The FS_IOC_[GS]ETXATTR ioctls use struct fsxattr, which contains
fsx_xflags.  But there are flags returned (and set) by
FS_IOC_[GS]ETFLAGS that are not in fsx_xflags --- and vice versa, of
course.

That's not a problem for xfs, since fsx_xflags was originally designed
for xfs (and so it has some xfs-specific flags such as
FS_XFLAG_REALTIME, FS_XFLAG_RTINHERIT, FS_XFLAG_FILESTREAM,
FS_XFLAG_COWEXTSIZE, etc.).

But for other file systems, including btrfs, ext4 and f2fs, which
utilize FS_IOC_[GS]ETFLAGS flags which are not supported by
FS_IOC_[GS]ETXATTR's fsx_xflags.  Examples of such flags include
FS_ENCRYPT_FL, FS_TOPDIR_FL, FS_NOCMP_FL, and FS_CASEFOLD_FL.

There is also the _IO[RW] long vs int "API violation" for the
FS_IOC_[GS]ETVERSION which isn't addressed by FS_IOC_[GS]ETATTR.

So FS_IOC_[GS]ETATTR isn't a complete replacement/solution for these
"problem" ioctls.  And of course, even if we were to start telling
everyone to start using a new interface, it'll be many years before
any transition is complete.

						- Ted

Re: lsattr: incorrect size for ioctl result

[Rob Landley]

On 6/29/21 4:04 PM, Darrick J. Wong wrote:
> Why don't we deprecate FS_IOC_[GS]ETFLAGS and tell everyone to use
> FS[GS]ETXATTR?  They use the same code paths and vfs helpers now.

How does "invent a new API and tell everybody to use that instead" address a
concern about a least intrusive cleanup while maintaining binary compatibility
with historical weirdness?

(There IS a currently consistent API. It's the "32" versions. The surprise is
that the non-32 versions don't do something different than the 32 versions, even
though their definition implies they would.)

Rob