2009-08-01 14:59:25

by Mark Lord

[permalink] [raw]
Subject: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.

When using a 32-bit runtime on top of a 64-bit kernel,
programs like "filefrag" and "hdparm --fibmap" do not work correctly.

This is because there's no compat ioctl entry for the FIEMAP call.
FIEMAP returns file extent info, similar to FIBMAP (but better).

Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
program to reliably get detailed block information for a file
when run on top of a 64-bit kernel. This patch addresses the issue.

Once upstream, this patch could also be a candidate for -stable.

Signed-off-by: Mark Lord <[email protected]>

--- old/fs/compat_ioctl.c 2009-08-01 10:47:16.601066905 -0400
+++ linux/fs/compat_ioctl.c 2009-08-01 10:49:23.054387926 -0400
@@ -35,6 +35,7 @@
#include <linux/falloc.h>
#include <linux/fs.h>
#include <linux/file.h>
+#include <linux/fiemap.h>
#include <linux/ppp_defs.h>
#include <linux/if_ppp.h>
#include <linux/if_pppox.h>
@@ -1907,6 +1908,7 @@
COMPATIBLE_IOCTL(FIONREAD) /* This is also TIOCINQ */
/* 0x00 */
COMPATIBLE_IOCTL(FIBMAP)
+COMPATIBLE_IOCTL(FS_IOC_FIEMAP)
COMPATIBLE_IOCTL(FIGETBSZ)
/* 'X' - originally XFS but some now in the VFS */
COMPATIBLE_IOCTL(FIFREEZE)
@@ -2805,6 +2807,7 @@
goto out_fput;
#endif

+ case FS_IOC_FIEMAP:
case FIBMAP:
case FIGETBSZ:
case FIONREAD:


2009-08-01 15:17:13

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

Mark Lord wrote:
..
> Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
..

To stave off the inevitable question, FIBMAP doesn't appear to cope well
with allocated/uncommitted blocks. Creating a file with fallocate(),
and then immediately running FIBMAP, will not give any allocation data.

Doing a sync() after the fallocate() allows FIBMAP to work again.

FIEMAP doesn't require the sync().

Cheers

2009-08-03 17:48:15

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

(resending, no ack from anyone first time around).

Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.

When using a 32-bit runtime on top of a 64-bit kernel,
programs like "filefrag" and "hdparm --fibmap" do not work correctly.

This is because there's no compat ioctl entry for the FIEMAP call.
FIEMAP returns file extent info, similar to FIBMAP (but better).

Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
program to reliably get detailed block information for a file
when run on top of a 64-bit kernel. This patch addresses the issue.

Once upstream, this patch could also be a candidate for -stable.

Signed-off-by: Mark Lord <[email protected]>

Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.

When using a 32-bit runtime on top of a 64-bit kernel,
programs like "filefrag" and "hdparm --fibmap" do not work correctly.

This is because there's no compat ioctl entry for the FIEMAP call.
FIEMAP returns file extent info, similar to FIBMAP (but better).

Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
program to reliably get detailed block information for a file
when run on top of a 64-bit kernel. This patch addresses the issue.

Once upstream, this patch could also be a candidate for -stable.

Signed-off-by: Mark Lord <[email protected]>

--- old/fs/compat_ioctl.c 2009-08-01 10:47:16.601066905 -0400
+++ linux/fs/compat_ioctl.c 2009-08-01 10:49:23.054387926 -0400
@@ -35,6 +35,7 @@
#include <linux/falloc.h>
#include <linux/fs.h>
#include <linux/file.h>
+#include <linux/fiemap.h>
#include <linux/ppp_defs.h>
#include <linux/if_ppp.h>
#include <linux/if_pppox.h>
@@ -1907,6 +1908,7 @@
COMPATIBLE_IOCTL(FIONREAD) /* This is also TIOCINQ */
/* 0x00 */
COMPATIBLE_IOCTL(FIBMAP)
+COMPATIBLE_IOCTL(FS_IOC_FIEMAP)
COMPATIBLE_IOCTL(FIGETBSZ)
/* 'X' - originally XFS but some now in the VFS */
COMPATIBLE_IOCTL(FIFREEZE)
@@ -2805,6 +2807,7 @@
goto out_fput;
#endif

+ case FS_IOC_FIEMAP:
case FIBMAP:
case FIGETBSZ:
case FIONREAD:

2009-08-03 18:00:56

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

Mark Lord wrote:
> (resending, no ack from anyone first time around).

I had previously sent a fix for this to the ext4 list as well, although
w/o the added case for compat_ioctl or the extra #include.

Because this ioctl should be 100% compat everywhere, I don't -think-
it's needed, and

http://marc.info/?l=linux-ext4&m=124872536713005&w=2

suffices....

But if not, minor nitpick, you should move the COMPAT_IOCTL under the
"little f" comment.

-Eric


> Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.
>
> When using a 32-bit runtime on top of a 64-bit kernel,
> programs like "filefrag" and "hdparm --fibmap" do not work correctly.
>
> This is because there's no compat ioctl entry for the FIEMAP call.
> FIEMAP returns file extent info, similar to FIBMAP (but better).
>
> Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
> program to reliably get detailed block information for a file
> when run on top of a 64-bit kernel. This patch addresses the issue.
>
> Once upstream, this patch could also be a candidate for -stable.
>
> Signed-off-by: Mark Lord <[email protected]>
>
> Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.
>
> When using a 32-bit runtime on top of a 64-bit kernel,
> programs like "filefrag" and "hdparm --fibmap" do not work correctly.
>
> This is because there's no compat ioctl entry for the FIEMAP call.
> FIEMAP returns file extent info, similar to FIBMAP (but better).
>
> Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
> program to reliably get detailed block information for a file
> when run on top of a 64-bit kernel. This patch addresses the issue.
>
> Once upstream, this patch could also be a candidate for -stable.
>
> Signed-off-by: Mark Lord <[email protected]>
>
> --- old/fs/compat_ioctl.c 2009-08-01 10:47:16.601066905 -0400
> +++ linux/fs/compat_ioctl.c 2009-08-01 10:49:23.054387926 -0400
> @@ -35,6 +35,7 @@
> #include <linux/falloc.h>
> #include <linux/fs.h>
> #include <linux/file.h>
> +#include <linux/fiemap.h>
> #include <linux/ppp_defs.h>
> #include <linux/if_ppp.h>
> #include <linux/if_pppox.h>
> @@ -1907,6 +1908,7 @@
> COMPATIBLE_IOCTL(FIONREAD) /* This is also TIOCINQ */
> /* 0x00 */
> COMPATIBLE_IOCTL(FIBMAP)
> +COMPATIBLE_IOCTL(FS_IOC_FIEMAP)
> COMPATIBLE_IOCTL(FIGETBSZ)
> /* 'X' - originally XFS but some now in the VFS */
> COMPATIBLE_IOCTL(FIFREEZE)
> @@ -2805,6 +2807,7 @@
> goto out_fput;
> #endif
>
> + case FS_IOC_FIEMAP:
> case FIBMAP:
> case FIGETBSZ:
> case FIONREAD:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2009-08-03 18:00:11

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

On Monday 03 August 2009, Mark Lord wrote:
> (resending, no ack from anyone first time around).

I actually wrote a lengthy reply on how I think the code around it
should be done differently and then realized why we had done it
that way originally and did not send out my reply.

Your addition looks ok, the data structures are compatible
on all architectures.

> Add support for the FIEMAP ioctl for 32-bit user on 64-bit kernel.
>
> When using a 32-bit runtime on top of a 64-bit kernel,
> programs like "filefrag" and "hdparm --fibmap" do not work correctly.
>
> This is because there's no compat ioctl entry for the FIEMAP call.
> FIEMAP returns file extent info, similar to FIBMAP (but better).
>
> Since FIBMAP itself is b0rked on ext4, this leaves no way for a 32-bit
> program to reliably get detailed block information for a file
> when run on top of a 64-bit kernel. This patch addresses the issue.
>
> Once upstream, this patch could also be a candidate for -stable.
>
> Signed-off-by: Mark Lord <[email protected]>

Reviewed-by: Arnd Bergmann <[email protected]>

2009-08-03 22:07:55

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

Eric Sandeen wrote:
> Mark Lord wrote:
>> (resending, no ack from anyone first time around).
>
> I had previously sent a fix for this to the ext4 list as well, although
> w/o the added case for compat_ioctl or the extra #include.
>
> Because this ioctl should be 100% compat everywhere, I don't -think-
> it's needed, and
>
> http://marc.info/?l=linux-ext4&m=124872536713005&w=2
>
> suffices....
..

Well, whichever of the two works best for the maintainers.

We need *something* for it upstream, and probably back in -stable too.
Otherwise this prevents using 64-bit kernels on 32-bit userland,
as Linus likes to recommend so often. ;)

Cheers

2009-08-03 22:21:30

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

Mark Lord wrote:
> Eric Sandeen wrote:
>> Mark Lord wrote:
>>> (resending, no ack from anyone first time around).
>> I had previously sent a fix for this to the ext4 list as well, although
>> w/o the added case for compat_ioctl or the extra #include.
>>
>> Because this ioctl should be 100% compat everywhere, I don't -think-
>> it's needed, and
>>
>> http://marc.info/?l=linux-ext4&m=124872536713005&w=2
>>
>> suffices....
> ..
>
> Well, whichever of the two works best for the maintainers.
>
> We need *something* for it upstream, and probably back in -stable too.
> Otherwise this prevents using 64-bit kernels on 32-bit userland,
> as Linus likes to recommend so often. ;)
>
> Cheers
>

Heh, it's probably far from the only ioctl missing a compat handler, but
yeah. :)

-Eric

2009-08-03 23:23:24

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

On Mon, 03 Aug 2009 18:07:53 -0400
Mark Lord <[email protected]> wrote:

> Eric Sandeen wrote:
> > Mark Lord wrote:
> >> (resending, no ack from anyone first time around).
> >
> > I had previously sent a fix for this to the ext4 list as well, although
> > w/o the added case for compat_ioctl or the extra #include.
> >
> > Because this ioctl should be 100% compat everywhere, I don't -think-
> > it's needed, and
> >
> > http://marc.info/?l=linux-ext4&m=124872536713005&w=2

mutter. I don't read linux-fsdevel much.

> > suffices....
> ..
>
> Well, whichever of the two works best for the maintainers.
>
> We need *something* for it upstream, and probably back in -stable too.
> Otherwise this prevents using 64-bit kernels on 32-bit userland,
> as Linus likes to recommend so often. ;)

OK, here's what I have, with a somewhat reworked changelog.

I assumed that "Josef" == [email protected].

Arnd, could you please check that it still looks OK?

Thanks.


From: Eric Sandeen <[email protected]>

The FIEMAP_IOC_FIEMAP mapping ioctl was missing a 32-bit compat handler,
which means that 32-bit suerspace on 64-bit kernels cannot use this ioctl
command.

The structure is nicely aligned, padded, and sized, so it is just this
simple.

Tested w/ 32-bit ioctl tester (from Josef) on a 64-bit kernel on ext4.

Signed-off-by: Eric Sandeen <[email protected]>
Cc: <[email protected]>
Cc: Mark Lord <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Josef Bacik <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

fs/compat_ioctl.c | 1 +
1 file changed, 1 insertion(+)

diff -puN fs/compat_ioctl.c~compat_ioctl-hook-up-compat-handler-for-fiemap-ioctl fs/compat_ioctl.c
--- a/fs/compat_ioctl.c~compat_ioctl-hook-up-compat-handler-for-fiemap-ioctl
+++ a/fs/compat_ioctl.c
@@ -1838,6 +1838,7 @@ COMPATIBLE_IOCTL(FIONCLEX)
COMPATIBLE_IOCTL(FIOASYNC)
COMPATIBLE_IOCTL(FIONBIO)
COMPATIBLE_IOCTL(FIONREAD) /* This is also TIOCINQ */
+COMPATIBLE_IOCTL(FS_IOC_FIEMAP)
/* 0x00 */
COMPATIBLE_IOCTL(FIBMAP)
COMPATIBLE_IOCTL(FIGETBSZ)
_


2009-08-04 10:39:17

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] fs/compat_ioctl.c: add missing FS_IOC_FIEMAP support

On Tuesday 04 August 2009, Andrew Morton wrote:
> > Well, whichever of the two works best for the maintainers.
> >
> > We need something for it upstream, and probably back in -stable too.
> > Otherwise this prevents using 64-bit kernels on 32-bit userland,
> > as Linus likes to recommend so often. ;)
>
> OK, here's what I have, with a somewhat reworked changelog.
>
> I assumed that "Josef" == [email protected].
>
> Arnd, could you please check that it still looks OK?

Yes, it looks ok as well. The #include is not needed here,
and the difference in compat_sys_ioctl() is that with Eric's
patch, a file system or device driver could in theory implement
its own compat handler for FS_IOC_FIEMAP while it cannot do
that for the native ioctl.

I would like to keep the logic in compat_sys_ioctl in sync with
do_vfs_ioctl, but they have diverged already. I have an experimental
patch set to rework compat_ioctl handling that will also take care
of this.

Arnd <><