Re: [PATCH] exit: fix a race in release_task when flushing the dentry

Re: [PATCH] exit: fix a race in release_task when flushing the dentry

[Greg Kroah-Hartman]

On Sat, Nov 28, 2020 at 02:47:22PM +0800, Wen Yang wrote:
> [ Upstream commit 7bc3e6e55acf065500a24621f3b313e7e5998acf ]

No, that is not this commit at all.

What are you wanting to have happen here?

confused,

greg k-h

Re: [PATCH] exit: fix a race in release_task when flushing the dentry

[Greg Kroah-Hartman]

On Sat, Nov 28, 2020 at 09:59:09PM +0800, Wen Yang wrote:
> 
> 
> 在 2020/11/28 下午4:06, Greg Kroah-Hartman 写道:
> > On Sat, Nov 28, 2020 at 02:47:22PM +0800, Wen Yang wrote:
> > > [ Upstream commit 7bc3e6e55acf065500a24621f3b313e7e5998acf ]
> > 
> > No, that is not this commit at all.
> > 
> > What are you wanting to have happen here?
> > 
> > confused,
> > 
> > greg k-h
> > 
> 
> Thanks.
> Let's explain it briefly:
> 
> The dentries such as /proc/<pid>/ns/ipc have the DCACHE_OP_DELETE flag, they
> should be deleted when the process exits.
> Suppose the following race appears：
> 
> release_task                dput
> -> proc_flush_task
>                             ->  dentry->d_op->d_delete(dentry)
> -> __exit_signal
>                             -> dentry->d_lockref.count--  and return.
> 
> 
> In the proc_flush_task function, because another processe is using this
> dentry, it cannot be deleted;
> In the dput function, d_delete may be executed before __exit_signal (the pid
> has not been unhashed), so that d_delete returns false and the dentry can
> not be deleted.
> 
> So this dentry is still caches (count is 0), and its parent dentries are
> also caches, and those dentries can only be deleted when drop_caches is
> manually triggered.
> 
> 
> In the release_task function, we should move proc_flush_task after the
> tasklist_lock is released（Just like the commit
> 7bc3e6e55acf065500a24621f3b313e7e5998acf did).

I do not understand, is this a patch being submitted for the main kernel
tree, or for a stable kernel release?

If stable, please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

If main kernel tree, you can't have the "Upstream commit" line in the
changelog text as that makes no sense at all.

thanks,

greg k-h

Re: [PATCH] exit: fix a race in release_task when flushing the dentry

[Greg Kroah-Hartman]

On Sat, Nov 28, 2020 at 11:28:53PM +0800, Wen Yang wrote:
> 
> 
> 在 2020/11/28 下午10:05, Greg Kroah-Hartman 写道:
> > On Sat, Nov 28, 2020 at 09:59:09PM +0800, Wen Yang wrote:
> > > 
> > > 
> > > 在 2020/11/28 下午4:06, Greg Kroah-Hartman 写道:
> > > > On Sat, Nov 28, 2020 at 02:47:22PM +0800, Wen Yang wrote:
> > > > > [ Upstream commit 7bc3e6e55acf065500a24621f3b313e7e5998acf ]
> > > > 
> > > > No, that is not this commit at all.
> > > > 
> > > > What are you wanting to have happen here?
> > > > 
> > > > confused,
> > > > 
> > > > greg k-h
> > > > 
> > > 
> > > Thanks.
> > > Let's explain it briefly:
> > > 
> > > The dentries such as /proc/<pid>/ns/ipc have the DCACHE_OP_DELETE flag, they
> > > should be deleted when the process exits.
> > > Suppose the following race appears：
> > > 
> > > release_task                dput
> > > -> proc_flush_task
> > >                              ->  dentry->d_op->d_delete(dentry)
> > > -> __exit_signal
> > >                              -> dentry->d_lockref.count--  and return.
> > > 
> > > 
> > > In the proc_flush_task function, because another processe is using this
> > > dentry, it cannot be deleted;
> > > In the dput function, d_delete may be executed before __exit_signal (the pid
> > > has not been unhashed), so that d_delete returns false and the dentry can
> > > not be deleted.
> > > 
> > > So this dentry is still caches (count is 0), and its parent dentries are
> > > also caches, and those dentries can only be deleted when drop_caches is
> > > manually triggered.
> > > 
> > > 
> > > In the release_task function, we should move proc_flush_task after the
> > > tasklist_lock is released（Just like the commit
> > > 7bc3e6e55acf065500a24621f3b313e7e5998acf did).
> > 
> > I do not understand, is this a patch being submitted for the main kernel
> > tree, or for a stable kernel release?
> > 
> > If stable, please read:
> >      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > If main kernel tree, you can't have the "Upstream commit" line in the
> > changelog text as that makes no sense at all.
> 
> 
> Hi,
> This patch is submitted to the stable branches (from 4.9.y
> to 5.6.y).
> 
> This problem can also be solved if the following patch could be ported to
> the stable branch:
> 7bc3e6e55acf ("proc: Use a list of inodes to flush from proc")
> 26dbc60f385f ("proc: Generalize proc_sys_prune_dcache into
> proc_prune_siblings_dcache")
> f90f3cafe8d5 ("proc: Use d_invalidate in proc_prune_siblings_dcache")
> 
> However, the above-mentioned patches modify too much code (more than 100
> lines), and there may also be some undiscovered bugs.
> 
> So the safer method may be to apply this small patch（also ported from the
> equivalent fix already exist in Linus’ tree）.
> 
> We will reformat the patch later.

We always prefer to take the original, upstream patches, instead of
one-off changes as almost always, those one-off changes end up being
wrong and hard to work with over time.

So if we need more than one patch to solve this reported problem, that's
fine, can you test the above series of patches and provide a backported
set of them that we can use for this?

thanks,

gre gk-h

Re: [PATCH] exit: fix a race in release_task when flushing the dentry

[Wen Yang]

在 2020/11/29 下午2:05, Greg Kroah-Hartman 写道:
> On Sat, Nov 28, 2020 at 11:28:53PM +0800, Wen Yang wrote:
>>
>>
>> 在 2020/11/28 下午10:05, Greg Kroah-Hartman 写道:
>>> On Sat, Nov 28, 2020 at 09:59:09PM +0800, Wen Yang wrote:
>>>>
>>>>
>>>> 在 2020/11/28 下午4:06, Greg Kroah-Hartman 写道:
>>>>> On Sat, Nov 28, 2020 at 02:47:22PM +0800, Wen Yang wrote:
>>>>>> [ Upstream commit 7bc3e6e55acf065500a24621f3b313e7e5998acf ]
>>>>>
>>>>> No, that is not this commit at all.
>>>>>
>>>>> What are you wanting to have happen here?
>>>>>
>>>>> confused,
>>>>>
>>>>> greg k-h
>>>>>
>>>>
>>>> Thanks.
>>>> Let's explain it briefly:
>>>>
>>>> The dentries such as /proc/<pid>/ns/ipc have the DCACHE_OP_DELETE flag, they
>>>> should be deleted when the process exits.
>>>> Suppose the following race appears：
>>>>
>>>> release_task                dput
>>>> -> proc_flush_task
>>>>                               ->  dentry->d_op->d_delete(dentry)
>>>> -> __exit_signal
>>>>                               -> dentry->d_lockref.count--  and return.
>>>>
>>>>
>>>> In the proc_flush_task function, because another processe is using this
>>>> dentry, it cannot be deleted;
>>>> In the dput function, d_delete may be executed before __exit_signal (the pid
>>>> has not been unhashed), so that d_delete returns false and the dentry can
>>>> not be deleted.
>>>>
>>>> So this dentry is still caches (count is 0), and its parent dentries are
>>>> also caches, and those dentries can only be deleted when drop_caches is
>>>> manually triggered.
>>>>
>>>>
>>>> In the release_task function, we should move proc_flush_task after the
>>>> tasklist_lock is released（Just like the commit
>>>> 7bc3e6e55acf065500a24621f3b313e7e5998acf did).
>>>
>>> I do not understand, is this a patch being submitted for the main kernel
>>> tree, or for a stable kernel release?
>>>
>>> If stable, please read:
>>>       https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>> for how to do this properly.
>>>
>>> If main kernel tree, you can't have the "Upstream commit" line in the
>>> changelog text as that makes no sense at all.
>>
>>
>> Hi,
>> This patch is submitted to the stable branches (from 4.9.y
>> to 5.6.y).
>>
>> This problem can also be solved if the following patch could be ported to
>> the stable branch:
>> 7bc3e6e55acf ("proc: Use a list of inodes to flush from proc")
>> 26dbc60f385f ("proc: Generalize proc_sys_prune_dcache into
>> proc_prune_siblings_dcache")
>> f90f3cafe8d5 ("proc: Use d_invalidate in proc_prune_siblings_dcache")
>>
>> However, the above-mentioned patches modify too much code (more than 100
>> lines), and there may also be some undiscovered bugs.
>>
>> So the safer method may be to apply this small patch（also ported from the
>> equivalent fix already exist in Linus’ tree）.
>>
>> We will reformat the patch later.
> 
> We always prefer to take the original, upstream patches, instead of
> one-off changes as almost always, those one-off changes end up being
> wrong and hard to work with over time.
> 
> So if we need more than one patch to solve this reported problem, that's
> fine, can you test the above series of patches and provide a backported
> set of them that we can use for this?
> 

Ok, we will follow your suggestions.
Thanks.

--
Best wishes,
Wen