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

Groups > linux.kernel > #1496869 > unrolled thread

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

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: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Al Viro <viro@ZenIV.linux.org.uk> - 2016-10-06 23:40 +0200
    Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Sinan Kaya <okaya@codeaurora.org> - 2016-10-06 23:50 +0200
      Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Sinan Kaya <okaya@codeaurora.org> - 2016-10-07 00:00 +0200
        Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Sinan Kaya <okaya@codeaurora.org> - 2016-10-07 00:20 +0200
      Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Al Viro <viro@ZenIV.linux.org.uk> - 2016-10-07 00:10 +0200
        Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Al Viro <viro@ZenIV.linux.org.uk> - 2016-10-07 00:40 +0200
          Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Al Viro <viro@ZenIV.linux.org.uk> - 2016-10-07 00:50 +0200
            Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative" Sinan Kaya <okaya@codeaurora.org> - 2016-10-07 01:30 +0200

#1496869 — Re: [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"

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.

[toc] | [next] | [standalone]

#1496877

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.

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

#1496878

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.

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

#1496883

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.

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

#1496880

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.

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

#1496890

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

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

#1496894

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.

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

#1496916

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web