From: haosdent <haosdent@gmail.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
zhengyu.duan@shopee.com, Haosong Huang <huangh@sea.com>
Subject: Re: NULL pointer dereference when access /proc/net
Date: Tue, 27 Apr 2021 01:16:44 +0800 [thread overview]
Message-ID: <CAFt=ROOi+bi_N4NEkDQxagNwnoqM0zYR+sxiag7r2poNVW9u+w@mail.gmail.com> (raw)
In-Reply-To: <YIWlOlss7usVnvme@zeniv-ca.linux.org.uk>
> really should not assume ->d_inode stable
Hi, Alexander, sorry to disturb you again. Today I try to check what
`dentry->d_inode` and `nd->link_inode` looks like when `dentry` is
already been killed in `__dentry_kill`.
```
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
```
It looks like `dentry->d_inode` could be NULL while `nd->link_inode`
is always has value.
But this make me confuse, by right `nd->link_inode` is get from
`dentry->d_inode`, right?
For example, in `walk_component`, suppose we go into `lookup_slow`,
```
static int walk_component(struct nameidata *nd, int flags)
if (unlikely(err <= 0)) {
...
path.dentry = lookup_slow(&nd->last, nd->path.dentry, nd->flags);
...
inode = d_backing_inode(path.dentry); <=== get `inode` from
`dentry->d_inode`.
}
return step_into(nd, &path, flags, inode, seq); <=== set `inode` to
`nd->link_inode`.
}
```
then in `step_into` -> `pick_link`
```
static int pick_link(struct nameidata *nd, struct path *link,
struct inode *inode, unsigned seq)
{
...
nd->link_inode = inode; <=== set `inode` to `nd->link_inode`.
}
```
So for the mismatch of `nd->link_inode` and `dentry->d_inode` in
following output. Do it means in Thread 1, `walk_component`
get a `dentry` from `d_lookup`, at the same time, in Thread 2,
`__dentry_kill` is run and set `dentry->d_inode` to NULL.
```
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
nd->last.name: net/sockstat, dentry->d_lockref.count: -128,
dentry->d_inode: (nil), nd->link_inode: 0xffffffffab299966
```
If these concurrent operations in `dentry->d_inode` could happen, how
we ensure `nd->link_inode = inode` and `d_backing_inode` are always
run before
`__dentry_kill`? I still could not find the questions for this from
dcache's code, sorry for the stupid question.
On Mon, Apr 26, 2021 at 1:22 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Apr 26, 2021 at 01:04:46AM +0800, haosdent wrote:
> > Hi, Alexander, thanks a lot for your quick reply.
> >
> > > Not really - the crucial part is ->d_count == -128, i.e. it's already past
> > > __dentry_kill().
> >
> > Thanks a lot for your information, we would check this.
> >
> > > Which tree is that?
> > > If you have some patches applied on top of that...
> >
> > We use Ubuntu Linux Kernel "4.15.0-42.45~16.04.1" from launchpad directly
> > without any modification, the mapping Linux Kernel should be
> > "4.15.18" according
> > to https://people.canonical.com/~kernel/info/kernel-version-map.html
>
> Umm... OK, I don't have it Ubuntu source at hand, but the thing to look into
> would be
> * nd->flags contains LOOKUP_RCU
> * in the mainline from that period (i.e. back when __atime_needs_update()
> used to exist) we had atime_needs_update_rcu() called in get_link() under those
> conditions, with
> static inline bool atime_needs_update_rcu(const struct path *path,
> struct inode *inode)
> {
> return __atime_needs_update(path, inode, true);
> }
> and __atime_needs_update() passing its last argument (rcu:true in this case) to
> relatime_need_update() in
> if (!relatime_need_update(path, inode, now, rcu))
> relatime_need_update() hitting
> update_ovl_inode_times(path->dentry, inode, rcu);
> and update_ovl_inode_times() starting with
> if (rcu || likely(!(dentry->d_flags & DCACHE_OP_REAL)))
> return;
> with subsequent accesses to ->d_inode. Those obviously are *NOT* supposed
> to be reached in rcu mode, due to that check.
>
> Your oops looks like something similar to that call chain had been involved and
> somehow had managed to get through to those ->d_inode uses.
>
> Again, in RCU mode we really, really should not assume ->d_inode stable. That's
> why atime_needs_update() gets inode as a separate argument and does *NOT* look
> at path->dentry at all. In the kernels of 4.8..4.18 period there it used to do
> so, but only in non-RCU mode (which is the reason for explicit rcu argument passed
> through that callchain).
--
Best Regards,
Haosdent Huang
next prev parent reply other threads:[~2021-04-26 17:16 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAFt=RON+KYYf5yt9vM3TdOSn4zco+3XtFyi3VDRr1vbQUBPZ0g@mail.gmail.com>
2021-04-25 16:50 ` NULL pointer dereference when access /proc/net Al Viro
2021-04-25 17:04 ` haosdent
2021-04-25 17:14 ` haosdent
2021-04-25 17:22 ` Al Viro
2021-04-25 18:00 ` haosdent
2021-04-25 18:15 ` haosdent
2021-04-26 17:16 ` haosdent [this message]
2021-04-26 17:30 ` Al Viro
2021-05-03 15:31 ` haosdent
2021-05-06 10:21 ` haosdent
2021-04-25 15:47 haosdent
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='CAFt=ROOi+bi_N4NEkDQxagNwnoqM0zYR+sxiag7r2poNVW9u+w@mail.gmail.com' \
--to=haosdent@gmail.com \
--cc=huangh@sea.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=zhengyu.duan@shopee.com \
/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).