2005-12-15 12:25:29

by Qi Yong

[permalink] [raw]
Subject: [patch] ioctl BLKGETSIZE64 fix

Hello,

Two years ago, "[PATCH] use size_t for the broken ioctl numbers" brought in the problem.
<http://lkml.org/lkml/2003/9/7/14> (also FYI: <https://lwn.net/Articles/48360/>)

The patch below fixes the bug on BLKGETSIZE64. typeof(size_t) == 32, but we expect 64.
The choice of `size_t' is also a mistake. We should have taken `int'. This also affects
userland linux-kernel-headers.

Or am I missing something? Thanks.

Coywolf

Signed-off-by: Coywolf Qi Hunt <[email protected]>
---
diff -pruN 2.6.15-rc5-mm3/include/linux/fs.h 2.6.15-rc5-mm3~BLKGETSIZE64-fix/include/linux/fs.h
--- 2.6.15-rc5-mm3/include/linux/fs.h 2005-12-15 16:55:22.000000000 +0800
+++ 2.6.15-rc5-mm3~BLKGETSIZE64-fix/include/linux/fs.h 2005-12-15 20:08:52.000000000 +0800
@@ -197,7 +197,7 @@ extern int dir_notify_enable;
/* A jump here: 108-111 have been used for various private purposes. */
#define BLKBSZGET _IOR(0x12,112,size_t)
#define BLKBSZSET _IOW(0x12,113,size_t)
-#define BLKGETSIZE64 _IOR(0x12,114,size_t) /* return device size in bytes (u64 *arg) */
+#define BLKGETSIZE64 _IOR(0x12,114,u64) /* return device size in bytes (u64 *arg) */
#define BLKSTARTTRACE _IOWR(0x12,115,struct blk_user_trace_setup)
#define BLKSTOPTRACE _IO(0x12,116)


2005-12-15 13:01:50

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [patch] ioctl BLKGETSIZE64 fix

On Thu, Dec 15, 2005 at 08:25:27PM +0800, Coywolf Qi Hunt wrote:
> Two years ago, "[PATCH] use size_t for the broken ioctl numbers" brought in the problem.
> <http://lkml.org/lkml/2003/9/7/14> (also FYI: <https://lwn.net/Articles/48360/>)
>
> The patch below fixes the bug on BLKGETSIZE64. typeof(size_t) == 32, but we expect 64.
> The choice of `size_t' is also a mistake. We should have taken `int'. This also affects
> userland linux-kernel-headers.
>
> Or am I missing something? Thanks.

Did you test this change? I don't think you understood what the original
problem was.

This is the original definition:
-#define BLKGETSIZE64 _IOR(0x12,114,sizeof(u64)) /* return device
size in bytes (u64 *arg) */

What the author *meant* was to use u64. What they *got* was size_t.
Unfortunately, changing this to u64 breaks compatibility with userspace.
So we put in some checking code to make sure that people wouldn't make
this kind of mistake any more and converted all the bad users to size_t.

With Linux the size argument is *just* a convention. Some Unices do
the copyin/copyout thing in the ioctl dispatcher; Linux has the ioctl
handler do the copyin/copyout. So it does no harm to have the wrong size.

In summary: This change is wrong and causes ABI breakage for
architectures which have sizeof(u64) != sizeof(size_t).

> Coywolf
>
> Signed-off-by: Coywolf Qi Hunt <[email protected]>
> ---
> diff -pruN 2.6.15-rc5-mm3/include/linux/fs.h 2.6.15-rc5-mm3~BLKGETSIZE64-fix/include/linux/fs.h
> --- 2.6.15-rc5-mm3/include/linux/fs.h 2005-12-15 16:55:22.000000000 +0800
> +++ 2.6.15-rc5-mm3~BLKGETSIZE64-fix/include/linux/fs.h 2005-12-15 20:08:52.000000000 +0800
> @@ -197,7 +197,7 @@ extern int dir_notify_enable;
> /* A jump here: 108-111 have been used for various private purposes. */
> #define BLKBSZGET _IOR(0x12,112,size_t)
> #define BLKBSZSET _IOW(0x12,113,size_t)
> -#define BLKGETSIZE64 _IOR(0x12,114,size_t) /* return device size in bytes (u64 *arg) */
> +#define BLKGETSIZE64 _IOR(0x12,114,u64) /* return device size in bytes (u64 *arg) */
> #define BLKSTARTTRACE _IOWR(0x12,115,struct blk_user_trace_setup)
> #define BLKSTOPTRACE _IO(0x12,116)
>

2005-12-15 17:43:24

by Linus Torvalds

[permalink] [raw]
Subject: Re: [patch] ioctl BLKGETSIZE64 fix



On Thu, 15 Dec 2005, Coywolf Qi Hunt wrote:
>
> Two years ago, "[PATCH] use size_t for the broken ioctl numbers" brought in the problem.
> <http://lkml.org/lkml/2003/9/7/14> (also FYI: <https://lwn.net/Articles/48360/>)
>
> The patch below fixes the bug on BLKGETSIZE64. typeof(size_t) == 32, but we expect 64.
> The choice of `size_t' is also a mistake. We should have taken `int'. This also affects
> userland linux-kernel-headers.
>
> Or am I missing something? Thanks.

You're missing the fact that we can't just change existing numbers. It
just makes the ioctl not work.

A lot of the "size_t"'s got added because people had mis-understood the
_IOR/W macros, and had done a "sizeof()" by hand, when the macro itself
did the sizeof. Which caused the macro to make the ioctl number be based
on "sizeof(sizeof(realsize))", which is the same as "sizeof(size_t)".

And because we MUST NOT change the ioctl number (regardless of whether the
number shows the correct size or not), those got changed to use "size_t"
to match historical - if incorrect - numbering.

The _IOR/W macros got fixed so that you now _cannot_ use anything but a
real type, so the bug shouldn't happen again, but we're stuck with the old
broken numbers.

The number is just a number, after all. We shouldn't be using the size
encoding in the ioctl number, afaik (since for a _lot_ of them we can't
depend on it anyway).

Linus