From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Tao Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries Date: Sat, 18 Feb 2012 00:34:14 +0800 Message-ID: References: <20101227234641.GA22248@fieldses.org> <20110118204509.GA10903@fieldses.org> <20110118220817.GF10903@fieldses.org> <20110308181320.GA15566@fieldses.org> <20110310105821.GE22723@ZenIV.linux.org.uk> <20110311150749.2fa2be66@notabene.brown> <20120214170300.GA4309@fieldses.org> <20120215165633.GE12490@fieldses.org> <20120216140603.08cb4900@notabene.brown> <20120216223011.GA23997@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: NeilBrown , Al Viro , Nick Piggin , Nick Piggin , linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "J. Bruce Fields" Return-path: In-Reply-To: <20120216223011.GA23997-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On Fri, Feb 17, 2012 at 6:30 AM, J. Bruce Fields = wrote: > On Thu, Feb 16, 2012 at 02:06:03PM +1100, NeilBrown wrote: >> I was going ask how you managed to get an 'unhashed' dentry which wa= s not >> DISCONNECTED, and belonged to a directory that could be the subject = of >> d_splice_alias (that implies it has a name). >> >> The bug sounds like a race between lookup and rmdir, which should be >> prevented by i_mutex. >> >> I think that using __d_find_any_alias would just be papering over th= e >> problem, and would trigger a BUG_ON when it returned a non-DISCONNEC= TED alias. > > Looking through the latest upstream code, I can't come up with anothe= r > obvious reproducer. > > But I also can't see the code making any particular effort to ensure > that dentries are removed from inode's alias lists at the same time > they're unhashed. > > E.g., trace up through the callers of d_drop/__d_drop and try to > convince yourselves that they all end up removing the dentry from the > alias list. > > Can you see any reason why the following would actually create a > problem? > > --b. > > commit fcfef6b7319c5d19ea5064317528ff994343b011 > Author: J. Bruce Fields > Date: =C2=A0 Mon Feb 13 13:38:33 2012 -0500 > > =C2=A0 =C2=A0exports: stop d_splice_alias creating directory aliases > > =C2=A0 =C2=A0A directory should never have more than one dentry point= ing to it. > > =C2=A0 =C2=A0But d_splice_alias() does so in the case of a directory = with an > =C2=A0 =C2=A0already-existing non-DISCONNECTED dentry. > > =C2=A0 =C2=A0Prior to the removal of dentry_unhash() from vfs_rmdir()= , around v3.0, > =C2=A0 =C2=A0this could cause an nfsd deadlock like this: > > =C2=A0 =C2=A0 =C2=A0 =C2=A0- Somebody attempts to remove a non-empty = directory. > =C2=A0 =C2=A0 =C2=A0 =C2=A0- The dentry_unhash() in vfs_rmdir() unhas= hes the dentry > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pointing to the non-empty directory= =2E > =C2=A0 =C2=A0 =C2=A0 =C2=A0- ->rmdir() then fails with -ENOTEMPTY > =C2=A0 =C2=A0 =C2=A0 =C2=A0- Before the vfs_rmdir() caller reaches dp= ut(), an nfsd process > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0in rename looks up the directory by= filehandle; at the end of > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0that lookup, this dentry is found b= y d_alloc_anon(), and a > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0reference is taken on it, preventin= g dput() from removing it. > =C2=A0 =C2=A0 =C2=A0 =C2=A0- A regular lookup of the directory calls = d_splice_alias(), > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0finds only an unhashed (not a DISCO= NNECTED) dentry, and > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0insteads adds a new one, so the dir= ectory now has two > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dentries. > =C2=A0 =C2=A0 =C2=A0 =C2=A0- The nfsd process in rename, which was pr= eviously looking up > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0the source directory of the rename,= now looks up the target > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0directory (which is the same), and = gets the dentry newly > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0created by the previous lookup. > =C2=A0 =C2=A0 =C2=A0 =C2=A0- The rename, seeing two different dentrie= s, assumes this is a > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cross-directory rename and attempts= to take the i_mutex on the > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0directory twice. > > =C2=A0 =C2=A0I don't see as obvious a reproducer now, but I also don'= t see the > =C2=A0 =C2=A0existing code taking care to remove dentries from the al= ias list > =C2=A0 =C2=A0whenever they're unhashed. > > =C2=A0 =C2=A0It therefore seems safest to allow d_splice_alias to use= any dentry it > =C2=A0 =C2=A0finds. > > =C2=A0 =C2=A0Signed-off-by: J. Bruce Fields > > diff --git a/fs/dcache.c b/fs/dcache.c > index f68e193..1fd2256 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -1602,9 +1602,8 @@ struct dentry *d_splice_alias(struct inode *ino= de, struct dentry *dentry) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (inode && S_ISDIR(inode->i_mode)) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&ino= de->i_lock); > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 new =3D __d_find_a= lias(inode, 1); by replacing this, the want_discon argument is no longer in use. care to remove it as well? --=20 Thanks, Tao > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 new =3D __d_find_a= ny_alias(inode); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (new) { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED)); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0spin_unlock(&inode->i_lock); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0security_d_instantiate(new, inode); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0d_move(new, dentry); > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" = in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht= ml -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html