[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Sinan Kaya]

This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
negative") as it breaks the debugfs_remove_recursive API as show in the
callstack below.

Tested against:
c802e87 Add linux-next specific files for 20161006

Unable to handle kernel NULL pointer dereference at virtual address
000000a8
pgd = ffff800bc81c1000
[000000a8] *pgd=0000004bc83e1003, *pud=0000004bc6519003,
*pmd=0000000000000000
Internal error: Oops: 96000006 [#1] PREEMPT SMP
Modules linked in:
CPU: 13 PID: 1758 Comm: tee Not tainted 4.8.0-next-20161006-00013-gd66147c
Hardware name: (null) (DT)
task: ffff800bc7fe0000 task.stack: ffff800bc645c000
PC is at down_write+0x18/0x68
LR is at debugfs_remove_recursive+0x50/0x1c0
pc : [<ffff00000889f480>] lr : [<ffff00000831c220>] pstate: 80000145

[<ffff00000889f480>] down_write+0x18/0x68
[<ffff00000831c220>] debugfs_remove_recursive+0x50/0x1c0
[<ffff00000848e0a8>] hidma_debug_uninit+0x20/0x30
[<ffff00000848c5d8>] hidma_remove+0x48/0x98
[<ffff000008511b6c>] platform_drv_remove+0x24/0x68
[<ffff00000850fac8>] __device_release_driver+0x80/0x118
[<ffff00000850fb84>] device_release_driver+0x24/0x38
[<ffff00000850e928>] unbind_store+0xe8/0x110
[<ffff00000850dd30>] drv_attr_store+0x20/0x30
[<ffff000008253a48>] sysfs_kf_write+0x48/0x58
[<ffff000008252dd8>] kernfs_fop_write+0xb0/0x1d8
[<ffff0000081dab3c>] __vfs_write+0x1c/0x110
[<ffff0000081db940>] vfs_write+0xa0/0x1b8
[<ffff0000081dcd34>] SyS_write+0x44/0xa0
[<ffff000008082ef0>] el0_svc_naked+0x24/0x28

Reverting this change seems to fix the issue.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 fs/debugfs/inode.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f17fcf8..02166d6 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -621,6 +621,9 @@ void debugfs_remove(struct dentry *dentry)
 		return;
 
 	parent = dentry->d_parent;
+	if (!parent || d_really_is_negative(parent))
+		return;
+
 	inode_lock(d_inode(parent));
 	ret = __debugfs_remove(dentry, parent);
 	inode_unlock(d_inode(parent));
@@ -651,6 +654,10 @@ void debugfs_remove_recursive(struct dentry *dentry)
 	if (IS_ERR_OR_NULL(dentry))
 		return;
 
+	parent = dentry->d_parent;
+	if (!parent || d_really_is_negative(parent))
+		return;
+
 	parent = dentry;
  down:
 	inode_lock(d_inode(parent));
-- 
1.9.1

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Al Viro]

On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> negative") as it breaks the debugfs_remove_recursive API as show in the
> callstack below.

NAK.  Fix your code, don't break global asserts.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Sinan Kaya]

On 10/6/2016 5:37 PM, Al Viro wrote:
> On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
>> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
>> negative") as it breaks the debugfs_remove_recursive API as show in the
>> callstack below.
> 
> NAK.  Fix your code, don't break global asserts.
> 

I can fix the code if you tell me what the problem is:

http://lxr.free-electrons.com/ident?i=hidma_debug_uninit

http://lxr.free-electrons.com/ident?i=hidma_debug_init

The code didn't change between these commits.

git log --oneline fs/debugfs/inode.c

[doesn't work]
23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
c2050a4 fs: Replace current_fs_time() with current_time()
e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
acc29fb debugfs: ->d_parent is never NULL or negative
5614e77 Merge 4.6-rc4 into driver-core-next
87243de debugfs: Make automount point inodes permanently empty
c646880 debugfs: add support for self-protecting attribute file fops

[works]
dde78b1 Revert "debugfs: ->d_parent is never NULL or negative"
23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
c2050a4 fs: Replace current_fs_time() with current_time()
e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
acc29fb debugfs: ->d_parent is never NULL or negative
5614e77 Merge 4.6-rc4 into driver-core-next




-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Sinan Kaya]

On 10/6/2016 5:41 PM, Sinan Kaya wrote:
> On 10/6/2016 5:37 PM, Al Viro wrote:
>> On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
>>> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
>>> negative") as it breaks the debugfs_remove_recursive API as show in the
>>> callstack below.
>>
>> NAK.  Fix your code, don't break global asserts.
>>
> 
> I can fix the code if you tell me what the problem is:
> 
> http://lxr.free-electrons.com/ident?i=hidma_debug_uninit
> 
> http://lxr.free-electrons.com/ident?i=hidma_debug_init
> 

This is the directory structure:

/sys/kernel/debug/QCOM8061:00 # ls
chan0  stats
/sys/kernel/debug/QCOM8061:00 # cd chan0
/sys/kernel/debug/QCOM8061:00/chan0 # ls
stats

This is the QCOM8061:00 directory
debugfs_remove_recursive(dmadev->debugfs);

This is the stats file under the directory
debugfs_remove_recursive(dmadev->stats);

The fact that directory is removed might be leaving the stats in limbo.

Let me test without this line.


> The code didn't change between these commits.
> 
> git log --oneline fs/debugfs/inode.c
> 
> [doesn't work]
> 23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
> c2050a4 fs: Replace current_fs_time() with current_time()
> e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
> acc29fb debugfs: ->d_parent is never NULL or negative
> 5614e77 Merge 4.6-rc4 into driver-core-next
> 87243de debugfs: Make automount point inodes permanently empty
> c646880 debugfs: add support for self-protecting attribute file fops
> 
> [works]
> dde78b1 Revert "debugfs: ->d_parent is never NULL or negative"
> 23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
> c2050a4 fs: Replace current_fs_time() with current_time()
> e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
> acc29fb debugfs: ->d_parent is never NULL or negative
> 5614e77 Merge 4.6-rc4 into driver-core-next
> 
> 
> 
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Sinan Kaya]

On 10/6/2016 5:56 PM, Sinan Kaya wrote:
> The fact that directory is removed might be leaving the stats in limbo.
> 
> Let me test without this line.

This did the trick. Thanks for the heads up. The second line was a left over
from the code review. I was removing files piece by piece at the beginning. 
Then, somebody said why don't you use recursive. While changing it to recursive,
I forgot to remove the second line.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Al Viro]

On Thu, Oct 06, 2016 at 05:41:34PM -0400, Sinan Kaya wrote:
> On 10/6/2016 5:37 PM, Al Viro wrote:
> > On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> >> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> >> negative") as it breaks the debugfs_remove_recursive API as show in the
> >> callstack below.
> > 
> > NAK.  Fix your code, don't break global asserts.
> > 
> 
> I can fix the code if you tell me what the problem is:

Getting dentries with NULL ->d_parent should never, ever happen.  Find the
place where such a beast appears and you've got your problem.

The same goes for negative dentries with children.  Again, if your code
triggers such a situation, find where it does so and you've found a bug.
More than one, at that.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Al Viro]

On Thu, Oct 06, 2016 at 11:00:51PM +0100, Al Viro wrote:
> On Thu, Oct 06, 2016 at 05:41:34PM -0400, Sinan Kaya wrote:
> > On 10/6/2016 5:37 PM, Al Viro wrote:
> > > On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> > >> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> > >> negative") as it breaks the debugfs_remove_recursive API as show in the
> > >> callstack below.
> > > 
> > > NAK.  Fix your code, don't break global asserts.
> > > 
> > 
> > I can fix the code if you tell me what the problem is:
> 
> Getting dentries with NULL ->d_parent should never, ever happen.  Find the
> place where such a beast appears and you've got your problem.
> 
> The same goes for negative dentries with children.  Again, if your code
> triggers such a situation, find where it does so and you've found a bug.
> More than one, at that.

Note that there are exactly 4 places where ->d_parent of some struct
dentry is modified, all in fs/dcache.c.

1) __d_alloc() sets ->d_parent of new instance pointing to the instance
itself and does so before anyone else could observe that dentry.  That's
the only allocator of struct dentry - all of them start their life when
returned by it.  With non-NULL value of ->d_parent.

2) d_alloc() sets it to given (non-NULL) parent.  Note that it has
already dereferenced that parent (spin_lock(&parent->d_lock) a couple of
lines prior to that), so it would've oopsed before it reached that assignment
if it was passed NULL as parent.

3) d_alloc_cursor() - ditto, only there it had been an access to parent->d_sb.

4)?__d_move() does
                dentry->d_parent = target->d_parent;
                target->d_parent = target;
in one case and
                swap(dentry->d_parent, target->d_parent);
in another.  The values had either already been in ->d_parent of another
instance prior to that or are guaranteed to be non-NULL since we'd just
survived dereferencing them.

If you ever get NULL in ->d_parent of struct dentry instance, you are
practically certain to have a dangling pointer to memory that used to
contain a struct dentry at some point but got freed and reused since then.
Any such case is a bug, and this check only papers over that bug - after all,
we might have very well reused it for anything whatsoever, with arbitrary
values ending up in it.

As for the negatives...
	* if ->d_parent points to something other than dentry itself,
it contributes to ->d_count of parent
	* positive dentry can only be turned into negative if the
caller of d_delete() is holding the only reference to it
	* any code setting ->d_parent to another dentry does so only when
the parent to be is known to be positive at the moment.

So if you get a dentry with negative parent passed to it, the only way
it could happen (aside of outright memory corruption) is that dentry
you are passing has ->d_parent pointing to *itself* (and had never been
made positive).  If that can legitimately happen, the proper test is
IS_ROOT(dentry) && !dentry->d_inode, and I strongly suspect that the
second part is irrelevant.  But I would really like to see what leads
to that - I don't see any way for debugfs_create_{file,dir}() et.al. to
return a detached (or negative, for that matter) dentry.

are

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Al Viro]

On Thu, Oct 06, 2016 at 11:37:29PM +0100, Al Viro wrote:

> If you ever get NULL in ->d_parent of struct dentry instance, you are
> practically certain to have a dangling pointer to memory that used to
> contain a struct dentry at some point but got freed and reused since then.

... which is what happens in your case, apparently.  ->stats is still
pointing to a dentry that had just been freed and its memory reused.

[PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

[Sinan Kaya]

On 10/6/2016 6:41 PM, Al Viro wrote:
> On Thu, Oct 06, 2016 at 11:37:29PM +0100, Al Viro wrote:
> 
>> If you ever get NULL in ->d_parent of struct dentry instance, you are
>> practically certain to have a dangling pointer to memory that used to
>> contain a struct dentry at some point but got freed and reused since then.
> 
> ... which is what happens in your case, apparently.  ->stats is still
> pointing to a dentry that had just been freed and its memory reused.
> 

Thanks for explaining the behavior. I posted the change a minute ago
and forgot to include you. 

dmaengine: qcom_hidma: remove useless debugfs file removal

I have a very similar problem with sysfs now. It looks like the new kernel
is more assertive than the older ones. 

I'll post the sysfs change in a minute. 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.