Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl
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>> Date: Thu Oct 29 08:22:21 2015 + >> >> 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 signature.asc Description: OpenPGP digital signature
Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl
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 >> Date: Thu Oct 29 08:22:21 2015 + >> >> 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 signature.asc Description: OpenPGP digital signature
Re: [Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl
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> Date: Thu Oct 29 08:22:21 2015 + > > 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
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 > Date: Thu Oct 29 08:22:21 2015 + > > 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
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 DashjrDate: Thu Oct 29 08:22:21 2015 + 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
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 Date: Thu Oct 29 08:22:21 2015 + 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
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
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
[Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl
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 DashjrDate: Thu Oct 29 08:22:21 2015 + 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
[Regression 4.7-rc1] btrfs: bugfix: handle FS_IOC32_{GETFLAGS,SETFLAGS,GETVERSION} in btrfs_ioctl
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 Date: Thu Oct 29 08:22:21 2015 + 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