[REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

Hi Amir

Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
breaks squashfs with an ubifs overlay (both ubi volumes of the same
container).

Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
ubifs_add_orphan: orphaned twice". This corrupts the the filesystem and
the next attempt to mount the overlay will fail.

Regards
Ralph


Bisect log:

  git bisect start
  # bad: [c02ed2e75ef4c74e41e421acb4ef1494671585e8] Linux 4.11-rc4
  git bisect bad c02ed2e75ef4c74e41e421acb4ef1494671585e8
  # good: [c470abd4fde40ea6a0846a2beab642a578c0b8cd] Linux 4.10
  git bisect good c470abd4fde40ea6a0846a2beab642a578c0b8cd
  # good: [bc49a7831b1137ce1c2dda1c57e3631655f5d2ae] Merge branch 'akpm' (patches from Andrew)
  git bisect good bc49a7831b1137ce1c2dda1c57e3631655f5d2ae
  # good: [738bc38d49e017fe7acb3596712518e22c225816] kernel/ksysfs.c: add __ro_after_init to bin_attribute structure
  git bisect good 738bc38d49e017fe7acb3596712518e22c225816
  # good: [3f80dd67c367878aaad16e458eebc3c8980bb841] Merge tag 'acpi-extra-4.11-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
  git bisect good 3f80dd67c367878aaad16e458eebc3c8980bb841
  # bad: [4495c08e84729385774601b5146d51d9e5849f81] Linux 4.11-rc2
  git bisect bad 4495c08e84729385774601b5146d51d9e5849f81
  # bad: [2d62e0768d3c28536d4cfe4c40ba1e5e8e442a93] Merge tag 'kvm-4.11-2' of git://git.kernel.org/pub/scm/virt/kvm/kvm
  git bisect bad 2d62e0768d3c28536d4cfe4c40ba1e5e8e442a93
  # good: [1827adb11ad26b2290dc9fe2aaf54976b2439865] Merge branch 'WIP.sched-core-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
  git bisect good 1827adb11ad26b2290dc9fe2aaf54976b2439865
  # bad: [0b94da8dfc26ec2eb3e6640726e434abf8c53e49] Merge branch 'libnvdimm-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
  git bisect bad 0b94da8dfc26ec2eb3e6640726e434abf8c53e49
  # bad: [0a040b2113ec226bcf56fcbe02d035bb5b30c35e] Merge branch 'for-next' of git://git.samba.org/sfrench/cifs-2.6
  git bisect bad 0a040b2113ec226bcf56fcbe02d035bb5b30c35e
  # good: [590dce2d4934fb909b112cd80c80486362337744] Merge branch 'rebased-statx' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
  git bisect good 590dce2d4934fb909b112cd80c80486362337744
  # bad: [4e66c42c60fdf9be81837857454a41b39bf1b773] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse
  git bisect bad 4e66c42c60fdf9be81837857454a41b39bf1b773
  # bad: [e593b2bf513dd4d3fbfa0f435392eea2c7f776f0] ovl: properly implement sync_filesystem()
  git bisect bad e593b2bf513dd4d3fbfa0f435392eea2c7f776f0
  # bad: [d8514d8edb5b045cf7f708e14f888ce760d60f0b] ovl: copy up regular file using O_TMPFILE
  git bisect bad d8514d8edb5b045cf7f708e14f888ce760d60f0b
  # good: [42f269b925405d9dd45014823e5057786d6ca452] ovl: rearrange code in ovl_copy_up_locked()
  git bisect good 42f269b925405d9dd45014823e5057786d6ca452
  # first bad commit: [d8514d8edb5b045cf7f708e14f888ce760d60f0b] ovl: copy up regular file using O_TMPFILE

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Tue, Mar 28, 2017 at 4:01 AM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> Hi Amir
>
> Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
> breaks squashfs with an ubifs overlay (both ubi volumes of the same
> container).
>

Hi Ralph,

I am confused by the description above. Which are the 'both ubi volumes'?

Can you provide exact command of overlayfs mount, preferably
also a script to generate the lower/upper images and mount them
to remove any mkfs option doubts from test setup.

> Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
> ubifs_add_orphan: orphaned twice". This corrupts the the filesystem and
> the next attempt to mount the overlay will fail.
>

Does that happen on any attempt to rename a file?
A file that was only is lower I suppose?
Can you provide a simple script with your test, setting up the lower/upper
files and triggering the bug.

Thanks,
Amir.


>
>
> Bisect log:
>
>   git bisect start
>   # bad: [c02ed2e75ef4c74e41e421acb4ef1494671585e8] Linux 4.11-rc4
>   git bisect bad c02ed2e75ef4c74e41e421acb4ef1494671585e8
>   # good: [c470abd4fde40ea6a0846a2beab642a578c0b8cd] Linux 4.10
>   git bisect good c470abd4fde40ea6a0846a2beab642a578c0b8cd
>   # good: [bc49a7831b1137ce1c2dda1c57e3631655f5d2ae] Merge branch 'akpm' (patches from Andrew)
>   git bisect good bc49a7831b1137ce1c2dda1c57e3631655f5d2ae
>   # good: [738bc38d49e017fe7acb3596712518e22c225816] kernel/ksysfs.c: add __ro_after_init to bin_attribute structure
>   git bisect good 738bc38d49e017fe7acb3596712518e22c225816
>   # good: [3f80dd67c367878aaad16e458eebc3c8980bb841] Merge tag 'acpi-extra-4.11-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
>   git bisect good 3f80dd67c367878aaad16e458eebc3c8980bb841
>   # bad: [4495c08e84729385774601b5146d51d9e5849f81] Linux 4.11-rc2
>   git bisect bad 4495c08e84729385774601b5146d51d9e5849f81
>   # bad: [2d62e0768d3c28536d4cfe4c40ba1e5e8e442a93] Merge tag 'kvm-4.11-2' of git://git.kernel.org/pub/scm/virt/kvm/kvm
>   git bisect bad 2d62e0768d3c28536d4cfe4c40ba1e5e8e442a93
>   # good: [1827adb11ad26b2290dc9fe2aaf54976b2439865] Merge branch 'WIP.sched-core-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
>   git bisect good 1827adb11ad26b2290dc9fe2aaf54976b2439865
>   # bad: [0b94da8dfc26ec2eb3e6640726e434abf8c53e49] Merge branch 'libnvdimm-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
>   git bisect bad 0b94da8dfc26ec2eb3e6640726e434abf8c53e49
>   # bad: [0a040b2113ec226bcf56fcbe02d035bb5b30c35e] Merge branch 'for-next' of git://git.samba.org/sfrench/cifs-2.6
>   git bisect bad 0a040b2113ec226bcf56fcbe02d035bb5b30c35e
>   # good: [590dce2d4934fb909b112cd80c80486362337744] Merge branch 'rebased-statx' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
>   git bisect good 590dce2d4934fb909b112cd80c80486362337744
>   # bad: [4e66c42c60fdf9be81837857454a41b39bf1b773] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse
>   git bisect bad 4e66c42c60fdf9be81837857454a41b39bf1b773
>   # bad: [e593b2bf513dd4d3fbfa0f435392eea2c7f776f0] ovl: properly implement sync_filesystem()
>   git bisect bad e593b2bf513dd4d3fbfa0f435392eea2c7f776f0
>   # bad: [d8514d8edb5b045cf7f708e14f888ce760d60f0b] ovl: copy up regular file using O_TMPFILE
>   git bisect bad d8514d8edb5b045cf7f708e14f888ce760d60f0b
>   # good: [42f269b925405d9dd45014823e5057786d6ca452] ovl: rearrange code in ovl_copy_up_locked()
>   git bisect good 42f269b925405d9dd45014823e5057786d6ca452
>   # first bad commit: [d8514d8edb5b045cf7f708e14f888ce760d60f0b] ovl: copy up regular file using O_TMPFILE

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Tue, Mar 28, 2017 at 5:27 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 4:01 AM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
>> Hi Amir
>>
>> Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
>> breaks squashfs with an ubifs overlay (both ubi volumes of the same
>> container).
>>
>
> Hi Ralph,
>
> I am confused by the description above. Which are the 'both ubi volumes'?
>
> Can you provide exact command of overlayfs mount, preferably
> also a script to generate the lower/upper images and mount them
> to remove any mkfs option doubts from test setup.
>
>> Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
>> ubifs_add_orphan: orphaned twice". This corrupts the the filesystem and
>> the next attempt to mount the overlay will fail.
>>

Looking at the ubifs code, it does not appear that ubifs_link() ever calls
ubifs_delete_orphan() for the case of linking a temp file (nlink == 0),
so this looks like a ubifs bug.

Is ubifs being tested with xfstests? This should have been caught by test
generic/004 (link tempfile then delete it).

A quick fix for ubifs+overlayfs would be to disable ubifs O_TMPFILE
support (because it is broken) and then overlayfs won't try to copy up
using tmpfile.

Amir.

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Richard Weinberger]

Amir,

Am 28.03.2017 um 12:43 schrieb Amir Goldstein:
> On Tue, Mar 28, 2017 at 5:27 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 4:01 AM, Ralph Sennhauser
>> <ralph.sennhauser@gmail.com> wrote:
>>> Hi Amir
>>>
>>> Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
>>> breaks squashfs with an ubifs overlay (both ubi volumes of the same
>>> container).
>>>
>>
>> Hi Ralph,
>>
>> I am confused by the description above. Which are the 'both ubi volumes'?
>>
>> Can you provide exact command of overlayfs mount, preferably
>> also a script to generate the lower/upper images and mount them
>> to remove any mkfs option doubts from test setup.
>>
>>> Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
>>> ubifs_add_orphan: orphaned twice". This corrupts the the filesystem and
>>> the next attempt to mount the overlay will fail.
>>>
> 
> Looking at the ubifs code, it does not appear that ubifs_link() ever calls
> ubifs_delete_orphan() for the case of linking a temp file (nlink == 0),
> so this looks like a ubifs bug.

Thanks for the heads up, I'll look into this.

> Is ubifs being tested with xfstests? This should have been caught by test
> generic/004 (link tempfile then delete it).

Yes, it is.
David, can you please double-check wrt. test generic/004?

Thanks,
//richard

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

On Tue, 28 Mar 2017 05:27:03 -0400
Amir Goldstein <amir73il@gmail.com> wrote:

> On Tue, Mar 28, 2017 at 4:01 AM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
> > Hi Amir
> >
> > Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
> > breaks squashfs with an ubifs overlay (both ubi volumes of the same
> > container).
> >  
> 
> Hi Ralph,
> 
> I am confused by the description above. Which are the 'both ubi
> volumes'?

The ubi container has two volumes, the first is a squashfs, the second
volume an ubifs. The latter is mounted as an overlay.
 
> 
> Can you provide exact command of overlayfs mount, preferably
> also a script to generate the lower/upper images and mount them
> to remove any mkfs option doubts from test setup.

Both I mount from the initramfs as follows (rom / overlay are empty in
the initramfs):

  mount -o rw,nosuid,nodev,noexec,noatime -t proc proc /proc || rescue_shell "proc"
  mount -o rw,nosuid,nodev,noexec,noatime -t sysfs sysfs /sys || rescue_shell "sys"
  mount -o rw,nosuid -t devtmpfs devtmpfs /dev || rescue_shell "dev"

  ubiattach -m $(get_mtd_from_root_arg) /dev/ubi_ctrl || rescue_shell "attach"
  ubiblock --create /dev/ubi0_0 || rescue_shell || "block"

  mount -o ro -t squashfs /dev/ubiblock0_0 /rom || rescue_shell "mount rootfs"
  mount -o rw,noatime -t ubifs /dev/ubi0_1 /overlay || rescue_shell "mount rootfs_data"

  mkdir -p /overlay/upper || rescue_shell "mkdir upper"
  mkdir -p /overlay/work || rescue_shell "mkdir work"

  mount -o rw,noatime,lowerdir=/rom,upperdir=/overlay/upper,workdir=/overlay/work \
          -t overlay overlay /newroot || rescue_shell "mount overlay"

  mount --move /rom /newroot/rom || rescue_shell "move rootfs"
  mount --move /overlay /newroot/overlay || rescue_shell "move rootfs_data"

  mount --move /dev /newroot/dev || rescue_shell "move dev"
  mount --move /sys /newroot/sys || rescue_shell "move sys"
  mount --move /proc /newroot/proc || rescue_shell "move proc"

  exec switch_root /newroot /sbin/init

I use OpenWrt as a basis, replacing the kernel with a vanilla one.

The options used to generate the file systems are:

Squashfs:  -p 128KiB -m 2048 -s 512 -O 2048
Ubifs: -m 2048 -e 124KiB -c 4096 -F

> 
> > Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
> > ubifs_add_orphan: orphaned twice". This corrupts the the filesystem
> > and the next attempt to mount the overlay will fail.
> >  
> 
> Does that happen on any attempt to rename a file?
> A file that was only is lower I suppose?

That's how I trigger it, yes. Can reproduce it on any attempt.

> Can you provide a simple script with your test, setting up the
> lower/upper files and triggering the bug.

Any more you need than the above mount script? A call to "mv somefile
somefile.back && reboot" on a fresh install is all I do.

Thanks
Ralph

PS: Reverting 01ad3eb8a073 ("ovl: concurrent copy up of regular files")
as a dependency and the commit mentioned in Subject fix the issue for
me. Tested on v4.11-rc4 and next-20170327.

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Tue, Mar 28, 2017 at 6:45 AM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> On Tue, 28 Mar 2017 05:27:03 -0400
> Amir Goldstein <amir73il@gmail.com> wrote:
>
>> On Tue, Mar 28, 2017 at 4:01 AM, Ralph Sennhauser
>> <ralph.sennhauser@gmail.com> wrote:
>> > Hi Amir
>> >
>> > Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE")
>> > breaks squashfs with an ubifs overlay (both ubi volumes of the same
>> > container).
>> >
>>
>> Hi Ralph,
>>
>> I am confused by the description above. Which are the 'both ubi
>> volumes'?
>
> The ubi container has two volumes, the first is a squashfs, the second
> volume an ubifs. The latter is mounted as an overlay.
>
>>
>> Can you provide exact command of overlayfs mount, preferably
>> also a script to generate the lower/upper images and mount them
>> to remove any mkfs option doubts from test setup.
>
> Both I mount from the initramfs as follows (rom / overlay are empty in
> the initramfs):
>
>   mount -o rw,nosuid,nodev,noexec,noatime -t proc proc /proc || rescue_shell "proc"
>   mount -o rw,nosuid,nodev,noexec,noatime -t sysfs sysfs /sys || rescue_shell "sys"
>   mount -o rw,nosuid -t devtmpfs devtmpfs /dev || rescue_shell "dev"
>
>   ubiattach -m $(get_mtd_from_root_arg) /dev/ubi_ctrl || rescue_shell "attach"
>   ubiblock --create /dev/ubi0_0 || rescue_shell || "block"
>
>   mount -o ro -t squashfs /dev/ubiblock0_0 /rom || rescue_shell "mount rootfs"
>   mount -o rw,noatime -t ubifs /dev/ubi0_1 /overlay || rescue_shell "mount rootfs_data"
>
>   mkdir -p /overlay/upper || rescue_shell "mkdir upper"
>   mkdir -p /overlay/work || rescue_shell "mkdir work"
>
>   mount -o rw,noatime,lowerdir=/rom,upperdir=/overlay/upper,workdir=/overlay/work \
>           -t overlay overlay /newroot || rescue_shell "mount overlay"
>
>   mount --move /rom /newroot/rom || rescue_shell "move rootfs"
>   mount --move /overlay /newroot/overlay || rescue_shell "move rootfs_data"
>
>   mount --move /dev /newroot/dev || rescue_shell "move dev"
>   mount --move /sys /newroot/sys || rescue_shell "move sys"
>   mount --move /proc /newroot/proc || rescue_shell "move proc"
>
>   exec switch_root /newroot /sbin/init
>
> I use OpenWrt as a basis, replacing the kernel with a vanilla one.
>
> The options used to generate the file systems are:
>
> Squashfs:  -p 128KiB -m 2048 -s 512 -O 2048
> Ubifs: -m 2048 -e 124KiB -c 4096 -F
>
>>
>> > Renaming a file results in an error "UBIFS error (ubi0:1 pid 1394):
>> > ubifs_add_orphan: orphaned twice". This corrupts the the filesystem
>> > and the next attempt to mount the overlay will fail.
>> >
>>
>> Does that happen on any attempt to rename a file?
>> A file that was only is lower I suppose?
>
> That's how I trigger it, yes. Can reproduce it on any attempt.
>
>> Can you provide a simple script with your test, setting up the
>> lower/upper files and triggering the bug.
>
> Any more you need than the above mount script? A call to "mv somefile
> somefile.back && reboot" on a fresh install is all I do.
>
> Thanks
> Ralph
>
> PS: Reverting 01ad3eb8a073 ("ovl: concurrent copy up of regular files")
> as a dependency and the commit mentioned in Subject fix the issue for
> me. Tested on v4.11-rc4 and next-20170327.

That is not surprising.
Overlayfs now uses O_TMPFILE for copy up and it works fine with all the
file systems I tested (tmpfs, xfs, ext4).
If I am right and O_TMPFILE is broken in ubifs, you are most likely the first
person to test it (indirectly by overlayfs).

Please try to reproduce the bug with following patch to disable ubifs
O_TMPFILE support:

--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1685,7 +1685,7 @@ const struct inode_operations
ubifs_dir_inode_operations = {
 #ifdef CONFIG_UBIFS_ATIME_SUPPORT
        .update_time = ubifs_update_time,
 #endif
-       .tmpfile     = ubifs_tmpfile,
+       //.tmpfile     = ubifs_tmpfile,
 };

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

Hi Amir,

On Tue, 28 Mar 2017 07:03:11 -0400
Amir Goldstein <amir73il@gmail.com> wrote:

> Overlayfs now uses O_TMPFILE for copy up and it works fine with all
> the file systems I tested (tmpfs, xfs, ext4).
> If I am right and O_TMPFILE is broken in ubifs, you are most likely
> the first person to test it (indirectly by overlayfs).
> 
> Please try to reproduce the bug with following patch to disable ubifs
> O_TMPFILE support:
> 
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -1685,7 +1685,7 @@ const struct inode_operations
> ubifs_dir_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>         .update_time = ubifs_update_time,
>  #endif
> -       .tmpfile     = ubifs_tmpfile,
> +       //.tmpfile     = ubifs_tmpfile,
>  };

Get a unused warning during build but all seems to be working fine now.

Thanks
Ralph

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Tue, Mar 28, 2017 at 7:28 AM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> Hi Amir,
>
> On Tue, 28 Mar 2017 07:03:11 -0400
> Amir Goldstein <amir73il@gmail.com> wrote:
>
>> Overlayfs now uses O_TMPFILE for copy up and it works fine with all
>> the file systems I tested (tmpfs, xfs, ext4).
>> If I am right and O_TMPFILE is broken in ubifs, you are most likely
>> the first person to test it (indirectly by overlayfs).
>>
>> Please try to reproduce the bug with following patch to disable ubifs
>> O_TMPFILE support:
>>
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -1685,7 +1685,7 @@ const struct inode_operations
>> ubifs_dir_inode_operations = {
>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>         .update_time = ubifs_update_time,
>>  #endif
>> -       .tmpfile     = ubifs_tmpfile,
>> +       //.tmpfile     = ubifs_tmpfile,
>>  };
>
> Get a unused warning during build but all seems to be working fine now.
>

OK. I'll wait for ubifs developers to fix the bug.
Otherwise, I'll send a proper patch to disable ubifs O_TMPFILE support.
Will add tested-by you.

Thanks!

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

On Tue, 28 Mar 2017 08:08:51 -0400
Amir Goldstein <amir73il@gmail.com> wrote:

> On Tue, Mar 28, 2017 at 7:28 AM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
> > Hi Amir,
> >
> > On Tue, 28 Mar 2017 07:03:11 -0400
> > Amir Goldstein <amir73il@gmail.com> wrote:
> >  
> >> Overlayfs now uses O_TMPFILE for copy up and it works fine with all
> >> the file systems I tested (tmpfs, xfs, ext4).
> >> If I am right and O_TMPFILE is broken in ubifs, you are most likely
> >> the first person to test it (indirectly by overlayfs).
> >>
> >> Please try to reproduce the bug with following patch to disable
> >> ubifs O_TMPFILE support:
> >>
> >> --- a/fs/ubifs/dir.c
> >> +++ b/fs/ubifs/dir.c
> >> @@ -1685,7 +1685,7 @@ const struct inode_operations
> >> ubifs_dir_inode_operations = {
> >>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
> >>         .update_time = ubifs_update_time,
> >>  #endif
> >> -       .tmpfile     = ubifs_tmpfile,
> >> +       //.tmpfile     = ubifs_tmpfile,
> >>  };  
> >
> > Get a unused warning during build but all seems to be working fine
> > now. 
> 
> OK. I'll wait for ubifs developers to fix the bug.
> Otherwise, I'll send a proper patch to disable ubifs O_TMPFILE
> support. Will add tested-by you.

Sounds like a good plan, there is still time for 4.11-rc5. Fine with
you adding my tested-by in case it will come to this.

Appreciated
Ralph

> 
> Thanks!

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Tue, Mar 28, 2017 at 3:16 PM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> On Tue, 28 Mar 2017 08:08:51 -0400
> Amir Goldstein <amir73il@gmail.com> wrote:
>
>> On Tue, Mar 28, 2017 at 7:28 AM, Ralph Sennhauser
>> <ralph.sennhauser@gmail.com> wrote:
>> > Hi Amir,
>> >
>> > On Tue, 28 Mar 2017 07:03:11 -0400
>> > Amir Goldstein <amir73il@gmail.com> wrote:
>> >
>> >> Overlayfs now uses O_TMPFILE for copy up and it works fine with all
>> >> the file systems I tested (tmpfs, xfs, ext4).
>> >> If I am right and O_TMPFILE is broken in ubifs, you are most likely
>> >> the first person to test it (indirectly by overlayfs).
>> >>
>> >> Please try to reproduce the bug with following patch to disable
>> >> ubifs O_TMPFILE support:
>> >>
>> >> --- a/fs/ubifs/dir.c
>> >> +++ b/fs/ubifs/dir.c
>> >> @@ -1685,7 +1685,7 @@ const struct inode_operations
>> >> ubifs_dir_inode_operations = {
>> >>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>> >>         .update_time = ubifs_update_time,
>> >>  #endif
>> >> -       .tmpfile     = ubifs_tmpfile,
>> >> +       //.tmpfile     = ubifs_tmpfile,
>> >>  };
>> >
>> > Get a unused warning during build but all seems to be working fine
>> > now.
>>
>> OK. I'll wait for ubifs developers to fix the bug.
>> Otherwise, I'll send a proper patch to disable ubifs O_TMPFILE
>> support. Will add tested-by you.
>
> Sounds like a good plan, there is still time for 4.11-rc5. Fine with
> you adding my tested-by in case it will come to this.
>


Ralph,

Can you please try to reproduce the problem with this command
on ubifs:

# create and link a tmpfile - then remove it
sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo; sudo rm foo

That's the gist of xfstest generic/004.

You should expect the same behavior of corrupted fs after boot.

This issue should be reproduced since kernel 4.9 when ubifs tmpfile
support was added.

I hope this info can help the ubifs developers fix the problem
independently from overlayfs setup.

Thanks,
Amir.

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

On Wed, 29 Mar 2017 22:16:10 +0300
Amir Goldstein <amir73il@gmail.com> wrote:

> On Tue, Mar 28, 2017 at 3:16 PM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
> > On Tue, 28 Mar 2017 08:08:51 -0400
> > Amir Goldstein <amir73il@gmail.com> wrote:
> >  
> >> On Tue, Mar 28, 2017 at 7:28 AM, Ralph Sennhauser
> >> <ralph.sennhauser@gmail.com> wrote:  
> >> > Hi Amir,
> >> >
> >> > On Tue, 28 Mar 2017 07:03:11 -0400
> >> > Amir Goldstein <amir73il@gmail.com> wrote:
> >> >  
> >> >> Overlayfs now uses O_TMPFILE for copy up and it works fine with
> >> >> all the file systems I tested (tmpfs, xfs, ext4).
> >> >> If I am right and O_TMPFILE is broken in ubifs, you are most
> >> >> likely the first person to test it (indirectly by overlayfs).
> >> >>
> >> >> Please try to reproduce the bug with following patch to disable
> >> >> ubifs O_TMPFILE support:
> >> >>
> >> >> --- a/fs/ubifs/dir.c
> >> >> +++ b/fs/ubifs/dir.c
> >> >> @@ -1685,7 +1685,7 @@ const struct inode_operations
> >> >> ubifs_dir_inode_operations = {
> >> >>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
> >> >>         .update_time = ubifs_update_time,
> >> >>  #endif
> >> >> -       .tmpfile     = ubifs_tmpfile,
> >> >> +       //.tmpfile     = ubifs_tmpfile,
> >> >>  };  
> >> >
> >> > Get a unused warning during build but all seems to be working
> >> > fine now.  
> >>
> >> OK. I'll wait for ubifs developers to fix the bug.
> >> Otherwise, I'll send a proper patch to disable ubifs O_TMPFILE
> >> support. Will add tested-by you.  
> >
> > Sounds like a good plan, there is still time for 4.11-rc5. Fine with
> > you adding my tested-by in case it will come to this.
> >  
> 
> 
> Ralph,
> 
> Can you please try to reproduce the problem with this command
> on ubifs:

Replaced the squashfs with the ubifs overlay with a plain ubifs root.

> 
> # create and link a tmpfile - then remove it
> sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo; sudo rm
> foo

next-20170328, obviously without your patch:

  # xfs_io -T -c "flink foo" .
  .: Not supported
  # xfs_io -V
  xfs_io version 4.9.0
  # strace xfs_io -T -c "flink foo" .
  execve("/sbin/xfs_io", ["xfs_io", "-T", "-c", "flink foo", "."], [/* 8 vars */]) = 0
  set_tls(0xb6f17544, 0xbee0ac10, 0xb6f180a0, 0, 0xb6f1749c) = 0
  set_tid_address(0xb6f174b8)             = 2556
  open("/etc/ld-musl-armhf.path", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENOENT (No such file or directory)
  open("/lib/libgcc_s.so.1", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
  fcntl64(3, F_SETFD, FD_CLOEXEC)         = 0
  fstat64(3, {st_mode=S_IFREG|0644, st_size=38203, ...}) = 0
  read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0(\0\1\0\0\0\240;\0\0004\0\0\0"..., 936) = 936
  mmap2(NULL, 106496, PROT_READ|PROT_EXEC, MAP_PRIVATE, 3, 0) = 0xb6e88000
  mmap2(0xb6ea1000, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED, 3, 0x9000) = 0xb6ea1000
  close(3)                                = 0
  mprotect(0xb6f15000, 4096, PROT_READ)   = 0
  mprotect(0x38000, 4096, PROT_READ)      = 0
  clock_gettime(CLOCK_REALTIME, {1490822261, 558395832}) = 0
  open(".", O_RDWR|O_LARGEFILE|O_DIRECTORY|O_TMPFILE) = -1 EOPNOTSUPP (Not supported)
  writev(2, [{iov_base="", iov_len=0}, {iov_base=".", iov_len=1}], 2.) = 1
  writev(2, [{iov_base="", iov_len=0}, {iov_base=":", iov_len=1}], 2:) = 1
  writev(2, [{iov_base="", iov_len=0}, {iov_base=" ", iov_len=1}], 2 ) = 1
  writev(2, [{iov_base="", iov_len=0}, {iov_base="Not supported", iov_len=13}], 2Not supported) = 13
  writev(2, [{iov_base="", iov_len=0}, {iov_base="\n", iov_len=1}], 2
  ) = 1
  exit_group(1)                           = ?
  +++ exited with 1 +++


Need some sleep before looking into what might be going on here.

Ralph

> 
> That's the gist of xfstest generic/004.
> 
> You should expect the same behavior of corrupted fs after boot.
> 
> This issue should be reproduced since kernel 4.9 when ubifs tmpfile
> support was added.
> 
> I hope this info can help the ubifs developers fix the problem
> independently from overlayfs setup.
> 
> Thanks,
> Amir.

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Richard Weinberger]

Ralph,

Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:
>> # create and link a tmpfile - then remove it
>> sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo; sudo rm
>> foo
> 
> next-20170328, obviously without your patch:
> 
>   # xfs_io -T -c "flink foo" .
>   .: Not supported

-T tells xfs_io to create a tmpfile but you have it disabled in UBIFS.

Anyway, can you please test the attached patch?
Amir was right, UBIFS misses a corner case in ubifs_link(). ;-\

I think I understand also why we never noticed this in xfstests, UBIFS prints
only a non-fatal error while umounting the filesystem in this case.
But will test tomorrow in more detail, need some sleep now.

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 30825d882aa9..95e348a2da29 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -748,6 +748,9 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 		goto out_fname;

 	lock_2_inodes(dir, inode);
+	if (inode->i_nlink == 0)
+		ubifs_delete_orphan(c, inode->i_ino);
+
 	inc_nlink(inode);
 	ihold(inode);
 	inode->i_ctime = ubifs_current_time(inode);

Thanks,
//richard

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

Hi Richard,

On Thu, 30 Mar 2017 00:15:31 +0200
Richard Weinberger <richard@nod.at> wrote:

> Ralph,
> 
> Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:
> >> # create and link a tmpfile - then remove it
> >> sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo; sudo
> >> rm foo  
> > 
> > next-20170328, obviously without your patch:
> > 
> >   # xfs_io -T -c "flink foo" .
> >   .: Not supported  
> 
> -T tells xfs_io to create a tmpfile but you have it disabled in UBIFS.

Bash history says I booted the other partition than I flashed (uname
just happened to match), the downside of the dual boot layout. :)

  # rm -rf foo
  # xfs_io -T -c "flink foo" .
  # ls -l foo
  -rw-------    1 root     root             0 Mar 30 02:00 foo
  # rm foo
  [  305.001436] UBIFS error (ubi0:0 pid 2493): ubifs_add_orphan: orphaned twice

However, unlike with the overlay setup the filesystem can be mounted
just fine without imediatly visible side effects.

> 
> Anyway, can you please test the attached patch?
> Amir was right, UBIFS misses a corner case in ubifs_link(). ;-\
> 
> I think I understand also why we never noticed this in xfstests,
> UBIFS prints only a non-fatal error while umounting the filesystem in
> this case. But will test tomorrow in more detail, need some sleep now.
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 30825d882aa9..95e348a2da29 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -748,6 +748,9 @@ static int ubifs_link(struct dentry *old_dentry,
> struct inode *dir, goto out_fname;
> 
>  	lock_2_inodes(dir, inode);
> +	if (inode->i_nlink == 0)
> +		ubifs_delete_orphan(c, inode->i_ino);
> +
>  	inc_nlink(inode);
>  	ihold(inode);
>  	inode->i_ctime = ubifs_current_time(inode);
> 
> Thanks,
> //richard

With this patch I'm no longer able to reproduce _this_ issue, however,
rename no longer works either:

  # mv /etc/config/wireless /etc/config/wireless.back
  mv: can't rename '/etc/config/wireless': Invalid argument
  # ls /etc/config/wireless
  /etc/config/wireless
  # rm /etc/config/wireless
  # ls /etc/config/wireless
  ls: /etc/config/wireless: No such file or directory


Thanks
Ralph

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Amir Goldstein]

On Thu, Mar 30, 2017 at 8:53 AM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> Hi Richard,
>
> On Thu, 30 Mar 2017 00:15:31 +0200
> Richard Weinberger <richard@nod.at> wrote:
>
>> Ralph,
>>
>> Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:
>> >> # create and link a tmpfile - then remove it
>> >> sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo; sudo
>> >> rm foo
>> >
>> > next-20170328, obviously without your patch:
>> >
>> >   # xfs_io -T -c "flink foo" .
>> >   .: Not supported
>>
>> -T tells xfs_io to create a tmpfile but you have it disabled in UBIFS.
>
> Bash history says I booted the other partition than I flashed (uname
> just happened to match), the downside of the dual boot layout. :)
>
>   # rm -rf foo
>   # xfs_io -T -c "flink foo" .
>   # ls -l foo
>   -rw-------    1 root     root             0 Mar 30 02:00 foo
>   # rm foo
>   [  305.001436] UBIFS error (ubi0:0 pid 2493): ubifs_add_orphan: orphaned twice
>
> However, unlike with the overlay setup the filesystem can be mounted
> just fine without imediatly visible side effects.
>

Maybe try to rename instead of rm . that's how you reproduces with overlayfs.

>>
>> Anyway, can you please test the attached patch?
>> Amir was right, UBIFS misses a corner case in ubifs_link(). ;-\
>>
>> I think I understand also why we never noticed this in xfstests,
>> UBIFS prints only a non-fatal error while umounting the filesystem in
>> this case. But will test tomorrow in more detail, need some sleep now.
>>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 30825d882aa9..95e348a2da29 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -748,6 +748,9 @@ static int ubifs_link(struct dentry *old_dentry,
>> struct inode *dir, goto out_fname;
>>
>>       lock_2_inodes(dir, inode);
>> +     if (inode->i_nlink == 0)
>> +             ubifs_delete_orphan(c, inode->i_ino);
>> +
>>       inc_nlink(inode);
>>       ihold(inode);
>>       inode->i_ctime = ubifs_current_time(inode);
>>
>> Thanks,
>> //richard
>
> With this patch I'm no longer able to reproduce _this_ issue, however,
> rename no longer works either:
>
>   # mv /etc/config/wireless /etc/config/wireless.back
>   mv: can't rename '/etc/config/wireless': Invalid argument
>   # ls /etc/config/wireless
>   /etc/config/wireless
>   # rm /etc/config/wireless
>   # ls /etc/config/wireless
>   ls: /etc/config/wireless: No such file or directory
>
>
> Thanks
> Ralph

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Richard Weinberger]

Ralph,

Am 30.03.2017 um 07:53 schrieb Ralph Sennhauser:
> With this patch I'm no longer able to reproduce _this_ issue, however,
> rename no longer works either:
> 
>   # mv /etc/config/wireless /etc/config/wireless.back
>   mv: can't rename '/etc/config/wireless': Invalid argument
>   # ls /etc/config/wireless
>   /etc/config/wireless
>   # rm /etc/config/wireless
>   # ls /etc/config/wireless
>   ls: /etc/config/wireless: No such file or directory
> 

Please apply this one too:
http://lists.infradead.org/pipermail/linux-mtd/2017-March/072613.html

Thanks,
//richard

Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

[Ralph Sennhauser]

Hi Richard,

On Thu, 30 Mar 2017 08:56:57 +0200
Richard Weinberger <richard@nod.at> wrote:

>     
> Ralph,
> Please apply this
> fix:http://lists.infradead.org/pipermail/linux-mtd/2017-March/072613.html
> 

No longer able to observe an issue with this one on top. The two
patches seem all that was needed to get things back to a working state.

Do you want me to test anything else than linux-next?

Thanks
Ralph

> 
> Von meinem Samsung Gerät gesendet.
> 
> -------- Ursprüngliche Nachricht --------
> Von: Ralph Sennhauser <ralph.sennhauser@gmail.com> 
> Datum: 30.03.17  07:53  (GMT+01:00) 
> An: Richard Weinberger <richard@nod.at> 
> Cc: Amir Goldstein <amir73il@gmail.com>, Miklos Szeredi
> <miklos@szeredi.hu>, linux-unionfs@vger.kernel.org, linux-kernel
> <linux-kernel@vger.kernel.org>, linux-mtd@lists.infradead.org,
> regressions@leemhuis.info, Artem Bityutskiy <dedekind1@gmail.com>,
> Adrian Hunter <adrian.hunter@intel.com>, David Oberhollenzer
> <goliath@sigma-star.at> Betreff: Re: [REGRESSION 4.11] Commit
> d8514d8edb5b ("ovl: copy up regular\r&nbsp; file using O_TMPFILE") breaks ubifs 
> 
> Hi Richard,
> 
> On Thu, 30 Mar 2017 00:15:31 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
> > Ralph,
> > 
> > Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:  
> > >> # create and link a tmpfile - then remove it
> > >> sudo rm -rf foo; sudo xfs_io -T -c "flink foo" . ; ls -l foo;
> > >> sudo rm foo    
> > > 
> > > next-20170328, obviously without your patch:
> > > 
> > >   # xfs_io -T -c "flink foo" .
> > >   .: Not supported    
> > 
> > -T tells xfs_io to create a tmpfile but you have it disabled in
> > UBIFS.  
> 
> Bash history says I booted the other partition than I flashed (uname
> just happened to match), the downside of the dual boot layout. :)
> 
>   # rm -rf foo
>   # xfs_io -T -c "flink foo" .
>   # ls -l foo
>   -rw-------    1 root     root             0 Mar 30 02:00 foo
>   # rm foo
>   [  305.001436] UBIFS error (ubi0:0 pid 2493): ubifs_add_orphan:
> orphaned twice
> 
> However, unlike with the overlay setup the filesystem can be mounted
> just fine without imediatly visible side effects.
> 
> > 
> > Anyway, can you please test the attached patch?
> > Amir was right, UBIFS misses a corner case in ubifs_link(). ;-\
> > 
> > I think I understand also why we never noticed this in xfstests,
> > UBIFS prints only a non-fatal error while umounting the filesystem
> > in this case. But will test tomorrow in more detail, need some
> > sleep now.
> > 
> > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> > index 30825d882aa9..95e348a2da29 100644
> > --- a/fs/ubifs/dir.c
> > +++ b/fs/ubifs/dir.c
> > @@ -748,6 +748,9 @@ static int ubifs_link(struct dentry *old_dentry,
> > struct inode *dir, goto out_fname;
> > 
> >  	lock_2_inodes(dir, inode);
> > +	if (inode->i_nlink == 0)
> > +		ubifs_delete_orphan(c, inode->i_ino);
> > +
> >  	inc_nlink(inode);
> >  	ihold(inode);
> >  	inode->i_ctime = ubifs_current_time(inode);
> > 
> > Thanks,
> > //richard  
> 
> With this patch I'm no longer able to reproduce _this_ issue, however,
> rename no longer works either:
> 
>   # mv /etc/config/wireless /etc/config/wireless.back
>   mv: can't rename '/etc/config/wireless': Invalid argument
>   # ls /etc/config/wireless
>   /etc/config/wireless
>   # rm /etc/config/wireless
>   # ls /etc/config/wireless
>   ls: /etc/config/wireless: No such file or directory
> 
> 
> Thanks
> Ralph