linux-ext4 - Re: [PATCH] ext4: fix deadlock in ext4_xattr_inode

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240620031044.GB1553731@mit.edu>
Date: Wed, 19 Jun 2024 23:10:44 -0400
From: "Theodore Ts'o" <tytso@....edu>
To: Edward Adam Davis <eadavis@...com>
Cc: syzbot+ee72b9a7aad1e5a77c5c@...kaller.appspotmail.com,
        adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] ext4: fix deadlock in ext4_xattr_inode_iget

On Thu, Apr 04, 2024 at 09:54:02AM +0800, Edward Adam Davis wrote:
> According to mark inode dirty context, it does not need to be protected by lock
> i_data_sem, and if it is protected by i_data_sem, a deadlock will occur.

The i_data_sem lock is not to protect mark_inode_dirty_context, but to
avoid races with the writeback code, which you can see right before
you added the down_write() line.

More detail about why it is necessary can be found in commit
90e775b71ac4 ("ext4: fix lost truncate due to race with writeback"):

    The following race can lead to a loss of i_disksize update from truncate
    thus resulting in a wrong inode size if the inode size isn't updated
    again before inode is reclaimed:

    ext4_setattr()                          mpage_map_and_submit_extent()
      EXT4_I(inode)->i_disksize = attr->ia_size;
      ...                                     ...
                                              disksize = ((loff_t)mpd->first_page) << PAGE_CACHE_SHIFT
                                              /* False because i_size isn't
                                               * updated yet */
                                              if (disksize > i_size_read(inode))
                                              /* True, because i_disksize is
                                               * already truncated */
                                              if (disksize > EXT4_I(inode)->i_disksize)
                                                /* Overwrite i_disksize
                                                 * update from truncate */
                                                ext4_update_i_disksize()
      i_size_write(inode, attr->ia_size);

    For other places updating i_disksize such race cannot happen because
    i_mutex prevents these races. Writeback is the only place where we do
    not hold i_mutex and we cannot grab it there because of lock ordering.

    We fix the race by doing both i_disksize and i_size update in truncate
    Atomically under i_data_sem and in mpage_map_and_submit_extent() we move
    the check against i_size under i_data_sem as well.

So your proposed fix would introduce a regression by re-enabling the
bug which is fixed by commit 90e775b71ac4.

In any case, as Andreas has pointed out, this is a false positive; the
supposed deadlock involves an ea_inode in stack trace #0, whereas the
stack trace #1 involves a write to a data inode.  Andreas has
suggested fixing this by annotating the lock appropriately.  This case
is not going to happen in real production systems today, since
triggering it requires using the debugging mount option
debug_want_extra_isize.

So while it would be good to avoid the false positive lockdep warning,
fixing this is a lower priority bug --- it certainly isn't security
issue that syzbot developers like to point at when talking about the
"Linux security disaster".  It isn't even a real production level bug!

Cheers,

      	       		  	     - Ted