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 X-Spam-Level: X-Spam-Status: No, score=-17.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 733B0C4743C for ; Mon, 21 Jun 2021 13:27:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 50DDB60249 for ; Mon, 21 Jun 2021 13:27:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229968AbhFUN36 (ORCPT ); Mon, 21 Jun 2021 09:29:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44350 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229904AbhFUN36 (ORCPT ); Mon, 21 Jun 2021 09:29:58 -0400 Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3319AC061574 for ; Mon, 21 Jun 2021 06:27:43 -0700 (PDT) Received: by mail-io1-xd2c.google.com with SMTP id f10so10104843iok.6 for ; Mon, 21 Jun 2021 06:27:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=D3FuCrkfr4dPZDh4h+lsWoDfN+dzTOGefxXQvxBgPQQ=; b=pgkiF05RXbIlwpcesARUSpWkj+cYuZUNQWDsyyWZaEwtCXDMfoZLSUKvjTvmF0JCFE z72kq96Cx/uYlbHuRlQsFWHI6R9wJDIlfdVBLScf8wq1pRDnNXCzvZHSli1jDx0KkD3b l5AEbRz7KShWDE80cBnb18eUeQrU4z+tyfXsCtez0iLSGk+Ayw86MAmHsfVo4o3oB5xL s9i4kFujnv9iokPrhSvu6Nw1MB/5fknTXLy0mSdWcqVrUziVPsn2A5pPSs0RmhVsGBMa 8BTY/aX9bAxFS0x90T6tE3nlr8dfqFquIWb9jK1E+/JqvaXqtcTBO0JvCa3pU9sBz8js 2VIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=D3FuCrkfr4dPZDh4h+lsWoDfN+dzTOGefxXQvxBgPQQ=; b=MzyOb7BovbrNTh3AQg8+D7YJzdGtyFhTEw8g5cYDJEg5EgZsHdTIBwz3yLJZWWtTq+ WEC65yyza7XEN5iHqGvAkjt1vLgeRLKwLbXdyb2txV7KTBgOQ8dhswKGBXxa1MJYE8o4 2HQw/60uDFyh1kGSQ1+qr6J+4nMzH2ypBMwYsWDVHndwEMD/eY7FFZmUm66aa9iXX87V XYlZY7109pSpvEnKrQKZnChx6iJyK7CjlqzGVYgGjClJ5mQa4wXmVtwqrKRw2Z41BTDL V74eFYIZkvNQ0PB0+VVqr9Psa+Zn39ndldkJ58uTC6bORtr5LI97sJwrfEgPasN0jgaM SWzw== X-Gm-Message-State: AOAM532yhP0WUlLUbfxaqpyqIQmwyl/A3yXRWpi7oqgUl5ErskPaIwnA N3Wa/TsbXfZ2qAdV9aiBgzTE72D0QaRaeZV1XV8nG5PVHeE= X-Google-Smtp-Source: ABdhPJyyB1bxtSoo97ErGWMfASggzNlGGGQq+0yZ3plx/nsy2d6hnV0pOct1uoCoIF7eHYw54DD6uyuxcIaVlzMVM1g= X-Received: by 2002:a5d:8b03:: with SMTP id k3mr19712730ion.203.1624282062660; Mon, 21 Jun 2021 06:27:42 -0700 (PDT) MIME-Version: 1.0 References: <20210609181158.479781-1-amir73il@gmail.com> In-Reply-To: From: Amir Goldstein Date: Mon, 21 Jun 2021 16:27:31 +0300 Message-ID: Subject: Re: [PATCH] fuse: fix illegal access to inode with reused nodeid To: Miklos Szeredi Cc: Max Reitz , Vivek Goyal , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Mon, Jun 21, 2021 at 12:27 PM Miklos Szeredi wrote: > > On Wed, 9 Jun 2021 at 20:12, Amir Goldstein wrote: > > > > Server responds to LOOKUP and other ops (READDIRPLUS/CREATE/MKNOD/...) > > with outarg containing nodeid and generation. > > > > If a fuse inode is found in inode cache with the same nodeid but > > different generation, the existing fuse inode should be unhashed and > > marked "bad" and a new inode with the new generation should be hashed > > instead. > > > > This can happen, for example, with passhrough fuse filesystem that > > returns the real filesystem ino/generation on lookup and where real inode > > numbers can get recycled due to real files being unlinked not via the fuse > > passthrough filesystem. > > > > With current code, this situation will not be detected and an old fuse > > dentry that used to point to an older generation real inode, can be used > > to access a completely new inode, which should be accessed only via the > > new dentry. > > > > Note that because the FORGET message carries the nodeid w/o generation, > > the server should wait to get FORGET counts for the nlookup counts of > > the old and reused inodes combined, before it can free the resources > > associated to that nodeid. > > > > Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxgDMGUpK35huwqFYGH_idBB8S6eLiz85o0DDKOyDH4Syg@mail.gmail.com/ > > Signed-off-by: Amir Goldstein > > --- > > > > Miklos, > > > > I was able to reproduce this issue with a passthrough fs that stored > > ino+generation and uses then to open fd on lookup. > > > > I extended libfuse's test_syscalls [1] program to demonstrate the issue > > described in commit message. > > > > Max, IIUC, you are making a modification to virtiofs-rs that would > > result is being exposed to this bug. You are welcome to try out the > > test and let me know if you can reproduce the issue. > > > > Note that some test_syscalls test fail with cache enabled, so libfuse's > > test_examples.py only runs test_syscalls in cache disabled config. > > > > Thanks, > > Amir. > > > > [1] https://github.com/amir73il/libfuse/commits/test-reused-inodes > > > > fs/fuse/dir.c | 3 ++- > > fs/fuse/fuse_i.h | 9 +++++++++ > > fs/fuse/inode.c | 4 ++-- > > fs/fuse/readdir.c | 7 +++++-- > > 4 files changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > > index 1b6c001a7dd1..b06628fd7d8e 100644 > > --- a/fs/fuse/dir.c > > +++ b/fs/fuse/dir.c > > @@ -239,7 +239,8 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags) > > if (!ret) { > > fi = get_fuse_inode(inode); > > if (outarg.nodeid != get_node_id(inode) || > > - (bool) IS_AUTOMOUNT(inode) != (bool) (outarg.attr.flags & FUSE_ATTR_SUBMOUNT)) { > > + fuse_stale_inode(inode, outarg.generation, > > + &outarg.attr)) { > > This changes behavior in the inode_wrong_type() case. I guess that > was not intended. > Right. fixed in v2. > > > fuse_queue_forget(fm->fc, forget, > > outarg.nodeid, 1); > > goto invalid; > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > > index 7e463e220053..f1bd28c176a9 100644 > > --- a/fs/fuse/fuse_i.h > > +++ b/fs/fuse/fuse_i.h > > @@ -867,6 +867,15 @@ static inline u64 fuse_get_attr_version(struct fuse_conn *fc) > > return atomic64_read(&fc->attr_version); > > } > > > > +static inline bool fuse_stale_inode(const struct inode *inode, int generation, > > + struct fuse_attr *attr) > > +{ > > + return inode->i_generation != generation || > > + inode_wrong_type(inode, attr->mode) || > > + (bool) IS_AUTOMOUNT(inode) != > > + (bool) (attr->flags & FUSE_ATTR_SUBMOUNT); > > +} > > + > > static inline void fuse_make_bad(struct inode *inode) > > { > > remove_inode_hash(inode); > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > > index 393e36b74dc4..257bb3e1cac8 100644 > > --- a/fs/fuse/inode.c > > +++ b/fs/fuse/inode.c > > @@ -350,8 +350,8 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid, > > inode->i_generation = generation; > > fuse_init_inode(inode, attr); > > unlock_new_inode(inode); > > - } else if (inode_wrong_type(inode, attr->mode)) { > > - /* Inode has changed type, any I/O on the old should fail */ > > + } else if (fuse_stale_inode(inode, generation, attr)) { > > This one adds the automount check. That should be okay, since > directories must never have aliases and such a beast would error out > anyway later on d_splice_alias. > > > + /* nodeid was reused, any I/O on the old inode should fail */ > > fuse_make_bad(inode); > > iput(inode); > > goto retry; > > diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c > > index 277f7041d55a..bc267832310c 100644 > > --- a/fs/fuse/readdir.c > > +++ b/fs/fuse/readdir.c > > @@ -200,9 +200,12 @@ static int fuse_direntplus_link(struct file *file, > > if (!d_in_lookup(dentry)) { > > struct fuse_inode *fi; > > inode = d_inode(dentry); > > + if (inode && get_node_id(inode) != o->nodeid) > > + inode = NULL; > > if (!inode || > > - get_node_id(inode) != o->nodeid || > > - inode_wrong_type(inode, o->attr.mode)) { > > + fuse_stale_inode(inode, o->generation, &o->attr)) { > > Again, automount check added, I'm not at all sure how automount and > readdirplus interact. This needs to be looked at (though it's > mostly a separate issue from this patch). > I removed the automount check completely from fuse_stale_inode() for v2. Thanks, Amir.