linux-kernel - Re: [PATCH] ubifs: Fix a potential space leak problem while linking tmpfile

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0c543297-d94f-ad40-7dd0-2198f39336bb@huawei.com>
Date:   Sat, 11 Jul 2020 14:37:18 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Richard Weinberger <richard@....at>
CC:     Richard Weinberger <richard.weinberger@...il.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        yi zhang <yi.zhang@...wei.com>
Subject: Re: [PATCH] ubifs: Fix a potential space leak problem while linking
 tmpfile

在 2020/7/7 20:47, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>>>> Perhaps I misunderstood what commit 32fe905c17f001 ("ubifs: Fix
>>>> O_TMPFILE corner case in ubifs_link()") wanted to fix.
>>>> I think orphan area is used to remind filesystem don't forget to delete
>>>> inodes (whose nlink is 0) in next unclean rebooting. Generally, the file
>>>> system is not corrupted caused by replaying orphan nodes.
>>>> Ralph reported a filesystem corruption in combination with overlayfs.
>>>> Can you tell me the details about that problem? Thanks.
>>> On my test bed I didn't see a fs corruption, what I saw was a failing orphan
>>> self test while playing with O_TMPFILE and linkat().
>> Do we have a reproducer, or can I get the fail testcase? Is it a xfstest
>> case?
> I think xfstests triggered it, yes.
> Later today I can check. :)
>
> Thanks,
> //richard
>
> .

I think I have found the testcases, overlay/006 and overlay/041.

The 'mv' and 'rm' operations will put lowertestfile into orphan list 
twice, so we must reseve the orphan deletion operation in ubifs_link(), 
otherwise the testcase fails and we will see the following msg:

   overlay/006 2s ... - output mismatch (see 
/root/git/xfstests-dev/results//overlay/006.out.bad)
     --- tests/overlay/006.out    2020-07-07 21:42:57.737000000 +0800
     +++ /root/git/xfstests-dev/results//overlay/006.out.bad 2020-07-11 
14:31:55.340000000 +0800
     @@ -1,2 +1,4 @@
      QA output created by 006
      Silence is golden
     +rm: cannot remove 
'/tmp/scratch/ovl-mnt/uppertestdir/lowertestfile': Invalid argument
     +lowertestfile
     ...

   [  382.258210] UBIFS error (ubi0:1 pid 11896): orphan_add [ubifs]: 
orphaned twice
   [  382.352535] UBIFS error (ubi0:1 pid 11930): free_orphans [ubifs]: 
orphan list not empty at unmount


So, how about moving ubifs_delete_orphan() after ubifs_jnl_update() in 
function ubifs_link(). Following modifications applied in linux-5.8 has 
been tested by overlay/041, overlay/006 and  other tmpfile cases 
(generic/531, generic/530, generic/509, generic/389, generic/004).

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index ef85ec167a84..fd4443a5e8c6 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -722,11 +722,6 @@ static int ubifs_link(struct dentry *old_dentry, 
struct inode *dir,
                 goto out_fname;

         lock_2_inodes(dir, inode);
-
-       /* Handle O_TMPFILE corner case, it is allowed to link a 
O_TMPFILE. */
-       if (inode->i_nlink == 0)
-               ubifs_delete_orphan(c, inode->i_ino);
-
inc_nlink(inode);
ihold(inode);
         inode->i_ctime = current_time(inode);
@@ -736,6 +731,11 @@ static int ubifs_link(struct dentry *old_dentry, 
struct inode *dir,
         err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
         if (err)
                 goto out_cancel;
+
+       /* Handle O_TMPFILE corner case, it is allowed to link a 
O_TMPFILE. */
+       if (inode->i_nlink == 1)
+               ubifs_delete_orphan(c, inode->i_ino);
+
         unlock_2_inodes(dir, inode);

         ubifs_release_budget(c, &req);
@@ -747,8 +747,6 @@ static int ubifs_link(struct dentry *old_dentry, 
struct inode *dir,
         dir->i_size -= sz_change;
         dir_ui->ui_size = dir->i_size;
drop_nlink(inode);
-       if (inode->i_nlink == 0)
-               ubifs_add_orphan(c, inode->i_ino);
         unlock_2_inodes(dir, inode);
         ubifs_release_budget(c, &req);
iput(inode);
--