linux-ext4 - Re: [PATCH 2/4] ext4: fix deadlock between inline_data and ext4_expand_extra_isize

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: <20170124122722.GE20153@quack2.suse.cz>
Date:   Tue, 24 Jan 2017 13:27:22 +0100
From:   Jan Kara <jack@...e.cz>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Jan Kara <jack@...e.cz>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux@...encehorizons.net, stable@...r.kernel.org, #@...nk.org
Subject: Re: [PATCH 2/4] ext4: fix deadlock between inline_data and
 ext4_expand_extra_isize_ea()

On Sun 22-01-17 17:25:27, Ted Tso wrote:
> > > @@ -1497,12 +1493,11 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
> > >  	int error = 0, tried_min_extra_isize = 0;
> > >  	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
> > >  	int isize_diff;	/* How much do we need to grow i_extra_isize */
> > > +	int no_expand;
> > > +
> > > +	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
> > > +		return 0;
> > 
> > Why do you play tricks with trylock here? ext4_mark_inode_dirty() checks
> > EXT4_STATE_NO_EXPAND and thus we should not ever get here if we already
> > hold xattr_sem...
> 
> The problem is still a lock inversion in the truncate code path.  The
> simplest way of dealing with it to simply avoiding doing the
> expand_isize operation on truncates.  In the case where this is
> happening on the deletion of an inode, doing the expansion is
> pointless anyway.

I see, thanks for explanation. Well seeing all these problems with
ext4_expand_extra_isize() wouldn't we be better off by not calling it from
ext4_mark_inode_dirty() but rather explicitely from several well-defined
places? Because this implicit calling looks like it causes us too much
trouble.

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