From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: A missing i_mutex in rename? (Linux kernel 2.6.latest) Date: Thu, 20 Apr 2006 11:59:29 +0100 Message-ID: <20060420105929.GV27946@ftp.linux.org.uk> References: <20060419121826.GI24104@parisc-linux.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Matthew Wilcox , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:23783 "EHLO ZenIV.linux.org.uk") by vger.kernel.org with ESMTP id S1750850AbWDTK7c (ORCPT ); Thu, 20 Apr 2006 06:59:32 -0400 To: Anton Altaparmakov Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, Apr 19, 2006 at 01:51:21PM +0100, Anton Altaparmakov wrote: > > - I don't immediately see a race that taking the lock on the victim of > > sys_unlink() solves; however, for symmetry with sys_rmdir(), it seems > > desirable. > > I guess the symmetry thing is fair enough. Not only; it does, among other things, guarantee that fs can assume that ->link() won't race with unlink() (and that link count is protected by ->i_mutex, while we are at it). > > - sys_link() needs to lock the target to be sure it isn't removed and > > replaced with a directory in the meantime. > > Agreed. "Replaced" part is bogus - we'd done lookup, so we won't get anything new. > > If you need to lock the old inode inside ntfs for your own consistency > > purposes, that looks like it should be fine, but the VFS doesn't need to > > lock it for you. > > Great, thanks. That was my own conclusion also but it never hurts to be > sure. (-: > > ntfs_rename() at the moment looks roughly like this: > > if (target_inode) { > if (S_ISDIR(target_inode->i_mode) > ntfs_rmdir(target_dir_inode, target_dentry); > else > ntfs_unlink(target_dir_inode, target_dentry); > } > mutex_lock(&old_inode->i_mutex); > ntfs_link(old_dentry, target_dir_inode, target_dentry); > ntfs_unlink(old_dir_inode, old_dentry); > mutex_unlock(&old_inode->i_mutex); Have fun dealing with error handling in the above... Note that failing rename() should _NOT_ lead to target disappearing.