[PATCH 2/2] fs: fix bug where dentry freed earlier, might be getting freed again.

[PATCH 2/2] fs: fix bug where dentry freed earlier, might be getting freed again.

[Santosh Madiraju]

From: madiraju <madiraju.santosh@gmail.com>

While shrinking the dentry list in shrink_dentry_list function,
if DCACHE_MAY_FREE flag is set, it frees the dentry, and it again
tries to do it in dentry_kill.

Signed-off-by: Santosh Madiraju <madiraju.santosh@gmail.com>
---
 fs/dcache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 92d5140..7aa2252 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -986,8 +986,8 @@ static void shrink_dentry_list(struct list_head *list)
 				spin_unlock(&parent->d_lock);
 			continue;
 		}
-
-		__dentry_kill(dentry);
+		if (dentry)
+			__dentry_kill(dentry);
 
 		/*
 		 * We need to prune ancestors too. This is necessary to prevent
-- 
1.9.1

Re: [PATCH 2/2] fs: fix bug where dentry freed earlier, might be getting freed again.

[Al Viro]

On Sat, Jan 30, 2016 at 12:51:46PM -0800, Santosh Madiraju wrote:
> From: madiraju <madiraju.santosh@gmail.com>
> 
> While shrinking the dentry list in shrink_dentry_list function,
> if DCACHE_MAY_FREE flag is set, it frees the dentry, and it again
> tries to do it in dentry_kill.
[snip]
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 92d5140..7aa2252 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -986,8 +986,8 @@ static void shrink_dentry_list(struct list_head *list)
>  				spin_unlock(&parent->d_lock);
>  			continue;
>  		}
> -
> -		__dentry_kill(dentry);
> +		if (dentry)
> +			__dentry_kill(dentry);

Considering the fact that this call of __dentry_kill() is immediately preceded
by
                inode = dentry->d_inode;
                if (inode && unlikely(!spin_trylock(&inode->i_lock))) {
                        d_shrink_add(dentry, list);
                        spin_unlock(&dentry->d_lock);
                        if (parent)
                                spin_unlock(&parent->d_lock);
                        continue;
                }
it is flat-out impossible to get to that call with dentry equal to NULL -
we would've oopsed on attempt to fetch dentry->d_inode, not to mention that
the value in dentry ultimately comes from
                dentry = list_entry(list->prev, struct dentry, d_lru);
which _never_ yields NULL.  And that access of dentry->d_inode is not the
only place in between where we would've oopsed with dentry being NULL,
while we are at it.

And description also makes no sense whatsoever - if we run into DCACHE_MAY_FREE
(which can't be set without DCACHE_DENTRY_KILLED having been set first), we
won't even reach that __dentry_kill() - we'll run into
                if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) {
                        bool can_free = dentry->d_flags & DCACHE_MAY_FREE;
                        spin_unlock(&dentry->d_lock);
                        if (parent)
                                spin_unlock(&parent->d_lock);
                        if (can_free)
                                dentry_free(dentry);
                        continue;
                }
and that's it as far as this dentry is concerned.  We'd done
d_shrink_del(dentry) - that's the very first thing we do and we do it
unconditionally.

What's more, making no sense is about the only thing description and patch
have in common.  NAK.