From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 28743C433EF for ; Tue, 18 Jan 2022 01:32:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238581AbiARBce (ORCPT ); Mon, 17 Jan 2022 20:32:34 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45776 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229705AbiARBcd (ORCPT ); Mon, 17 Jan 2022 20:32:33 -0500 Received: from zeniv-ca.linux.org.uk (zeniv-ca.linux.org.uk [IPv6:2607:5300:60:148a::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 72DE6C061574; Mon, 17 Jan 2022 17:32:33 -0800 (PST) Received: from viro by zeniv-ca.linux.org.uk with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9dMF-002gha-WE; Tue, 18 Jan 2022 01:32:24 +0000 Date: Tue, 18 Jan 2022 01:32:23 +0000 From: Al Viro To: Brian Foster Cc: Ian Kent , "Darrick J. Wong" , Christoph Hellwig , Miklos Szeredi , David Howells , Kernel Mailing List , linux-fsdevel , xfs , Linus Torvalds Subject: Re: [PATCH] vfs: check dentry is still valid in get_link() Message-ID: References: <164180589176.86426.501271559065590169.stgit@mickey.themaw.net> <275358741c4ee64b5e4e008d514876ed4ec1071c.camel@themaw.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: Al Viro Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Mon, Jan 17, 2022 at 07:48:49PM +0000, Al Viro wrote: > > But that critically depends upon the contents not getting mangled. If it > > *can* be screwed by such unlink, we risk successful lookup leading to the > > wrong place, with nothing to tell us that it's happening. We could handle > > that by adding a check to fs/namei.c:put_link(), and propagating the error > > to callers. It's not impossible, but it won't be pretty. > > > > And that assumes we avoid oopsen on string changing under us in the first > > place. Which might or might not be true - I hadn't finished the audit yet. > > Note that it's *NOT* just fs/namei.c + fs/dcache.c + some fs methods - > > we need to make sure that e.g. everything called by ->d_hash() instances > > is OK with strings changing right under them. Including utf8_to_utf32(), > > crc32_le(), utf8_casefold_hash(), etc. > > And AFAICS, ext4, xfs and possibly ubifs (I'm unfamiliar with that one and > the call chains there are deep enough for me to miss something) have the > "bugger the contents of string returned by RCU ->get_link() if unlink() > happens" problem. > > I would very much prefer to have them deal with that crap, especially > since I don't see why does ext4_evict_inode() need to do that memset() - > can't we simply check ->i_op in ext4_can_truncate() and be done with > that? This reuse-without-delay has another fun side, AFAICS. Suppose the new use for inode comes with the same ->i_op (i.e. it's a symlink again) and it happens right after ->get_link() has returned the pointer to body. We are already past whatever checks we might add in pick_link(). And the pointer is still valid. So we end up quietly traversing the body of completely unrelated symlink that never had been anywhere near any directory we might be looking at. With no indication of anything going wrong - just a successful resolution with bogus result. Could XFS folks explain what exactly goes wrong if we make actual marking inode as ready for reuse RCU-delayed, by shifting just that into ->free_inode()? Why would we need any extra synchronize_rcu() anywhere?