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]

Message-ID: <20190517201746.GA14175@iweiny-DESK2.sc.intel.com>
Date:   Fri, 17 May 2019 13:17:47 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     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, 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.

I don't understand why this is happening because failing here should be
skipping the call to truncate_pagecache() which AFAICT does user unmappings...
So...  I'm still investigating this.

But thanks for confirming this is a bug.  I will get a generic patch out soon.

Thanks,
Ira