[Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Joseph Salisbury]

Hi Luke,

A kernel bug report was opened against Ubuntu [0].  This bug was fixed
by the following commit in v4.7-rc1:


commit 4c63c2454eff996c5e27991221106eb511f7db38

Author: Luke Dashjr <luke@dashjr.org>
Date:   Thu Oct 29 08:22:21 2015 +0000

    btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in
btrfs_ioctl


However, this commit introduced a new regression.  With this commit
applied, "btrfs fi show" no longer works and the btrfs snapshot
functionality breaks.



I was hoping to get your feedback, since you are the patch author.  Do
you think gathering any additional data will help diagnose this issue,
or would it be best to submit a revert request?


Thanks,

Joe


[0] http://pad.lv/1619918

Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Luke Dashjr]

On Friday, January 06, 2017 5:22:34 PM Joseph Salisbury wrote:
>     btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in
> btrfs_ioctl
> 
> However, this commit introduced a new regression.  With this commit
> applied, "btrfs fi show" no longer works and the btrfs snapshot
> functionality breaks.

I don't see how this is even possibly related. Furthermore, "btrfs fi show" as 
well as snapshots seem to work just fine for me.

That being said, I've given up on running a 32-bit userspace, so I don't 
really care if this bugfix gets reverted or not.

Luke

Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Chris Mason]

On 01/06/2017 12:22 PM, Joseph Salisbury wrote:
> Hi Luke,
>
> A kernel bug report was opened against Ubuntu [0].  This bug was fixed
> by the following commit in v4.7-rc1:
>
>
> commit 4c63c2454eff996c5e27991221106eb511f7db38
>
> Author: Luke Dashjr <luke@dashjr.org>
> Date:   Thu Oct 29 08:22:21 2015 +0000
>
>     btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in
> btrfs_ioctl
>
>
> However, this commit introduced a new regression.  With this commit
> applied, "btrfs fi show" no longer works and the btrfs snapshot
> functionality breaks.
>
>
>
> I was hoping to get your feedback, since you are the patch author.  Do
> you think gathering any additional data will help diagnose this issue,
> or would it be best to submit a revert request?

This is working for me, could you please include an strace of the problem?

Thanks!

-chris

Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[David Sterba]

On Fri, Jan 06, 2017 at 12:22:34PM -0500, Joseph Salisbury wrote:
> A kernel bug report was opened against Ubuntu [0].  This bug was fixed
> by the following commit in v4.7-rc1:
> 
> commit 4c63c2454eff996c5e27991221106eb511f7db38
> 
> Author: Luke Dashjr <luke@dashjr.org>
> Date:   Thu Oct 29 08:22:21 2015 +0000
> 
>     btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in
> btrfs_ioctl
> 
> 
> However, this commit introduced a new regression.  With this commit
> applied, "btrfs fi show" no longer works and the btrfs snapshot
> functionality breaks.

A plain 32bit kernel with 32bit userspace works fine. The bug seems to
be on a 64bit kernel with 32bit userspace and the CONFIG_COMPAT compiled
in. Strace does not show anything special:

stat64("subv1", 0xffc64fcc)             = -1 ENOENT (No such file or directory)
statfs64(".", 84, {f_type=BTRFS_SUPER_MAGIC, f_bsize=4096, f_blocks=5110784, f_bfree=4076143, f_bavail=4010951, f_files=0, f_ffree=0, f_fsid={val=[
4260464218, 2297804334]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_RELATIME}) = 0
stat64(".", {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
stat64(".", {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
open(".", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC) = 3
fstat64(3, {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
write(1, "Create subvolume './subv1'\n", 27) = 27
ioctl(3, BTRFS_IOC_SUBVOL_CREATE, {fd=0, name="subv1"}) = -1 ENOTTY (Inappropriate ioctl for device)

The value of BTRFS_IOC_SUBVOL_CREATE is same on 32bit and 64bit kernels.
As it returns ENOTTY, the value is not recognized. A candidate function
is btrfs_compat_ioctl that checks for just the IOC32 numbers and returns
-ENOIOCTLCMD otherwise.

The callchain in fs/compat_ioctl.c:ioctl cheks for the specific callback
first, if it retunrs -ENOIOCTLCMD then goes to the normal ioctl
callback, so there's always a point we reach the handler of
BTRFS_IOC_SUBVOL_CREATE. So I don't see how it could happen.

Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2605 bytes --]

On 1/9/17 6:28 AM, David Sterba wrote:
> On Fri, Jan 06, 2017 at 12:22:34PM -0500, Joseph Salisbury wrote:
>> A kernel bug report was opened against Ubuntu [0].  This bug was fixed
>> by the following commit in v4.7-rc1:
>>
>> commit 4c63c2454eff996c5e27991221106eb511f7db38
>>
>> Author: Luke Dashjr <luke@dashjr.org>
>> Date:   Thu Oct 29 08:22:21 2015 +0000
>>
>>     btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in
>> btrfs_ioctl
>>
>>
>> However, this commit introduced a new regression.  With this commit
>> applied, "btrfs fi show" no longer works and the btrfs snapshot
>> functionality breaks.
> 
> A plain 32bit kernel with 32bit userspace works fine. The bug seems to
> be on a 64bit kernel with 32bit userspace and the CONFIG_COMPAT compiled
> in. Strace does not show anything special:
> 
> stat64("subv1", 0xffc64fcc)             = -1 ENOENT (No such file or directory)
> statfs64(".", 84, {f_type=BTRFS_SUPER_MAGIC, f_bsize=4096, f_blocks=5110784, f_bfree=4076143, f_bavail=4010951, f_files=0, f_ffree=0, f_fsid={val=[
> 4260464218, 2297804334]}, f_namelen=255, f_frsize=4096, f_flags=ST_VALID|ST_RELATIME}) = 0
> stat64(".", {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
> stat64(".", {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
> open(".", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC) = 3
> fstat64(3, {st_mode=S_IFDIR|0700, st_size=228, ...}) = 0
> fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
> write(1, "Create subvolume './subv1'\n", 27) = 27
> ioctl(3, BTRFS_IOC_SUBVOL_CREATE, {fd=0, name="subv1"}) = -1 ENOTTY (Inappropriate ioctl for device)
> 
> The value of BTRFS_IOC_SUBVOL_CREATE is same on 32bit and 64bit kernels.
> As it returns ENOTTY, the value is not recognized. A candidate function
> is btrfs_compat_ioctl that checks for just the IOC32 numbers and returns
> -ENOIOCTLCMD otherwise.
> 
> The callchain in fs/compat_ioctl.c:ioctl cheks for the specific callback
> first, if it retunrs -ENOIOCTLCMD then goes to the normal ioctl
> callback, so there's always a point we reach the handler of
> BTRFS_IOC_SUBVOL_CREATE. So I don't see how it could happen.

I got a minute to look at this today.  It doesn't fallback to the normal
ioctl handler.  Since we have ->unlocked_ioctl defined, it checks the
hard-coded compat tables and then fails with -ENOTTY.

Fortunately, the fix is simple.  The default stanza of that switch
statement in btrfs_compat_ioctl needs to be removed.  Then it just
works.  I'll post a fix momentarily.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]