linux-ext4 - Re: Can ext4_break_layouts() ever fail?

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]

Date:   Mon, 20 May 2019 10:23:40 +0200
From:   Jan Kara <jack@...e.cz>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Jan Kara <jack@...e.cz>, Theodore Ts'o <tytso@....edu>,
        linux-ext4@...r.kernel.org, Dan Williams <dan.j.williams@...el.com>
Subject: Re: Can ext4_break_layouts() ever fail?

On Fri 17-05-19 13:17:47, Ira Weiny wrote:
> On Fri, May 17, 2019 at 11:02:52AM +0200, Jan Kara wrote:
> > On Thu 16-05-19 13:56:15, Ira Weiny wrote:
> > 
> > > It looks to me like it is possible for ext4_break_layouts() to fail if
> > > prepare_to_wait_event() sees a pending signal.  Therefore I think this is a bug
> > > in ext4 regardless of how I may implement a truncate failure.
> > 
> > Yes, it's a bug in ext4.
> > 
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -5648,6 +5648,8 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> > >                 if (rc) {  
> > >                         up_write(&EXT4_I(inode)->i_mmap_sem);
> > >                         error = rc;
> > > +                       if (orphan)
> > > +                               ext4_orphan_del(NULL, inode);
> > 
> > This isn't quite correct. This would silence the warning but leave the
> > inode in on-disk orphan list. That is OK in case of fs-meltdown types of
> > failures like IO errors for metadata, aborted journal, or stuff like that.
> > But failing ext4_break_layouts() needs to be handled gracefully maintaining
> > fs consistency. So you rather need something like:
> > 
> > 			if (orphan && inode->i_nlink > 0) {
> > 				handle_t *handle;
> > 
> > 				handle = ext4_journal_start(inode,
> > 						EXT4_HT_INODE, 3);
> > 				if (IS_ERR(handle)) {
> > 					ext4_orphan_del(NULL, inode);
> > 					goto err_out;
> > 				}
> > 				ext4_orphan_del(handle, inode);
> > 				ext4_journal_stop(handle);
> > 			}
> >
> 
> Thanks!  Unfortunately, even with your suggestion something is still
> wrong with my code.
> 
> For some reason this does not seem to be "canceling" the truncate
> completely.  With my test code for FS DAX which fails ext4_break_layout()
> the file is being truncated and an application which is writing past that
> truncation is getting a SIGBUS.

Looking at the code again, I'm not really surprised. The path bailing out
of truncate in case ext4_break_layouts() fails is really hosed. The problem
is that when we get to ext4_break_layouts(), we have already updated i_size
and i_disksize and we happily leave them at their new values when bailing
out. So we need to somewhat reorder the stuff we do in ext4_setattr(). I'll
send a patch for that since it needs some considerations for proper lock
ordering etc... Thanks for experimenting with this :)

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR