From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: Elevated i_writecount doesn't guarantee ->release to be called Date: Thu, 29 Jan 2015 17:57:16 +0100 Message-ID: <20150129165716.GD11635@quack.suse.cz> References: <54c95874.1pbbCloLAl3wWsPR%akpm@linux-foundation.org> <20150128224534.GB29656@ZenIV.linux.org.uk> <20150129124630.GB11635@quack.suse.cz> <1928119748.16961.1422550076048.open-xchange@webmail.nmp.proximus.be> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Jan Kara , akpm@linux-foundation.org, mm-commits@vger.kernel.org, linux-fsdevel@vger.kernel.org To: Fabian Frederick Return-path: Received: from cantor2.suse.de ([195.135.220.15]:38626 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752139AbbA2Q5T (ORCPT ); Thu, 29 Jan 2015 11:57:19 -0500 Content-Disposition: inline In-Reply-To: <1928119748.16961.1422550076048.open-xchange@webmail.nmp.proximus.be> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 29-01-15 17:47:56, Fabian Frederick wrote: > > On 29 January 2015 at 13:46 Jan Kara wrote: > > > > > >=A0 =A0Changed subject and added linux-fsdevel to CC so that other d= evelopers > > read this don't fall into the same trap :). > > > > On Wed 28-01-15 22:45:34, Al Viro wrote: > > > On Wed, Jan 28, 2015 at 01:45:24PM -0800, akpm@linux-foundation.o= rg wrote: > > > > atomic_t i_opencnt was used to free allocation in case there we= re no more > > > > opens.=A0 This patch replaces affs_file_open by generic_file_op= en and uses > > > > FMODE_WRITE/i_writecount=3D=3D1 for the task like other FS. > > > > > > > > > >=A0 affs_file_release(struct inode *inode, struct file *filp) > > > >=A0 { > > > > - pr_debug("release(%lu, %d)\n", > > > > -=A0 =A0 =A0 =A0 =A0 inode->i_ino, atomic_read(&AFFS_I(inode)->= i_opencnt)); > > > > + pr_debug("release(%lu)\n", inode->i_ino); > > > >=A0 > > > > - if (atomic_dec_and_test(&AFFS_I(inode)->i_opencnt)) { > > > > + if ((filp->f_mode & FMODE_WRITE) && > > > > +=A0 =A0 =A0(atomic_read(&inode->i_writecount) =3D=3D 1)) { > > > > > > I'm not at all convinced that this is correct for affs.=A0 Or for= anything > > > else, for that matter.=A0 Look: suppose somebody else is trying t= o open > > > that sucker with O_TRUNC at that moment and they'd already gotten= past > > > get_write_access() in handle_truncate(), only to fail on > > > locks_verify_locked(). > > > _That_ open() won't get anywhere near opening the file, so there = won't be > > > ->release() for it.=A0 And our ->release() will see ->i_writecoun= t greater > > > than 1, due to get_write_access() done in handle_truncate() and s= till not > > > balanced by coming put_write_access() in there - we'll call it af= ter the > > > locks_verify_locked() reports failure, but that hasn't happened y= et. > > > > > > Similar scenarios can almost certainly be constructed for other c= alls of > > > get_write_access() as well, but this one is enough to NAK this pa= tch, _and_ > > > to make the similar logics in other filesystems very suspicious..= =2E > >=A0 =A0Thanks for pointing this out. You made me at look where exact= ly is > > get_write_access() called and there are even places where we call i= t > > without having file descriptor at all (e.g.=A0 truncate path). So e= xt3, ext4, > > udf, and gfs2 are racy. If we race, results aren't that bad (we jus= t keep > > preallocated blocks in the inode) but still it would be nice to fix= =2E > > > > Obviously we could maintain a private writecount in ->open() method= but it > > would seem a bit sad to do that for this mostly theoretical issue. = Maybe we > > just verify whether preallocation is truncated when evicting inode = from > > memory and if not, do it there. It's not perfect but even with curr= ent racy > > solution noone noticed in practice. > Note that udf is slightly different ; it checks for i_writecount > 1 = not =3D1 > which means it would release the file in scenario described above ... I know but that's because it has two bugs in a single condition ;) I = have a patch queued for udf which changes the condition to =3D=3D 1. Honza --=20 Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html