From: Jan Kara Subject: Re: [PATCH] fs/ext{3,4}: fix potential race when setversion ioctl updates inode Date: Thu, 5 Jan 2012 00:32:54 +0100 Message-ID: <20120104233254.GH28907@quack.suse.cz> References: <20120103013152.GA26455@dztty> <20120104174609.GD28907@quack.suse.cz> <6C16105A-D0EE-413E-B993-F223CFC75307@dilger.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Djalal Harouni , Jan Kara , Andrew Morton , "Darrick J. Wong" , Theodore Ts'o , Yongqiang Yang , ext4 development , linux-kernel Mailing List , Al Viro To: Andreas Dilger Return-path: Received: from cantor2.suse.de ([195.135.220.15]:43285 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752281Ab2ADXc4 (ORCPT ); Wed, 4 Jan 2012 18:32:56 -0500 Content-Disposition: inline In-Reply-To: <6C16105A-D0EE-413E-B993-F223CFC75307@dilger.ca> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Wed 04-01-12 16:15:04, Andreas Dilger wrote: > On 2012-01-04, at 10:46 AM, Jan Kara wrote: > > On Tue 03-01-12 02:31:52, Djalal Harouni wrote: > >> > >> The EXT{3,4}_IOC_SETVERSION ioctl() updates the inode without i_mutex, > >> this can lead to a race with the other operations that update the same > >> inode. > >> > >> Patch tested. > > > > OK, so I've taken the patch into my tree, just updated the changelog > > which result of our discussion in this thread. I also took the ext4 part > > since there is no risk of conflict and the patch looks obvious. > > Actually, I'd like to hear more about whether this is a real problem, or > if it is just a theoretical problem found during code inspection or from > some static code analysis tool? As far as I understood that was just a theoretical issue and I applied the patch just on the grounds that it is more consistent to use i_mutex for i_generation changes. > With the metadata checksum feature we were discussing using the inode > generation as part of the seed for the directory leaf block checksum, so > that it wasn't possible to incorrectly access stale directory blocks from > a previous incarnation of the same inode number. > > We were discussing just disabling this ioctl on filesystems with metadata > checksums, and printing a deprecation warning for filesystems without that > feature enabled. I'm not aware of any real-world use for this ioctl, since > NFS cannot use it to reconstruct handles because there's no API to allocate > an inode with a specific number, so setting the generation is pointless. OK, I didn't know this. I'm fine with deprecating the ioctl if it's useless but since that's going to take a while I think the cleanup still makes some sense. Honza -- Jan Kara SUSE Labs, CR