Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Jari Ruusu]

This patch for 3.10 branch appears to be missing one important

+       dentry->d_flags |= DCACHE_RCUACCESS;

in fs/dcache.c __d_materialise_dentry() function. When Ben Hutchings
backported Al Viro's original fix to stable branches that he maintains,
he added that one additional line to both 3.2 and 3.16 branches. Please
consider including that additional one line fix for 3.10 stable branch
also.


Ben Hutchings said this on his 3.2.82-rc1 patch:
[bwh: Backported to 3.2:
 - Adjust context
 - Also set the flag in __d_materialise_dentry())]

http://marc.info/?l=linux-kernel&m=147117565612275&w=2


Ben Hutchings said this on his 3.16.37-rc1 patch:
[bwh: Backported to 3.16:
 - Adjust context
 - Also set the flag in __d_materialise_dentry())]

http://marc.info/?l=linux-kernel&m=147117433412006&w=2


Also mentioned by Sasha Levin on 3.18 and 4.1 commits:
Cc: stable@vger.kernel.org # v3.2+ (and watch out for __d_materialise_dentry())

http://marc.info/?l=linux-stable-commits&m=146648034410827&w=2
http://marc.info/?l=linux-stable-commits&m=146647471009771&w=2

-- 
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

On Mon, Aug 22, 2016 at 04:56:57PM +0300, Jari Ruusu wrote:
> This patch for 3.10 branch appears to be missing one important
> 
> +       dentry->d_flags |= DCACHE_RCUACCESS;
> 
> in fs/dcache.c __d_materialise_dentry() function. When Ben Hutchings
> backported Al Viro's original fix to stable branches that he maintains,
> he added that one additional line to both 3.2 and 3.16 branches. Please
> consider including that additional one line fix for 3.10 stable branch
> also.
(...)

Many thanks Jari, I'll use Ben's backport then.

Cheers,
Willy

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

Greg, Jiri,

I checked Jari's explanation below and found that v3.14.77 and v3.12.62
are missing the same fix as 3.10. In fact Al's original commit 3d56c25
("fix d_walk()/non-delayed __d_free() race") used to mention to check 
this __d_materialise_dentry() function in the Cc: stable line, but this
got lost during the backports.

Normally all of our 3 kernels need to apply the following patch that
Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
right now.

Cheers,
Willy

diff --git a/fs/dcache.c b/fs/dcache.c
index 2a808fb..2d0b9d2 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2401,6 +2401,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 	switch_names(dentry, anon);
 	swap(dentry->d_name.hash, anon->d_name.hash);
 
+	dentry->d_flags |= DCACHE_RCUACCESS;
 	dentry->d_parent = dentry;
 	list_del_init(&dentry->d_child);
 	anon->d_parent = dparent;


On Mon, Aug 22, 2016 at 04:56:57PM +0300, Jari Ruusu wrote:
> This patch for 3.10 branch appears to be missing one important
> 
> +       dentry->d_flags |= DCACHE_RCUACCESS;
> 
> in fs/dcache.c __d_materialise_dentry() function. When Ben Hutchings
> backported Al Viro's original fix to stable branches that he maintains,
> he added that one additional line to both 3.2 and 3.16 branches. Please
> consider including that additional one line fix for 3.10 stable branch
> also.
> 
> 
> Ben Hutchings said this on his 3.2.82-rc1 patch:
> [bwh: Backported to 3.2:
>  - Adjust context
>  - Also set the flag in __d_materialise_dentry())]
> 
> http://marc.info/?l=linux-kernel&m=147117565612275&w=2
> 
> 
> Ben Hutchings said this on his 3.16.37-rc1 patch:
> [bwh: Backported to 3.16:
>  - Adjust context
>  - Also set the flag in __d_materialise_dentry())]
> 
> http://marc.info/?l=linux-kernel&m=147117433412006&w=2
> 
> 
> Also mentioned by Sasha Levin on 3.18 and 4.1 commits:
> Cc: stable@vger.kernel.org # v3.2+ (and watch out for __d_materialise_dentry())
> 
> http://marc.info/?l=linux-stable-commits&m=146648034410827&w=2
> http://marc.info/?l=linux-stable-commits&m=146647471009771&w=2
> 
> -- 
> Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

On Sat, 2016-08-27 at 11:31 +0200, Willy Tarreau wrote:
> Greg, Jiri,
> 
> I checked Jari's explanation below and found that v3.14.77 and v3.12.62
> are missing the same fix as 3.10. In fact Al's original commit 3d56c25
> ("fix d_walk()/non-delayed __d_free() race") used to mention to check 
> this __d_materialise_dentry() function in the Cc: stable line, but this
> got lost during the backports.
> 
> Normally all of our 3 kernels need to apply the following patch that
> Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
> right now.

I never did get positive confirmation that this is the right change in
__d_materialise_dentry().  Al, could you please comment?

Ben.

> > Cheers,
> Willy
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 2a808fb..2d0b9d2 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2401,6 +2401,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> >  	switch_names(dentry, anon);
> >  	swap(dentry->d_name.hash, anon->d_name.hash);
>  
> > +	dentry->d_flags |= DCACHE_RCUACCESS;
> >  	dentry->d_parent = dentry;
> >  	list_del_init(&dentry->d_child);
> >  	anon->d_parent = dparent;
> 
> 
> On Mon, Aug 22, 2016 at 04:56:57PM +0300, Jari Ruusu wrote:
> > 
> > This patch for 3.10 branch appears to be missing one important
> > 
> > +       dentry->d_flags |= DCACHE_RCUACCESS;
> > 
> > in fs/dcache.c __d_materialise_dentry() function. When Ben Hutchings
> > backported Al Viro's original fix to stable branches that he maintains,
> > he added that one additional line to both 3.2 and 3.16 branches. Please
> > consider including that additional one line fix for 3.10 stable branch
> > also.
> > 
> > 
> > Ben Hutchings said this on his 3.2.82-rc1 patch:
> > [bwh: Backported to 3.2:
> >  - Adjust context
> >  - Also set the flag in __d_materialise_dentry())]
> > 
> > http://marc.info/?l=linux-kernel&m=147117565612275&w=2
> > 
> > 
> > Ben Hutchings said this on his 3.16.37-rc1 patch:
> > [bwh: Backported to 3.16:
> >  - Adjust context
> >  - Also set the flag in __d_materialise_dentry())]
> > 
> > http://marc.info/?l=linux-kernel&m=147117433412006&w=2
> > 
> > 
> > Also mentioned by Sasha Levin on 3.18 and 4.1 commits:
> > > > Cc: stable@vger.kernel.org # v3.2+ (and watch out for __d_materialise_dentry())
> > 
> > http://marc.info/?l=linux-stable-commits&m=146648034410827&w=2
> > http://marc.info/?l=linux-stable-commits&m=146647471009771&w=2
> > 
> > -- 
> > Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189
-- 
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

On Sat, Aug 27, 2016 at 12:38:38PM +0100, Ben Hutchings wrote:
> On Sat, 2016-08-27 at 11:31 +0200, Willy Tarreau wrote:
> > Greg, Jiri,
> > 
> > I checked Jari's explanation below and found that v3.14.77 and v3.12.62
> > are missing the same fix as 3.10. In fact Al's original commit 3d56c25
> > ("fix d_walk()/non-delayed __d_free() race") used to mention to check 
> > this __d_materialise_dentry() function in the Cc: stable line, but this
> > got lost during the backports.
> > 
> > Normally all of our 3 kernels need to apply the following patch that
> > Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
> > right now.
> 
> I never did get positive confirmation that this is the right change in
> __d_materialise_dentry().  Al, could you please comment?

Well in my experience Al checks our reviews and steps in when there's
a mistake. Also your patch seems to reproduce the fix for the code
that was later killed by commit 63cf427 ("kill __d_materialise_dentry()")
which factors it out into __d_move() so I'm inclined to think that what
you did makes sense.

Cheers,
Willy

Re: [PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

On Sat, Aug 27, 2016 at 12:38:38PM +0100, Ben Hutchings wrote:
> On Sat, 2016-08-27 at 11:31 +0200, Willy Tarreau wrote:
> > Greg, Jiri,
> > 
> > I checked Jari's explanation below and found that v3.14.77 and v3.12.62
> > are missing the same fix as 3.10. In fact Al's original commit 3d56c25
> > ("fix d_walk()/non-delayed __d_free() race") used to mention to check�
> > this __d_materialise_dentry() function in the Cc: stable line, but this
> > got lost during the backports.
> > 
> > Normally all of our 3 kernels need to apply the following patch that
> > Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
> > right now.
> 
> I never did get positive confirmation that this is the right change in
> __d_materialise_dentry(). �Al, could you please comment?

Well in my experience Al checks our reviews and steps in when there's
a mistake. Also your patch seems to reproduce the fix for the code
that was later killed by commit 63cf427 ("kill __d_materialise_dentry()")
which factors it out into __d_move() so I'm inclined to think that what
you did makes sense.

Cheers,
Willy

Patch "fix d_walk()/non-delayed __d_free() race" has been added to the 3.14-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    fix d_walk()/non-delayed __d_free() race

to the 3.14-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     fix-d_walk-non-delayed-__d_free-race.patch
and it can be found in the queue-3.14 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From w@1wt.eu  Fri Sep  9 16:26:43 2016
From: Willy Tarreau <w@1wt.eu>
Date: Sat, 27 Aug 2016 11:31:35 +0200
Subject: fix d_walk()/non-delayed __d_free() race
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jslaby@suse.cz>
Cc: Jari Ruusu <jariruusu@users.sourceforge.net>, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>, Ben Hutchings <ben@decadent.org.uk>
Message-ID: <20160827093135.GA28378@1wt.eu>
Content-Disposition: inline

From: Willy Tarreau <w@1wt.eu>

I checked Jari's explanation below and found that v3.14.77 and v3.12.62
are missing the same fix as 3.10. In fact Al's original commit 3d56c25
("fix d_walk()/non-delayed __d_free() race") used to mention to check 
this __d_materialise_dentry() function in the Cc: stable line, but this
got lost during the backports.

Normally all of our 3 kernels need to apply the following patch that
Ben correctly put in 3.16 and 3.2. I'm fixing the backport in 3.10.103
right now.

On Mon, Aug 22, 2016 at 04:56:57PM +0300, Jari Ruusu wrote:
> This patch for 3.10 branch appears to be missing one important
> 
> +       dentry->d_flags |= DCACHE_RCUACCESS;
> 
> in fs/dcache.c __d_materialise_dentry() function. When Ben Hutchings
> backported Al Viro's original fix to stable branches that he maintains,
> he added that one additional line to both 3.2 and 3.16 branches. Please
> consider including that additional one line fix for 3.10 stable branch
> also.
> 
> 
> Ben Hutchings said this on his 3.2.82-rc1 patch:
> [bwh: Backported to 3.2:
>  - Adjust context
>  - Also set the flag in __d_materialise_dentry())]
> 
> http://marc.info/?l=linux-kernel&m=147117565612275&w=2
> 
> 
> Ben Hutchings said this on his 3.16.37-rc1 patch:
> [bwh: Backported to 3.16:
>  - Adjust context
>  - Also set the flag in __d_materialise_dentry())]
> 
> http://marc.info/?l=linux-kernel&m=147117433412006&w=2
> 
> 
> Also mentioned by Sasha Levin on 3.18 and 4.1 commits:
> Cc: stable@vger.kernel.org # v3.2+ (and watch out for __d_materialise_dentry())
> 
> http://marc.info/?l=linux-stable-commits&m=146648034410827&w=2
> http://marc.info/?l=linux-stable-commits&m=146647471009771&w=2


Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 fs/dcache.c |    1 +
 1 file changed, 1 insertion(+)

--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2652,6 +2652,7 @@ static void __d_materialise_dentry(struc
 	switch_names(dentry, anon);
 	swap(dentry->d_name.hash, anon->d_name.hash);
 
+	dentry->d_flags |= DCACHE_RCUACCESS;
 	dentry->d_parent = dentry;
 	list_del_init(&dentry->d_child);
 	anon->d_parent = dparent;


Patches currently in stable-queue which might be from w@1wt.eu are

queue-3.14/fix-d_walk-non-delayed-__d_free-race.patch
queue-3.14/hid-hid-input-add-parentheses-to-quell-gcc-warning.patch
queue-3.14/revert-can-fix-handling-of-unmodifiable-configuration-options-fix.patch
queue-3.14/be2iscsi-fix-bogus-warn_on-length-check.patch
queue-3.14/stb6100-fix-buffer-length-check-in-stb6100_write_reg_range.patch
queue-3.14/alsa-oxygen-fix-logical-not-parentheses-warning.patch

[PATCH 3.10 099/180] fix d_walk()/non-delayed __d_free() race

[Willy Tarreau]

From: Al Viro <viro@zeniv.linux.org.uk>

commit 3d56c25e3bb0726a5c5e16fc2d9e38f8ed763085 upstream.

Ascend-to-parent logics in d_walk() depends on all encountered child
dentries not getting freed without an RCU delay.  Unfortunately, in
quite a few cases it is not true, with hard-to-hit oopsable race as
the result.

Fortunately, the fix is simiple; right now the rule is "if it ever
been hashed, freeing must be delayed" and changing it to "if it
ever had a parent, freeing must be delayed" closes that hole and
covers all cases the old rule used to cover.  Moreover, pipes and
sockets remain _not_ covered, so we do not introduce RCU delay in
the cases which are the reason for having that delay conditional
in the first place.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 fs/dcache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 17222fa..2a808fb 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1311,7 +1311,7 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
 	struct dentry *dentry = __d_alloc(parent->d_sb, name);
 	if (!dentry)
 		return NULL;
-
+	dentry->d_flags |= DCACHE_RCUACCESS;
 	spin_lock(&parent->d_lock);
 	/*
 	 * don't need child lock because it is not subject
@@ -2101,7 +2101,6 @@ static void __d_rehash(struct dentry * entry, struct hlist_bl_head *b)
 {
 	BUG_ON(!d_unhashed(entry));
 	hlist_bl_lock(b);
-	entry->d_flags |= DCACHE_RCUACCESS;
 	hlist_bl_add_head_rcu(&entry->d_hash, b);
 	hlist_bl_unlock(b);
 }
@@ -2285,6 +2284,7 @@ static void __d_move(struct dentry * dentry, struct dentry * target)
 
 	/* ... and switch the parents */
 	if (IS_ROOT(dentry)) {
+		dentry->d_flags |= DCACHE_RCUACCESS;
 		dentry->d_parent = target->d_parent;
 		target->d_parent = target;
 		INIT_LIST_HEAD(&target->d_child);
-- 
2.8.0.rc2.1.gbe9624a