Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341265 > unrolled thread

Re: fs: NULL deref in atime_needs_update

Started byIan Kent <raven@themaw.net>
First post2016-02-24 04:20 +0100
Last post2016-02-26 22:30 +0100
Articles 8 on this page of 28 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: fs: NULL deref in atime_needs_update Ian Kent <raven@themaw.net> - 2016-02-24 04:20 +0100
    Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-24 05:50 +0100
      Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 11:10 +0100
        Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 11:20 +0100
          Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-24 14:40 +0100
            Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-24 16:20 +0100
              Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-25 09:30 +0100
                Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-25 17:40 +0100
                  Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-26 22:30 +0100
                    Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-26 23:10 +0100
                      Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-27 23:30 +0100
                        Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-28 17:10 +0100
                          Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-28 18:10 +0100
                            Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-28 21:10 +0100
                              Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 10:40 +0100
                                Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 13:40 +0100
                                  Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 17:20 +0100
                              Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 14:10 +0100
                                Re: fs: NULL deref in atime_needs_update David Howells <dhowells@redhat.com> - 2016-02-29 14:50 +0100
                                Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 17:00 +0100
                                  Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 17:30 +0100
                                    Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-29 19:30 +0100
                                      Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 10:00 +0100
                                Re: fs: NULL deref in atime_needs_update Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-29 17:50 +0100
                                  Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 18:00 +0100
                                    Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 18:30 +0100
                                    Re: fs: NULL deref in atime_needs_update Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-29 18:30 +0100
                  Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-26 22:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1345958

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2016-02-29 17:30 +0100
Message-ID<r7yRQ-5oO-9@gated-at.bofh.it>
In reply to#1345934
On Mon, Feb 29, 2016 at 04:54:54PM +0100, Dmitry Vyukov wrote:

> Regardless of whether reordering is wrong or not, do we see how it can
> fix the WARNINGs/oopses? Because it does seem to. I've tried to revert
> just this part:
> 
> -               *inode = d_backing_inode(dentry);
>                 negative = d_is_negative(dentry);
> +               *inode = d_backing_inode(dentry);
> 
> And got:
> 
> [  976.609688] WARNING: CPU: 0 PID: 12126 at fs/namei.c:1587
> lookup_fast+0x3fa/0x450()
> [  976.626768] WARNING: CPU: 0 PID: 12126 at fs/namei.c:3123
> path_openat+0x12bc/0x1520()
> 
> in 15 minutes.

dentry going from negative to positive			lookup_fast()
							fetch NULL ->d_inode
store non-NULL ->d_inode
store new ->d_flags
							fetch new ->d_flags
							check ->d_seq
bump ->d_seq by 2

Change the order of fetches and you'll get rid of that scenario.

> In particular, applying this on top the previous patch will be
> inconclusive, because I already don't see the warnings.

Apply it with that reordering reversed, please.

[toc] | [prev] | [next] | [standalone]

#1346078

FromDmitry Vyukov <dvyukov@google.com>
Date2016-02-29 19:30 +0100
Message-ID<r7AJZ-6Ab-47@gated-at.bofh.it>
In reply to#1345958
On Mon, Feb 29, 2016 at 5:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Feb 29, 2016 at 04:54:54PM +0100, Dmitry Vyukov wrote:
>
>> Regardless of whether reordering is wrong or not, do we see how it can
>> fix the WARNINGs/oopses? Because it does seem to. I've tried to revert
>> just this part:
>>
>> -               *inode = d_backing_inode(dentry);
>>                 negative = d_is_negative(dentry);
>> +               *inode = d_backing_inode(dentry);
>>
>> And got:
>>
>> [  976.609688] WARNING: CPU: 0 PID: 12126 at fs/namei.c:1587
>> lookup_fast+0x3fa/0x450()
>> [  976.626768] WARNING: CPU: 0 PID: 12126 at fs/namei.c:3123
>> path_openat+0x12bc/0x1520()
>>
>> in 15 minutes.
>
> dentry going from negative to positive                  lookup_fast()
>                                                         fetch NULL ->d_inode
> store non-NULL ->d_inode
> store new ->d_flags
>                                                         fetch new ->d_flags
>                                                         check ->d_seq
> bump ->d_seq by 2
>
> Change the order of fetches and you'll get rid of that scenario.
>
>> In particular, applying this on top the previous patch will be
>> inconclusive, because I already don't see the warnings.
>
> Apply it with that reordering reversed, please.


OK, just wanted to make sure that we keep track of the situation.
Restarted testing with combined patch. Here is it just in case:
https://gist.githubusercontent.com/dvyukov/67fe363d5ce2e2b06c71/raw/4d1b6c23f8dff7e0f8e2e3cab7e50208fddb0570/gistfile1.txt

[toc] | [prev] | [next] | [standalone]

#1346459

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-01 10:00 +0100
Message-ID<r7OjU-6MP-7@gated-at.bofh.it>
In reply to#1346078
On Mon, Feb 29, 2016 at 7:19 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Feb 29, 2016 at 5:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Mon, Feb 29, 2016 at 04:54:54PM +0100, Dmitry Vyukov wrote:
>>
>>> Regardless of whether reordering is wrong or not, do we see how it can
>>> fix the WARNINGs/oopses? Because it does seem to. I've tried to revert
>>> just this part:
>>>
>>> -               *inode = d_backing_inode(dentry);
>>>                 negative = d_is_negative(dentry);
>>> +               *inode = d_backing_inode(dentry);
>>>
>>> And got:
>>>
>>> [  976.609688] WARNING: CPU: 0 PID: 12126 at fs/namei.c:1587
>>> lookup_fast+0x3fa/0x450()
>>> [  976.626768] WARNING: CPU: 0 PID: 12126 at fs/namei.c:3123
>>> path_openat+0x12bc/0x1520()
>>>
>>> in 15 minutes.
>>
>> dentry going from negative to positive                  lookup_fast()
>>                                                         fetch NULL ->d_inode
>> store non-NULL ->d_inode
>> store new ->d_flags
>>                                                         fetch new ->d_flags
>>                                                         check ->d_seq
>> bump ->d_seq by 2
>>
>> Change the order of fetches and you'll get rid of that scenario.
>>
>>> In particular, applying this on top the previous patch will be
>>> inconclusive, because I already don't see the warnings.
>>
>> Apply it with that reordering reversed, please.
>
>
> OK, just wanted to make sure that we keep track of the situation.
> Restarted testing with combined patch. Here is it just in case:
> https://gist.githubusercontent.com/dvyukov/67fe363d5ce2e2b06c71/raw/4d1b6c23f8dff7e0f8e2e3cab7e50208fddb0570/gistfile1.txt

No warnings/crashes in 15 hours on 3 VMs!

[toc] | [prev] | [next] | [standalone]

#1345988

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2016-02-29 17:50 +0100
Message-ID<r7zbc-5xn-13@gated-at.bofh.it>
In reply to#1345780
On Mon, Feb 29, 2016 at 5:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> The more I look at the situation with d_is_...() wrt barriers and ->d_seq,
> the less I understand it; outside of RCU mode we don't really need the
> barriers for that stuff and in RCU mode ->d_flags handling had been
> a serious headache all along...

Yeah, one of my least favorite "recent" vfs improvements.

> I'm tempted to do as below .. [ changing it to be unde the seqlock ]
>
> David, Linus, do you see any problems with that?  To me it looks saner
> that way and as cheap as the current code, but I might be missing something
> here...

I'd absolutely love to see this. The memory ordering for the flags
updates and reading was always really confusing, and I hated how it
was hidden inside the random access functions. And apparently it
wasn't just confusing, it was buggy too.

But I'd love it _more_ if this also means that we can get rid of the
rmb's, which your patch didn't. Can we? Or does the ordering still
remain for some other issue?

            Linus

[toc] | [prev] | [next] | [standalone]

#1345993

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2016-02-29 18:00 +0100
Message-ID<r7zkS-5AE-17@gated-at.bofh.it>
In reply to#1345988
On Mon, Feb 29, 2016 at 08:45:37AM -0800, Linus Torvalds wrote:

> > David, Linus, do you see any problems with that?  To me it looks saner
> > that way and as cheap as the current code, but I might be missing something
> > here...
> 
> I'd absolutely love to see this. The memory ordering for the flags
> updates and reading was always really confusing, and I hated how it
> was hidden inside the random access functions. And apparently it
> wasn't just confusing, it was buggy too.
> 
> But I'd love it _more_ if this also means that we can get rid of the
> rmb's, which your patch didn't. Can we? Or does the ordering still
> remain for some other issue?

In __d_entry_type(), you mean?  Should be, along with READ_ONCE() there.
AFAICS, ordering shouldn't be an issue anymore...

[toc] | [prev] | [next] | [standalone]

#1346013

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2016-02-29 18:30 +0100
Message-ID<r7zNT-60o-3@gated-at.bofh.it>
In reply to#1345993
On Mon, Feb 29, 2016 at 04:50:31PM +0000, Al Viro wrote:

> In __d_entry_type(), you mean?  Should be, along with READ_ONCE() there.
> AFAICS, ordering shouldn't be an issue anymore...

	FWIW, I've pushed the candidate fix (including the above) into
vfs.git#for-linus; I'd prefer to wait for confirmation that dcache.c
part is sufficient to fix the problem (_without_ reordering in lookup_fast())
before sending a pull request, though.

[toc] | [prev] | [next] | [standalone]

#1346017

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2016-02-29 18:30 +0100
Message-ID<r7zNU-60o-19@gated-at.bofh.it>
In reply to#1345993
On Mon, Feb 29, 2016 at 8:50 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> In __d_entry_type(), you mean?  Should be, along with READ_ONCE() there.
> AFAICS, ordering shouldn't be an issue anymore...

That's the one.

It results in those barriers in very subtle places, and it was always
unclear whether any of the users of those "d_is_*()" helper functions
really understood the subtle memory ordering involved.

The smp_rmb() is also potentially quite expensive on some architectures.

So getting rid of those hidden memory orderings would be a goodness
quite apart from fixing the bug Dmitry found.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1344691

FromAl Viro <viro@ZenIV.linux.org.uk>
Date2016-02-26 22:30 +0100
Message-ID<r6y7w-NB-17@gated-at.bofh.it>
In reply to#1343301
On Thu, Feb 25, 2016 at 04:39:27PM +0000, Al Viro wrote:
> Hrm...  OK, seeing that you still seem to trigger those within an hour or
> two (and *any* of remaining WARN_ON() are serious bugs - none of the
> "mitigation had been triggered" remained, sorry for not making it clear),
> let's try this.  Again, any WARN_ON triggered means that we'd caught something,
> whether it progresses into oops or not.

Any news on that one?  I'm going to carve fixes for understood bugs out of
that one and put those into tonight push, but it would be nice to sort out
all remaining crap lurking in that area...

Another question: what about the very first trace you'd posted, with apparent
GPF at 00000050?  Have you seen anything like that afterwards?

> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index c6d7d3d..86f81e3 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -323,6 +323,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
>  		struct dentry *new = d_lookup(parent, &dentry->d_name);
>  		if (!new)
>  			return NULL;
> +		WARN_ON(d_is_negative(new));
>  		ino = autofs4_dentry_ino(new);
>  		ino->last_used = jiffies;
>  		dput(path->dentry);
> diff --git a/fs/namei.c b/fs/namei.c
> index f624d13..daa6b25 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1209,6 +1209,7 @@ static int follow_managed(struct path *path, struct nameidata *nd)
>  		/* Handle an automount point */
>  		if (managed & DCACHE_NEED_AUTOMOUNT) {
>  			ret = follow_automount(path, nd, &need_mntput);
> +			WARN_ON(d_is_negative(path->dentry));
>  			if (ret < 0)
>  				break;
>  			continue;
> @@ -1260,6 +1261,7 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
>  {
>  	for (;;) {
>  		struct mount *mounted;
> +		void *p;
>  		/*
>  		 * Don't forget we might have a non-mountpoint managed dentry
>  		 * that wants to block transit.
> @@ -1289,7 +1291,9 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
>  		 * dentry sequence number here after this d_inode read,
>  		 * because a mount-point is always pinned.
>  		 */
> -		*inode = path->dentry->d_inode;
> +		p = *inode = path->dentry->d_inode;
> +		if (unlikely(!p))
> +			WARN_ON(!read_seqretry(&mount_lock, nd->m_seq));
>  	}
>  	return !read_seqretry(&mount_lock, nd->m_seq) &&
>  		!(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
> @@ -1580,10 +1584,12 @@ static int lookup_fast(struct nameidata *nd,
>  		 */
>  		if (negative)
>  			return -ENOENT;
> +		WARN_ON(!*inode);		// ->d_seq was fucked somehow
>  		path->mnt = mnt;
>  		path->dentry = dentry;
> -		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
> +		if (likely(__follow_mount_rcu(nd, path, inode, seqp))) {
>  			return 0;
> +		}
>  unlazy:
>  		if (unlazy_walk(nd, dentry, seq))
>  			return -ECHILD;
> @@ -1613,8 +1619,10 @@ unlazy:
>  	path->mnt = mnt;
>  	path->dentry = dentry;
>  	err = follow_managed(path, nd);
> -	if (likely(!err))
> +	if (likely(!err)) {
>  		*inode = d_backing_inode(path->dentry);
> +		WARN_ON(!*inode);
> +	}
>  	return err;
>  
>  need_lookup:
> @@ -1712,6 +1720,12 @@ static inline int should_follow_link(struct nameidata *nd, struct path *link,
>  		return 0;
>  	if (!follow)
>  		return 0;
> +	/* make sure that d_is_symlink above matches inode */
> +	if (nd->flags & LOOKUP_RCU) {
> +		if (read_seqcount_retry(&link->dentry->d_seq, seq))
> +			return -ECHILD;
> +	}
> +	WARN_ON(!inode);		// now, _that_ should not happen.
>  	return pick_link(nd, link, inode, seq);
>  }
>  
> @@ -1743,11 +1757,11 @@ static int walk_component(struct nameidata *nd, int flags)
>  		if (err < 0)
>  			return err;
>  
> -		inode = d_backing_inode(path.dentry);
>  		seq = 0;	/* we are already out of RCU mode */
>  		err = -ENOENT;
>  		if (d_is_negative(path.dentry))
>  			goto out_path_put;
> +		inode = d_backing_inode(path.dentry);
>  	}
>  
>  	if (flags & WALK_PUT)
> @@ -3106,8 +3120,10 @@ static int do_last(struct nameidata *nd,
>  			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
>  		/* we _can_ be in RCU mode here */
>  		error = lookup_fast(nd, &path, &inode, &seq);
> -		if (likely(!error))
> +		if (likely(!error)) {
> +			WARN_ON(!inode);
>  			goto finish_lookup;
> +		}
>  
>  		if (error < 0)
>  			return error;
> @@ -3192,12 +3208,13 @@ retry_lookup:
>  		return error;
>  
>  	BUG_ON(nd->flags & LOOKUP_RCU);
> -	inode = d_backing_inode(path.dentry);
>  	seq = 0;	/* out of RCU mode, so the value doesn't matter */
>  	if (unlikely(d_is_negative(path.dentry))) {
>  		path_to_nameidata(&path, nd);
>  		return -ENOENT;
>  	}
> +	inode = d_backing_inode(path.dentry);
> +	WARN_ON(!inode);
>  finish_lookup:
>  	if (nd->depth)
>  		put_link(nd);
> @@ -3206,11 +3223,6 @@ finish_lookup:
>  	if (unlikely(error))
>  		return error;
>  
> -	if (unlikely(d_is_symlink(path.dentry)) && !(open_flag & O_PATH)) {
> -		path_to_nameidata(&path, nd);
> -		return -ELOOP;
> -	}
> -
>  	if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) {
>  		path_to_nameidata(&path, nd);
>  	} else {
> @@ -3229,6 +3241,10 @@ finish_open:
>  		return error;
>  	}
>  	audit_inode(nd->name, nd->path.dentry, 0);
> +	if (unlikely(d_is_symlink(nd->path.dentry)) && !(open_flag & O_PATH)) {
> +		error = -ELOOP;
> +		goto out;
> +	}
>  	error = -EISDIR;
>  	if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
>  		goto out;
> @@ -3273,6 +3289,10 @@ opened:
>  			goto exit_fput;
>  	}
>  out:
> +	if (unlikely(error > 0)) {
> +		WARN_ON(1);
> +		error = -EINVAL;
> +	}
>  	if (got_write)
>  		mnt_drop_write(nd->path.mnt);
>  	path_put(&save_parent);
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4fb1691..4128a5c 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1060,6 +1060,8 @@ static void cleanup_mnt(struct mount *mnt)
>  	 * so mnt_get_writers() below is safe.
>  	 */
>  	WARN_ON(mnt_get_writers(mnt));
> +	WARN_ON(!mnt->mnt.mnt_root->d_inode);	// some joker has managed to
> +						// make mnt_root negative on us
>  	if (unlikely(mnt->mnt_pins.first))
>  		mnt_pin_kill(mnt);
>  	fsnotify_vfsmount_delete(&mnt->mnt);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web