From: "J. Bruce Fields" <bfields@fieldses.org>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: nfs4_show_superblock considered harmful :-)
Date: Fri, 29 May 2020 18:06:08 -0400 [thread overview]
Message-ID: <20200529220608.GA22758@fieldses.org> (raw)
In-Reply-To: <871rn38suc.fsf@notabene.neil.brown.name>
On Fri, May 29, 2020 at 10:53:15AM +1000, NeilBrown wrote:
> I've received a report of a 5.3 kernel crashing in
> nfs4_show_superblock().
> I was part way through preparing a patch when I concluded that
> the problem wasn't as straight forward as I thought.
>
> In the crash, the 'struct file *' passed to nfs4_show_superblock()
> was NULL.
> This file was acquired from find_any_file(), and every other caller
> of find_any_file() checks that the returned value is not NULL (though
> one BUGs if it is NULL - another WARNs).
> But nfs4_show_open() and nfs4_show_lock() don't.
> Maybe they should. I didn't double check, but I suspect they don't
> hold enough locks to ensure that the files don't get removed.
I think the only lock held is cl_lock, acquired in states_start.
We're starting here with an nfs4_stid that was found in the cl_stateids
idr.
A struct nfs4_stid is freed by nfs4_put_stid(), which removes it from
that idr under cl_lock before freeing the nfs4_stid and anything it
points to.
I think that was the theory....
One possible problem is downgrades, like nfs4_stateid_downgrade.
I'll keep mulling it over, thanks.
--b.
>
>
> Then I noticed that nfs4_show_deleg() accesses fi_deleg_file without
> checking if it is NULL - Should it take fi_lock and make sure it is
> not NULL - and get a counted reference?
> And maybe nfs4_show_layout() has the same problem?
>
> I could probably have worked my way through fixing all of these, but
> then I discovered that these things are now 'struct nfsd_file *' rather
> than 'struct file *' and that the helpful documentation says:
>
> * Note that this object doesn't
> * hold a reference to the inode by itself, so the nf_inode pointer should
> * never be dereferenced, only used for comparison.
>
> and yet nfs4_show_superblock() contains:
>
> struct inode *inode = f->nf_inode;
>
> seq_printf(s, "superblock: \"%02x:%02x:%ld\"",
> MAJOR(inode->i_sb->s_dev),
> MINOR(inode->i_sb->s_dev),
> inode->i_ino);
>
> do you see my problem?
>
> Is this really safe and the doco wrong? (I note that the use of nf_inode
> in nfsd_file_mark_find_or_create() looks wrong, but is actually safe).
> Or should we check if nf_file is non-NULL and use that?
>
> In short:
> - We should check find_any_file() return value - correct?
> - Do we need extra locking to stabilize fi_deleg_file?
> - ditto for ->ls_file
> - how can nfs4_show_superblock safely get s_dev and i_ino from a
> nfsd_file?
>
> Thanks,
>
> NeilBrown
next prev parent reply other threads:[~2020-05-29 22:06 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-29 0:53 nfs4_show_superblock considered harmful :-) NeilBrown
2020-05-29 22:06 ` J. Bruce Fields [this message]
2020-06-01 2:01 ` NeilBrown
2020-07-15 18:54 ` J. Bruce Fields
2020-07-15 23:05 ` NeilBrown
2020-07-15 23:43 ` J. Bruce Fields
2020-07-16 17:19 ` J. Bruce Fields
2020-07-16 23:43 ` NeilBrown
2020-07-17 1:03 ` J. Bruce Fields
2020-07-17 1:31 ` NeilBrown
2020-07-17 2:18 ` J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200529220608.GA22758@fieldses.org \
--to=bfields@fieldses.org \
--cc=jlayton@kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).