This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
functions rcu_dereference(...) to use when dereferencing a __rcu
pointer. This functions returns the non __rcu tagged pointer.
Like normal pointer there should be a check for null case when
further dereferencing the returned dereferenced __rcu pointer.
Signed-off-by: Abhinav Singh <[email protected]>
---
kernel/fork.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..5afb1b389a66 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,9 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ const struct cred *real_cred = rcu_dereference(p->real_cred);
+
+ if (real_cred && real_cred->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
--
2.39.2
On Thu, 26 Oct 2023 03:58:11 +0530 Abhinav Singh <[email protected]> wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> functions rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This functions returns the non __rcu tagged pointer.
Seems sensible.
> Like normal pointer there should be a check for null case when
> further dereferencing the returned dereferenced __rcu pointer.
Why is this?
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,9 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + const struct cred *real_cred = rcu_dereference(p->real_cred);
> +
> + if (real_cred && real_cred->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
The old code assumes that p->read_cred cannot be NULL and the new code
does nothing to make it possible that `real_cred' can be NULL?
In other words, I see no reason to add this new check for NULL?
On 10/26/23 04:08, Andrew Morton wrote:
> On Thu, 26 Oct 2023 03:58:11 +0530 Abhinav Singh <[email protected]> wrote:
>
>> This patch fixes the warning about directly dereferencing a pointer
>> tagged with __rcu annotation.
>>
>> Dereferencing the pointers tagged with __rcu directly should
>> always be avoided according to the docs. There is a rcu helper
>> functions rcu_dereference(...) to use when dereferencing a __rcu
>> pointer. This functions returns the non __rcu tagged pointer.
>
> Seems sensible.
>
>> Like normal pointer there should be a check for null case when
>> further dereferencing the returned dereferenced __rcu pointer.
>
> Why is this?
>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -2369,7 +2369,9 @@ __latent_entropy struct task_struct *copy_process(
>>
>> retval = -EAGAIN;
>> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
>> - if (p->real_cred->user != INIT_USER &&
>> + const struct cred *real_cred = rcu_dereference(p->real_cred);
>> +
>> + if (real_cred && real_cred->user != INIT_USER &&
>> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>> goto bad_fork_cleanup_count;
>
> The old code assumes that p->read_cred cannot be NULL and the new code
> does nothing to make it possible that `real_cred' can be NULL?
>
> In other words, I see no reason to add this new check for NULL?
Thank you for the response!
I thought it will be better to have check before accessing it, just so
we dont have any segmentation fault in future.
Also I just noticed there are two more places where direct dereferencing
of __rcu pointer is done in this same file. Should I do those changes in
this patch ?
On Thu, 26 Oct 2023 04:57:42 +0530 Abhinav Singh <[email protected]> wrote:
> On 10/26/23 04:08, Andrew Morton wrote:
> >> +++ b/kernel/fork.c
> >> @@ -2369,7 +2369,9 @@ __latent_entropy struct task_struct *copy_process(
> >>
> >> retval = -EAGAIN;
> >> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> >> - if (p->real_cred->user != INIT_USER &&
> >> + const struct cred *real_cred = rcu_dereference(p->real_cred);
> >> +
> >> + if (real_cred && real_cred->user != INIT_USER &&
> >> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> >> goto bad_fork_cleanup_count;
> >
> > The old code assumes that p->read_cred cannot be NULL and the new code
> > does nothing to make it possible that `real_cred' can be NULL?
> >
> > In other words, I see no reason to add this new check for NULL?
>
> Thank you for the response!
>
> I thought it will be better to have check before accessing it, just so
> we dont have any segmentation fault in future.
That would be adding code which has no effect?
> Also I just noticed there are two more places where direct dereferencing
> of __rcu pointer is done in this same file. Should I do those changes in
> this patch ?
I don't see why. rcu_dereference(p) cannot return NULL if `p' is non-NULL?
This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
functions rcu_dereference(...) to use when dereferencing a __rcu
pointer. This functions returns the non __rcu tagged pointer which
can be dereferenced just like a normal pointers.
Signed-off-by: Abhinav Singh <[email protected]>
---
kernel/fork.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..802b7bbe3d92 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ if (rcu_dereference(p->real_cred)->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
@@ -2692,7 +2692,7 @@ __latent_entropy struct task_struct *copy_process(
*/
p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
p->real_parent->signal->is_child_subreaper;
- list_add_tail(&p->sibling, &p->real_parent->children);
+ list_add_tail(&p->sibling, &(rcu_dereference(p->real_parent)->children));
list_add_tail_rcu(&p->tasks, &init_task.tasks);
attach_pid(p, PIDTYPE_TGID);
attach_pid(p, PIDTYPE_PGID);
--
2.39.2
On 10/26/23 05:20, Andrew Morton wrote:
> On Thu, 26 Oct 2023 04:57:42 +0530 Abhinav Singh <[email protected]> wrote:
>
>> On 10/26/23 04:08, Andrew Morton wrote:
>>>> +++ b/kernel/fork.c
>>>> @@ -2369,7 +2369,9 @@ __latent_entropy struct task_struct *copy_process(
>>>>
>>>> retval = -EAGAIN;
>>>> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
>>>> - if (p->real_cred->user != INIT_USER &&
>>>> + const struct cred *real_cred = rcu_dereference(p->real_cred);
>>>> +
>>>> + if (real_cred && real_cred->user != INIT_USER &&
>>>> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>>>> goto bad_fork_cleanup_count;
>>>
>>> The old code assumes that p->read_cred cannot be NULL and the new code
>>> does nothing to make it possible that `real_cred' can be NULL?
>>>
>>> In other words, I see no reason to add this new check for NULL?
>>
>> Thank you for the response!
>>
>> I thought it will be better to have check before accessing it, just so
>> we dont have any segmentation fault in future.
>
> That would be adding code which has no effect?
>
>> Also I just noticed there are two more places where direct dereferencing
>> of __rcu pointer is done in this same file. Should I do those changes in
>> this patch ?
>
> I don't see why. rcu_dereference(p) cannot return NULL if `p' is non-NULL?
This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
functions rcu_dereference(...) to use when dereferencing a __rcu
pointer. This functions returns the non __rcu tagged pointer which
can be dereferenced just like a normal pointers.
Signed-off-by: Abhinav Singh <[email protected]>
---
kernel/fork.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..802b7bbe3d92 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ if (rcu_dereference(p->real_cred)->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
@@ -2692,7 +2692,7 @@ __latent_entropy struct task_struct *copy_process(
*/
p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
p->real_parent->signal->is_child_subreaper;
- list_add_tail(&p->sibling, &p->real_parent->children);
+ list_add_tail(&p->sibling, &(rcu_dereference(p->real_parent)->children));
list_add_tail_rcu(&p->tasks, &init_task.tasks);
attach_pid(p, PIDTYPE_TGID);
attach_pid(p, PIDTYPE_PGID);
--
2.39.2
On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> functions rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This functions returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointers.
>
> Signed-off-by: Abhinav Singh <[email protected]>
Well yes but these need to be called under rcu_read_lock.
Who does it here?
If no one then maybe you found an actual bug and we need to
fix it not paper over it.
> ---
> kernel/fork.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..802b7bbe3d92 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
> @@ -2692,7 +2692,7 @@ __latent_entropy struct task_struct *copy_process(
> */
> p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + list_add_tail(&p->sibling, &(rcu_dereference(p->real_parent)->children));
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
> --
> 2.39.2
On 10/26/23, Michael S. Tsirkin <[email protected]> wrote:
> On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
>> This patch fixes the warning about directly dereferencing a pointer
>> tagged with __rcu annotation.
>>
>> Dereferencing the pointers tagged with __rcu directly should
>> always be avoided according to the docs. There is a rcu helper
>> functions rcu_dereference(...) to use when dereferencing a __rcu
>> pointer. This functions returns the non __rcu tagged pointer which
>> can be dereferenced just like a normal pointers.
>>
>> Signed-off-by: Abhinav Singh <[email protected]>
>
> Well yes but these need to be called under rcu_read_lock.
> Who does it here?
> If no one then maybe you found an actual bug and we need to
> fix it not paper over it.
>
There is no bug here.
p is the newly created thread, ->real_cred was initialized just prior
to this code and there is nobody to whack the creds from under it.
Second bit in the patch changes one real_parent deref, but leaves 2
others just above it. Once more no bug since the entire thing happens
under tasklist_lock, but the patch should either sort all these cases
or none.
I think it would help if the submitter had shown warnings they see.
--
Mateusz Guzik <mjguzik gmail.com>
On 2023-10-26 10:06, Mateusz Guzik wrote:
> On 10/26/23, Michael S. Tsirkin <[email protected]> wrote:
>> On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
>>> This patch fixes the warning about directly dereferencing a pointer
>>> tagged with __rcu annotation.
>>>
>>> Dereferencing the pointers tagged with __rcu directly should
>>> always be avoided according to the docs. There is a rcu helper
>>> functions rcu_dereference(...) to use when dereferencing a __rcu
>>> pointer. This functions returns the non __rcu tagged pointer which
>>> can be dereferenced just like a normal pointers.
>>>
>>> Signed-off-by: Abhinav Singh <[email protected]>
>>
>> Well yes but these need to be called under rcu_read_lock.
>> Who does it here?
>> If no one then maybe you found an actual bug and we need to
>> fix it not paper over it.
>>
>
> There is no bug here.
>
> p is the newly created thread, ->real_cred was initialized just prior
> to this code and there is nobody to whack the creds from under it.
>
> Second bit in the patch changes one real_parent deref, but leaves 2
> others just above it. Once more no bug since the entire thing happens
> under tasklist_lock, but the patch should either sort all these cases
> or none.
Drive-by comment: perhaps use rcu_dereference_protected() ?
Thanks,
Mathieu
>
> I think it would help if the submitter had shown warnings they see.
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
$Subject should indicate a subsystem, also you seem to have a somewhat
random collection of Cc. It looks like dhowells is the cred guy and he's
not on.
On Thu, Oct 26, 2023 at 05:57:48PM +0530, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> functions rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This functions returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointers.
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> kernel/fork.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..802b7bbe3d92 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
This seems entirely misguided and only makes the code more confusing.
AFAICT at this point @p is not life, we're constructing the new task,
but it's not yet published, therefore no concurrency possible.
Additionally we're not actually in an RCU critical section afaict.
> @@ -2692,7 +2692,7 @@ __latent_entropy struct task_struct *copy_process(
> */
> p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + list_add_tail(&p->sibling, &(rcu_dereference(p->real_parent)->children));
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
As to the real_parent, we hold the tasklist lock, which is the write
side lock for parent stuff, so rcu dereference is pointless here.
On 10/26/23, Mathieu Desnoyers <[email protected]> wrote:
> Drive-by comment: perhaps use rcu_dereference_protected() ?
>
Definitely.
But as I mentioned even after applying the patch there are uses which
should have been reported (and consequently sorted out). If one is to
bother with any of this at least the entire file should be covered.
--
Mateusz Guzik <mjguzik gmail.com>
On Thu, Oct 26, 2023 at 04:06:24PM +0200, Mateusz Guzik wrote:
> On 10/26/23, Michael S. Tsirkin <[email protected]> wrote:
> > On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
> >> This patch fixes the warning about directly dereferencing a pointer
> >> tagged with __rcu annotation.
> >>
> >> Dereferencing the pointers tagged with __rcu directly should
> >> always be avoided according to the docs. There is a rcu helper
> >> functions rcu_dereference(...) to use when dereferencing a __rcu
> >> pointer. This functions returns the non __rcu tagged pointer which
> >> can be dereferenced just like a normal pointers.
> >>
> >> Signed-off-by: Abhinav Singh <[email protected]>
> >
> > Well yes but these need to be called under rcu_read_lock.
> > Who does it here?
> > If no one then maybe you found an actual bug and we need to
> > fix it not paper over it.
> >
>
> There is no bug here.
>
> p is the newly created thread, ->real_cred was initialized just prior
> to this code and there is nobody to whack the creds from under it.
>
> Second bit in the patch changes one real_parent deref, but leaves 2
> others just above it. Once more no bug since the entire thing happens
> under tasklist_lock, but the patch should either sort all these cases
> or none.
>
> I think it would help if the submitter had shown warnings they see.
Yes, and this must be tested under lockdep, which I think would
spit out warnings for this patch.
What should be used here I'm not sure. IIUC rcu_dereference_protected(p, 1)
is discouraged now?
--
MST
On 10/26/23 20:47, Michael S. Tsirkin wrote:
> On Thu, Oct 26, 2023 at 04:06:24PM +0200, Mateusz Guzik wrote:
>> On 10/26/23, Michael S. Tsirkin <[email protected]> wrote:
>>> On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
>>>> This patch fixes the warning about directly dereferencing a pointer
>>>> tagged with __rcu annotation.
>>>>
>>>> Dereferencing the pointers tagged with __rcu directly should
>>>> always be avoided according to the docs. There is a rcu helper
>>>> functions rcu_dereference(...) to use when dereferencing a __rcu
>>>> pointer. This functions returns the non __rcu tagged pointer which
>>>> can be dereferenced just like a normal pointers.
>>>>
>>>> Signed-off-by: Abhinav Singh <[email protected]>
>>>
>>> Well yes but these need to be called under rcu_read_lock.
>>> Who does it here?
>>> If no one then maybe you found an actual bug and we need to
>>> fix it not paper over it.
>>>
>>
>> There is no bug here.
>>
>> p is the newly created thread, ->real_cred was initialized just prior
>> to this code and there is nobody to whack the creds from under it.
>>
>> Second bit in the patch changes one real_parent deref, but leaves 2
>> others just above it. Once more no bug since the entire thing happens
>> under tasklist_lock, but the patch should either sort all these cases
>> or none.
Sparse reported 3 similar dereferencing warning this patch contains 2
fixes for 2, but yeah I should fixed all 3 of them.
>>
>> I think it would help if the submitter had shown warnings they see.
The warning message :- warning: dereference of noderef expression
>
> Yes, and this must be tested under lockdep, which I think would
> spit out warnings for this patch.
Not sure, but I tested this with sparse (make C=2) and after the above
changes I dont get the warning.
>
> What should be used here I'm not sure. IIUC rcu_dereference_protected(p, 1)
> is discouraged now?
>
Not sure but I read that, rcu_dereference should be prefered when
reading and rcu_dereference_protected should when writing.
On Thu, Oct 26, 2023 at 09:07:46PM +0530, Abhinav Singh wrote:
> On 10/26/23 20:47, Michael S. Tsirkin wrote:
> > On Thu, Oct 26, 2023 at 04:06:24PM +0200, Mateusz Guzik wrote:
> > > On 10/26/23, Michael S. Tsirkin <[email protected]> wrote:
> > > > On Thu, Oct 26, 2023 at 05:46:21PM +0530, Abhinav Singh wrote:
> > > > > This patch fixes the warning about directly dereferencing a pointer
> > > > > tagged with __rcu annotation.
> > > > >
> > > > > Dereferencing the pointers tagged with __rcu directly should
> > > > > always be avoided according to the docs. There is a rcu helper
> > > > > functions rcu_dereference(...) to use when dereferencing a __rcu
> > > > > pointer. This functions returns the non __rcu tagged pointer which
> > > > > can be dereferenced just like a normal pointers.
> > > > >
> > > > > Signed-off-by: Abhinav Singh <[email protected]>
> > > >
> > > > Well yes but these need to be called under rcu_read_lock.
> > > > Who does it here?
> > > > If no one then maybe you found an actual bug and we need to
> > > > fix it not paper over it.
> > > >
> > >
> > > There is no bug here.
> > >
> > > p is the newly created thread, ->real_cred was initialized just prior
> > > to this code and there is nobody to whack the creds from under it.
> > >
> > > Second bit in the patch changes one real_parent deref, but leaves 2
> > > others just above it. Once more no bug since the entire thing happens
> > > under tasklist_lock, but the patch should either sort all these cases
> > > or none.
> Sparse reported 3 similar dereferencing warning this patch contains 2 fixes
> for 2, but yeah I should fixed all 3 of them.
> > >
> > > I think it would help if the submitter had shown warnings they see.
> The warning message :- warning: dereference of noderef expression
> >
> > Yes, and this must be tested under lockdep, which I think would
> > spit out warnings for this patch.
> Not sure, but I tested this with sparse (make C=2) and after the above
> changes I dont get the warning.
sparse is a static analysis tool. You should also actually
test your patch.
> >
> > What should be used here I'm not sure. IIUC rcu_dereference_protected(p, 1)
> > is discouraged now?
> >
> Not sure but I read that, rcu_dereference should be prefered when reading
> and rcu_dereference_protected should when writing.
This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
functions rcu_dereference(...) to use when dereferencing a __rcu
pointer. This functions returns the non __rcu tagged pointer which
can be dereferenced just like a normal pointers.
Signed-off-by: Abhinav Singh <[email protected]>
---
kernel/fork.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..e78649974669 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ if (rcu_dereference(p->real_cred)->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
@@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
* tasklist_lock with adding child to the process tree
* for propagate_has_child_subreaper optimization.
*/
- p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
- p->real_parent->signal->is_child_subreaper;
- list_add_tail(&p->sibling, &p->real_parent->children);
+ p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
+ rcu_dereference(p->real_parent)->signal->is_child_subreaper;
+ list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
list_add_tail_rcu(&p->tasks, &init_task.tasks);
attach_pid(p, PIDTYPE_TGID);
attach_pid(p, PIDTYPE_PGID);
--
2.39.2
On 10/27/23 12:07, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> functions rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This functions returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointers.
>
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> kernel/fork.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..e78649974669 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
> @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
> * tasklist_lock with adding child to the process tree
> * for propagate_has_child_subreaper optimization.
> */
> - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> - p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
> + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
> + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
For this particular file I have resolved the rcu pointer dereferencing
issue and I have tested the above by using qemu using this command
qemu-system-x86_64 \
-m 2G \
-smp 2 \
-kernel /home/abhinav/linux_work/linux/arch/x86/boot/bzImage \
-append "console=ttyS0 root=/dev/sda earlyprintk=serial net.ifnames=0" \
-drive file=/home/abhinav/linux_work/boot_images/bullseye.img,format=raw \
-net user,host=10.0.2.10,hostfwd=tcp:127.0.0.1:10021-:22 \
-net nic,model=e1000 \
-enable-kvm \
-nographic \
-pidfile vm.pid \
2>&1 | tee vm.log
it booted without any issues.
On Fri, Oct 27, 2023 at 12:07:13PM +0530, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> functions
function
> rcu_dereference(...) to use when dereferencing a __rcu
> pointer.
... inside rcu read side critical sections.
> This functions
function
> returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointers.
pointer
>
>
Extra empty line here.
Did you test this with lockdep on or did you just build it?
Include info on how the patch was tested pls.
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
Changelog?
> kernel/fork.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..e78649974669 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
> @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
> * tasklist_lock with adding child to the process tree
> * for propagate_has_child_subreaper optimization.
> */
> - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> - p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
> + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
> + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
It looks like you are calling rcu_dereference outside of
read side critical section and that does not look right to me.
Test with lockdep on.
> --
> 2.39.2
On Fri, Oct 27, 2023 at 12:11:47PM +0530, Abhinav Singh wrote:
> On 10/27/23 12:07, Abhinav Singh wrote:
> > This patch fixes the warning about directly dereferencing a pointer
> > tagged with __rcu annotation.
> >
> > Dereferencing the pointers tagged with __rcu directly should
> > always be avoided according to the docs.
> There is a rcu helper
> > functions rcu_dereference(...) to use when dereferencing a __rcu
> > pointer. This functions returns the non __rcu tagged pointer which
> > can be dereferenced just like a normal pointers.
> >
> >
> > Signed-off-by: Abhinav Singh <[email protected]>
> > ---
> > kernel/fork.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 10917c3e1f03..e78649974669 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
> > retval = -EAGAIN;
> > if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > - if (p->real_cred->user != INIT_USER &&
> > + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> > !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > goto bad_fork_cleanup_count;
> > }
> > @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
> > * tasklist_lock with adding child to the process tree
> > * for propagate_has_child_subreaper optimization.
> > */
> > - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> > - p->real_parent->signal->is_child_subreaper;
> > - list_add_tail(&p->sibling, &p->real_parent->children);
> > + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
> > + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
> > + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
> > list_add_tail_rcu(&p->tasks, &init_task.tasks);
> > attach_pid(p, PIDTYPE_TGID);
> > attach_pid(p, PIDTYPE_PGID);
>
> For this particular file I have resolved the rcu pointer dereferencing issue
> and I have tested the above by using qemu using this command
> qemu-system-x86_64 \
> -m 2G \
> -smp 2 \
> -kernel /home/abhinav/linux_work/linux/arch/x86/boot/bzImage \
> -append "console=ttyS0 root=/dev/sda earlyprintk=serial net.ifnames=0" \
> -drive file=/home/abhinav/linux_work/boot_images/bullseye.img,format=raw \
> -net user,host=10.0.2.10,hostfwd=tcp:127.0.0.1:10021-:22 \
> -net nic,model=e1000 \
> -enable-kvm \
> -nographic \
> -pidfile vm.pid \
> 2>&1 | tee vm.log
> it booted without any issues.
Did you enable lockdep in your kernel?
CONFIG_PROVE_LOCKING and CONFIG_PROVE_RCU in particular.
--
MST
On 10/26, Peter Zijlstra wrote:
>
> > kernel/fork.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 10917c3e1f03..802b7bbe3d92 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
> >
> > retval = -EAGAIN;
> > if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> > - if (p->real_cred->user != INIT_USER &&
> > + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> > !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> > goto bad_fork_cleanup_count;
> > }
>
> This seems entirely misguided and only makes the code more confusing.
>
> AFAICT at this point @p is not life, we're constructing the new task,
> but it's not yet published, therefore no concurrency possible.
> Additionally we're not actually in an RCU critical section afaict.
>
> > @@ -2692,7 +2692,7 @@ __latent_entropy struct task_struct *copy_process(
> > */
> > p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> > p->real_parent->signal->is_child_subreaper;
> > - list_add_tail(&p->sibling, &p->real_parent->children);
> > + list_add_tail(&p->sibling, &(rcu_dereference(p->real_parent)->children));
> > list_add_tail_rcu(&p->tasks, &init_task.tasks);
> > attach_pid(p, PIDTYPE_TGID);
> > attach_pid(p, PIDTYPE_PGID);
>
> As to the real_parent, we hold the tasklist lock, which is the write
> side lock for parent stuff, so rcu dereference is pointless here.
Agreed.
Plus I don't think this change is correct, iiuc rcu_dereference() will trigger
the run-time "suspicious rcu_dereference_check() usage" warning, it is called
without rcu_read_lock().
Oleg.
This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
function rcu_dereference(...) to use when dereferencing a __rcu
pointer. This function returns the non __rcu tagged pointer which
can be dereferenced just like a normal pointer.
Signed-off-by: Abhinav Singh <[email protected]>
---
v1 -> v2 : added rcu_dereference(...) at line 2694
v2 -> v3 : added rcu_dereference(...) at line 2695
kernel/fork.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..e78649974669 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ if (rcu_dereference(p->real_cred)->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
@@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
* tasklist_lock with adding child to the process tree
* for propagate_has_child_subreaper optimization.
*/
- p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
- p->real_parent->signal->is_child_subreaper;
- list_add_tail(&p->sibling, &p->real_parent->children);
+ p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
+ rcu_dereference(p->real_parent)->signal->is_child_subreaper;
+ list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
list_add_tail_rcu(&p->tasks, &init_task.tasks);
attach_pid(p, PIDTYPE_TGID);
attach_pid(p, PIDTYPE_PGID);
--
2.39.2
On 10/28/23 15:52, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> function rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This function returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointer.
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> v1 -> v2 : added rcu_dereference(...) at line 2694
> v2 -> v3 : added rcu_dereference(...) at line 2695
>
> kernel/fork.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..e78649974669 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
> @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
> * tasklist_lock with adding child to the process tree
> * for propagate_has_child_subreaper optimization.
> */
> - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> - p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
> + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
> + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
I tested the above with these two config enabled, CONFIG_PROVE_LOCKING
and CONFIG_PROVE_RCU. In qemu it booted fine without any issues. I then
checked dmesg log (inside booted qemu envirnoment) for any issues with
rcu, but didnt get any error or warning.
I didnt receive any warning message "suspicious rcu_dereference_check()
usage" atleast from sparse tool or in the dmesg log as suggested by Oleg
Nesterov.
On Sat, Oct 28, 2023 at 03:52:47PM +0530, Abhinav Singh wrote:
> This patch fixes the warning about directly dereferencing a pointer
> tagged with __rcu annotation.
>
> Dereferencing the pointers tagged with __rcu directly should
> always be avoided according to the docs. There is a rcu helper
> function rcu_dereference(...) to use when dereferencing a __rcu
> pointer. This function returns the non __rcu tagged pointer which
> can be dereferenced just like a normal pointer.
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> v1 -> v2 : added rcu_dereference(...) at line 2694
> v2 -> v3 : added rcu_dereference(...) at line 2695
>
> kernel/fork.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..e78649974669 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>
> retval = -EAGAIN;
> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
> - if (p->real_cred->user != INIT_USER &&
> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> goto bad_fork_cleanup_count;
> }
> @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
> * tasklist_lock with adding child to the process tree
> * for propagate_has_child_subreaper optimization.
> */
> - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
> - p->real_parent->signal->is_child_subreaper;
> - list_add_tail(&p->sibling, &p->real_parent->children);
> + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
> + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
> + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
> list_add_tail_rcu(&p->tasks, &init_task.tasks);
> attach_pid(p, PIDTYPE_TGID);
> attach_pid(p, PIDTYPE_PGID);
> --
> 2.39.2
You seem to just ignore review comments. NAK. I'm not going to review this anymore.
--
MST
On 10/28/23 17:50, Michael S. Tsirkin wrote:
> On Sat, Oct 28, 2023 at 03:52:47PM +0530, Abhinav Singh wrote:
>> This patch fixes the warning about directly dereferencing a pointer
>> tagged with __rcu annotation.
>>
>> Dereferencing the pointers tagged with __rcu directly should
>> always be avoided according to the docs. There is a rcu helper
>> function rcu_dereference(...) to use when dereferencing a __rcu
>> pointer. This function returns the non __rcu tagged pointer which
>> can be dereferenced just like a normal pointer.
>>
>> Signed-off-by: Abhinav Singh <[email protected]>
>> ---
>> v1 -> v2 : added rcu_dereference(...) at line 2694
>> v2 -> v3 : added rcu_dereference(...) at line 2695
>>
>> kernel/fork.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 10917c3e1f03..e78649974669 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
>>
>> retval = -EAGAIN;
>> if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
>> - if (p->real_cred->user != INIT_USER &&
>> + if (rcu_dereference(p->real_cred)->user != INIT_USER &&
>> !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>> goto bad_fork_cleanup_count;
>> }
>> @@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
>> * tasklist_lock with adding child to the process tree
>> * for propagate_has_child_subreaper optimization.
>> */
>> - p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
>> - p->real_parent->signal->is_child_subreaper;
>> - list_add_tail(&p->sibling, &p->real_parent->children);
>> + p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
>> + rcu_dereference(p->real_parent)->signal->is_child_subreaper;
>> + list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
>> list_add_tail_rcu(&p->tasks, &init_task.tasks);
>> attach_pid(p, PIDTYPE_TGID);
>> attach_pid(p, PIDTYPE_PGID);
>> --
>> 2.39.2
>
>
> You seem to just ignore review comments. NAK. I'm not going to review this anymore.
>
>
I m really sorry for ignorance and careless behaviour. This is
completely my fault, a maintainer has to do a lot of work and he cant
correct me all the times. On my defense I will only say that I was
really confused about a thing, instead of asking you question, I thought
of sending in another patch with some more information is a better
choice then a clearing my confusion by sending in a extra mail. You were
very concise and clear about your comments but MY stupidity was on
another level today. I m sorry for ignorant behaviour. And also thanks
and appreciate a lot for reviewing this patch till now to all the
maintainers.
Not sure if this patch will be reviewed again or not, but I think I
should answer the queries.
The last patch I sent, I tested with `lockdep` on (I hope "test with
`lockdep on`" means booting the kernel with lockdep enabled),
with these config options `CONFIG_PROVE_RCU` and `CONFIG_PROVE_LOCKING`
enabled and it booted just fine. To confirm if lockdep was really
enabled I found these paths inside the qemu virtual envirnoment
/proc/lockdep
/proc/lockdep_chains
/proc/lockdep_stat
/proc/locks
/proc/lock_stats
I tested the above kernel using qemu with this command
qemu-system-x86_64 \
-m 2G \
-smp 2 \
-kernel /home/abhinav/linux_work/linux/arch/x86/boot/bzImage \
-append "console=ttyS0 root=/dev/sda earlyprintk=serial net.ifnames=0" \
-drive file=/home/abhinav/linux_work/boot_images/bullseye.img,format=raw \
-net user,host=10.0.2.10,hostfwd=tcp:127.0.0.1:10021-:22 \
-net nic,model=e1000 \
-enable-kvm \
-nographic \
-pidfile vm.pid \
2>&1 | tee vm.log
I did not get warning `the run-time "suspicious rcu_dereference_check()
usage"` as mentioned by Oleg Nesterov, which mean rcu_dereference(...)
it called inside of rcu read side critical sections.
This patch fixes the warning about directly dereferencing a pointer
tagged with __rcu annotation.
Dereferencing the pointers tagged with __rcu directly should
always be avoided according to the docs. There is a rcu helper
function rcu_dereference(...) to use when dereferencing a __rcu
pointer inside rcu read side critical sections. This function
returns the non __rcu tagged pointer which can be dereferenced
just like a normal pointer.
Signed-off-by: Abhinav Singh <[email protected]>
---
Link to original patch
https://lore.kernel.org/all/[email protected]/
Change from original -> v2 :
1. removed the null check before dereferencing the dereferenced rcu
pointer at line 2372.
2. added rcu_dereference(...) at line 2694
Changes from v2 -> v3
1. added rcu_dereference(...) at line 2693
kernel/fork.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..e78649974669 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2369,7 +2369,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
- if (p->real_cred->user != INIT_USER &&
+ if (rcu_dereference(p->real_cred)->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
@@ -2690,9 +2690,9 @@ __latent_entropy struct task_struct *copy_process(
* tasklist_lock with adding child to the process tree
* for propagate_has_child_subreaper optimization.
*/
- p->signal->has_child_subreaper = p->real_parent->signal->has_child_subreaper ||
- p->real_parent->signal->is_child_subreaper;
- list_add_tail(&p->sibling, &p->real_parent->children);
+ p->signal->has_child_subreaper = rcu_dereference(p->real_parent)->signal->has_child_subreaper ||
+ rcu_dereference(p->real_parent)->signal->is_child_subreaper;
+ list_add_tail(&p->sibling, &rcu_dereference(p->real_parent)->children);
list_add_tail_rcu(&p->tasks, &init_task.tasks);
attach_pid(p, PIDTYPE_TGID);
attach_pid(p, PIDTYPE_PGID);
--
2.39.2
Hello,
kernel test robot noticed "WARNING:suspicious_RCU_usage" on:
commit: 0c940f3d0d4c41567b1957a4e09ad68bdeee2111 ("[PATCH v3] Fixing directly deferencing a __rcu pointer warning")
url: https://github.com/intel-lab-lkp/linux/commits/Abhinav-Singh/Fixing-directly-deferencing-a-__rcu-pointer-warning/20231029-044918
base: https://git.kernel.org/cgit/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH v3] Fixing directly deferencing a __rcu pointer warning
in testcase: boot
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+--------------------------------------------------------+------------+------------+
| | 44c9217272 | 0c940f3d0d |
+--------------------------------------------------------+------------+------------+
| WARNING:suspicious_RCU_usage | 0 | 8 |
| kernel/fork.c:#suspicious_rcu_dereference_check()usage | 0 | 8 |
+--------------------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-lkp/[email protected]
[ 2.386253][ T0] WARNING: suspicious RCU usage
[ 2.386253][ T0] 6.6.0-rc4-00506-g0c940f3d0d4c #7 Not tainted
[ 2.386253][ T0] -----------------------------
[ 2.386253][ T0] kernel/fork.c:2688 suspicious rcu_dereference_check() usage!
[ 2.386253][ T0]
[ 2.386253][ T0] other info that might help us debug this:
[ 2.386253][ T0]
[ 2.386253][ T0] rcu_scheduler_active = 2, debug_locks = 1
[ 2.386253][ T0] 3 locks held by swapper/0:
[ 2.386253][ T0] #0: ffffffff84dbdc50 (cgroup_threadgroup_rwsem){.+.+}-{0:0}, at: cgroup_can_fork (kbuild/src/rand-x86_64-3/kernel/cgroup/cgroup.c:6538)
[ 2.386253][ T0] #1: ffffffff8482f010 (tasklist_lock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2624)
[ 2.386253][ T0] #2: ffffffff84846178 (init_sighand.siglock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2226 kbuild/src/rand-x86_64-3/kernel/fork.c:2643)
[ 2.386253][ T0]
[ 2.386253][ T0] stack backtrace:
[ 2.386253][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc4-00506-g0c940f3d0d4c #7 88a30a6c68427eeed926405592f52ff30544ebdd
[ 2.386253][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.386253][ T0] Call Trace:
[ 2.386253][ T0] <TASK>
[ 2.386253][ T0] copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2688 (discriminator 9))
[ 2.386253][ T0] ? pidfd_prepare (kbuild/src/rand-x86_64-3/kernel/fork.c:2245)
[ 2.386253][ T0] ? acpi_hw_validate_register (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:196)
[ 2.386253][ T0] ? proc_register (kbuild/src/rand-x86_64-3/fs/proc/generic.c:383)
[ 2.386253][ T0] kernel_clone (kbuild/src/rand-x86_64-3/include/linux/random.h:26 kbuild/src/rand-x86_64-3/kernel/fork.c:2903)
[ 2.386253][ T0] ? acpi_hw_register_read (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:489)
[ 2.386253][ T0] ? create_io_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2862)
[ 2.386253][ T0] user_mode_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? kernel_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? acpi_hw_validate_io_request (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwvalid.c:128)
[ 2.386253][ T0] ? rest_init (kbuild/src/rand-x86_64-3/init/main.c:1429)
[ 2.386253][ T0] rest_init (kbuild/src/rand-x86_64-3/init/main.c:691)
[ 2.386253][ T0] arch_call_rest_init+0x13/0x80
[ 2.386253][ T0] start_kernel (kbuild/src/rand-x86_64-3/init/main.c:992 (discriminator 1))
[ 2.386253][ T0] x86_64_start_reservations (kbuild/src/rand-x86_64-3/arch/x86/kernel/head64.c:544)
[ 2.386253][ T0] x86_64_start_kernel (??:?)
[ 2.386253][ T0] secondary_startup_64_no_verify (kbuild/src/rand-x86_64-3/arch/x86/kernel/head_64.S:433)
[ 2.386253][ T0] </TASK>
[ 2.386253][ T0]
[ 2.386253][ T0] =============================
[ 2.386253][ T0] WARNING: suspicious RCU usage
[ 2.386253][ T0] 6.6.0-rc4-00506-g0c940f3d0d4c #7 Not tainted
[ 2.386253][ T0] -----------------------------
[ 2.386253][ T0] kernel/fork.c:2689 suspicious rcu_dereference_check() usage!
[ 2.386253][ T0]
[ 2.386253][ T0] other info that might help us debug this:
[ 2.386253][ T0]
[ 2.386253][ T0] rcu_scheduler_active = 2, debug_locks = 1
[ 2.386253][ T0] 3 locks held by swapper/0:
[ 2.386253][ T0] #0: ffffffff84dbdc50 (cgroup_threadgroup_rwsem){.+.+}-{0:0}, at: cgroup_can_fork (kbuild/src/rand-x86_64-3/kernel/cgroup/cgroup.c:6538)
[ 2.386253][ T0] #1: ffffffff8482f010 (tasklist_lock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2624)
[ 2.386253][ T0] #2: ffffffff84846178 (init_sighand.siglock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2226 kbuild/src/rand-x86_64-3/kernel/fork.c:2643)
[ 2.386253][ T0]
[ 2.386253][ T0] stack backtrace:
[ 2.386253][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc4-00506-g0c940f3d0d4c #7 88a30a6c68427eeed926405592f52ff30544ebdd
[ 2.386253][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.386253][ T0] Call Trace:
[ 2.386253][ T0] <TASK>
[ 2.386253][ T0] copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2689 (discriminator 9))
[ 2.386253][ T0] ? pidfd_prepare (kbuild/src/rand-x86_64-3/kernel/fork.c:2245)
[ 2.386253][ T0] ? acpi_hw_validate_register (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:196)
[ 2.386253][ T0] ? proc_register (kbuild/src/rand-x86_64-3/fs/proc/generic.c:383)
[ 2.386253][ T0] kernel_clone (kbuild/src/rand-x86_64-3/include/linux/random.h:26 kbuild/src/rand-x86_64-3/kernel/fork.c:2903)
[ 2.386253][ T0] ? acpi_hw_register_read (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:489)
[ 2.386253][ T0] ? create_io_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2862)
[ 2.386253][ T0] user_mode_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? kernel_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? acpi_hw_validate_io_request (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwvalid.c:128)
[ 2.386253][ T0] ? rest_init (kbuild/src/rand-x86_64-3/init/main.c:1429)
[ 2.386253][ T0] rest_init (kbuild/src/rand-x86_64-3/init/main.c:691)
[ 2.386253][ T0] arch_call_rest_init+0x13/0x80
[ 2.386253][ T0] start_kernel (kbuild/src/rand-x86_64-3/init/main.c:992 (discriminator 1))
[ 2.386253][ T0] x86_64_start_reservations (kbuild/src/rand-x86_64-3/arch/x86/kernel/head64.c:544)
[ 2.386253][ T0] x86_64_start_kernel (??:?)
[ 2.386253][ T0] secondary_startup_64_no_verify (kbuild/src/rand-x86_64-3/arch/x86/kernel/head_64.S:433)
[ 2.386253][ T0] </TASK>
[ 2.386253][ T0]
[ 2.386253][ T0] =============================
[ 2.386253][ T0] WARNING: suspicious RCU usage
[ 2.386253][ T0] 6.6.0-rc4-00506-g0c940f3d0d4c #7 Not tainted
[ 2.386253][ T0] -----------------------------
[ 2.386253][ T0] kernel/fork.c:2690 suspicious rcu_dereference_check() usage!
[ 2.386253][ T0]
[ 2.386253][ T0] other info that might help us debug this:
[ 2.386253][ T0]
[ 2.386253][ T0] rcu_scheduler_active = 2, debug_locks = 1
[ 2.386253][ T0] 3 locks held by swapper/0:
[ 2.386253][ T0] #0: ffffffff84dbdc50 (cgroup_threadgroup_rwsem){.+.+}-{0:0}, at: cgroup_can_fork (kbuild/src/rand-x86_64-3/kernel/cgroup/cgroup.c:6538)
[ 2.386253][ T0] #1: ffffffff8482f010 (tasklist_lock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2624)
[ 2.386253][ T0] #2: ffffffff84846178 (init_sighand.siglock){....}-{2:2}, at: copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2226 kbuild/src/rand-x86_64-3/kernel/fork.c:2643)
[ 2.386253][ T0]
[ 2.386253][ T0] stack backtrace:
[ 2.386253][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc4-00506-g0c940f3d0d4c #7 88a30a6c68427eeed926405592f52ff30544ebdd
[ 2.386253][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.386253][ T0] Call Trace:
[ 2.386253][ T0] <TASK>
[ 2.386253][ T0] copy_process (kbuild/src/rand-x86_64-3/kernel/fork.c:2690 (discriminator 9))
[ 2.386253][ T0] ? pidfd_prepare (kbuild/src/rand-x86_64-3/kernel/fork.c:2245)
[ 2.386253][ T0] ? acpi_hw_validate_register (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:196)
[ 2.386253][ T0] ? proc_register (kbuild/src/rand-x86_64-3/fs/proc/generic.c:383)
[ 2.386253][ T0] kernel_clone (kbuild/src/rand-x86_64-3/include/linux/random.h:26 kbuild/src/rand-x86_64-3/kernel/fork.c:2903)
[ 2.386253][ T0] ? acpi_hw_register_read (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwregs.c:489)
[ 2.386253][ T0] ? create_io_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2862)
[ 2.386253][ T0] user_mode_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? kernel_thread (kbuild/src/rand-x86_64-3/kernel/fork.c:2971)
[ 2.386253][ T0] ? acpi_hw_validate_io_request (kbuild/src/rand-x86_64-3/drivers/acpi/acpica/hwvalid.c:128)
[ 2.386253][ T0] ? rest_init (kbuild/src/rand-x86_64-3/init/main.c:1429)
[ 2.386253][ T0] rest_init (kbuild/src/rand-x86_64-3/init/main.c:691)
[ 2.386253][ T0] arch_call_rest_init+0x13/0x80
[ 2.386253][ T0] start_kernel (kbuild/src/rand-x86_64-3/init/main.c:992 (discriminator 1))
[ 2.386253][ T0] x86_64_start_reservations (kbuild/src/rand-x86_64-3/arch/x86/kernel/head64.c:544)
[ 2.386253][ T0] x86_64_start_kernel (??:?)
[ 2.386253][ T0] secondary_startup_64_no_verify (kbuild/src/rand-x86_64-3/arch/x86/kernel/head_64.S:433)
[ 2.386253][ T0] </TASK>
[ 2.387851][ T1] RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
[ 2.389031][ T1] RCU Tasks Rude: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
[ 2.390637][ T1] RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
[ 2.391901][ T1] Running RCU-tasks wait API self tests
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20231103/[email protected]
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki