Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs

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.

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

Quoted 6 lines · 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).

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.

Quoted 4 lines

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.

Quoted 17 lines

Quoted 6 lines · 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).

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.

Quoted 4 lines

Quoted 4 lines

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).

Quoted 6 lines · 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).

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.

Quoted 5 lines

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.

Quoted 9 lines · On Tue, 28 Mar 2017 05:27:03 -0400 Amir Goldstein <[email protected]> wrote:

Quoted 6 lines · 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).

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

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:

Hi Amir,

Quoted 8 lines · On Tue, 28 Mar 2017 07:03:11 -0400 Amir Goldstein <[email protected]> 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:

fs/ubifs/dir.c const struct inode_operations · line 1690

Quoted 10 lines

fs/ubifs/dir.c +1 -1

@@ -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.

Quoted 13 lines · Ralph Sennhauser

Hi Amir,

Quoted 8 lines · On Tue, 28 Mar 2017 07:03:11 -0400 Amir Goldstein <[email protected]> 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:

Quoted 13 lines

fs/ubifs/dir.c const struct inode_operations · line 1690

Quoted 10 lines

fs/ubifs/dir.c +1 -1

@@ -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!

Quoted 29 lines · On Tue, 28 Mar 2017 08:08:51 -0400 Amir Goldstein <[email protected]> wrote:

Quoted 26 lines · Ralph Sennhauser

Hi Amir,

Quoted 11 lines · On Tue, 28 Mar 2017 07:03:11 -0400 Amir Goldstein <[email protected]> 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:

fs/ubifs/dir.c const struct inode_operations · line 1690

Quoted 7 lines

fs/ubifs/dir.c +1 -1

@@ -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.

Quoted 3 lines

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.

Quoted 32 lines · Ralph Sennhauser

Quoted 29 lines · On Tue, 28 Mar 2017 08:08:51 -0400 Amir Goldstein <[email protected]> wrote:

Quoted 26 lines · Ralph Sennhauser

Hi Amir,

Quoted 11 lines · On Tue, 28 Mar 2017 07:03:11 -0400 Amir Goldstein <[email protected]> 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:

fs/ubifs/dir.c const struct inode_operations · line 1690

Quoted 7 lines

fs/ubifs/dir.c +1 -1

@@ -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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

# 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

Ralph,

Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:

Quoted 4 lines

Quoted 3 lines

# 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  

Quoted 4 lines

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.

fs/ubifs/dir.c +3 -0

index 30825d882aa9..95e348a2da29 100644
@@ -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

Hi Richard,

Quoted 7 lines · On Thu, 30 Mar 2017 00:15:31 +0200 Richard Weinberger <[email protected]> wrote:

Ralph,

Am 29.03.2017 um 23:26 schrieb Ralph Sennhauser:

Quoted 4 lines

Quoted 3 lines

# 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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 23 lines

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.

fs/ubifs/dir.c +3 -0

index 30825d882aa9..95e348a2da29 100644
@@ -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);

Quoted 2 lines

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

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