From: Peter Zijlstra <peterz@infradead.org> To: Abhinav Singh <singhabhinav9051571833@gmail.com> Cc: brauner@kernel.org, mjguzik@gmail.com, mst@redhat.com, linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org, npiggin@gmail.com, Oleg Nesterov <oleg@redhat.com>, dhowells@redhat.com, mathieu.desnoyers@efficios.com, shakeelb@google.com, akpm@linux-foundation.org, surenb@google.com, michael.christie@oracle.com Subject: Re: [PATCH] Fixing directly deferencing a __rcu pointer warning Date: Thu, 26 Oct 2023 17:03:26 +0200 [thread overview] Message-ID: <20231026150326.GA33303@noisy.programming.kicks-ass.net> (raw) In-Reply-To: <20231026122748.359162-1-singhabhinav9051571833@gmail.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 <singhabhinav9051571833@gmail.com> > --- > 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. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org> To: Abhinav Singh <singhabhinav9051571833@gmail.com> Cc: akpm@linux-foundation.org, brauner@kernel.org, surenb@google.com, mst@redhat.com, michael.christie@oracle.com, mathieu.desnoyers@efficios.com, mjguzik@gmail.com, npiggin@gmail.com, shakeelb@google.com, linux-kernel@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, Oleg Nesterov <oleg@redhat.com>, dhowells@redhat.com Subject: Re: [PATCH] Fixing directly deferencing a __rcu pointer warning Date: Thu, 26 Oct 2023 17:03:26 +0200 [thread overview] Message-ID: <20231026150326.GA33303@noisy.programming.kicks-ass.net> (raw) In-Reply-To: <20231026122748.359162-1-singhabhinav9051571833@gmail.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 <singhabhinav9051571833@gmail.com> > --- > 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.
next prev parent reply other threads:[~2023-10-26 15:03 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-25 22:28 [PATCH] Fixing warning of directly dereferencing __rcu tagged Abhinav Singh 2023-10-25 22:28 ` Abhinav Singh 2023-10-25 22:38 ` Andrew Morton 2023-10-25 22:38 ` Andrew Morton 2023-10-25 23:27 ` Abhinav Singh 2023-10-25 23:27 ` Abhinav Singh 2023-10-25 23:50 ` Andrew Morton 2023-10-25 23:50 ` Andrew Morton 2023-10-26 12:16 ` [PATCH v2] Fixing directly deferencing a __rcu pointer warning Abhinav Singh 2023-10-26 12:16 ` Abhinav Singh 2023-10-26 13:14 ` Michael S. Tsirkin 2023-10-26 13:14 ` Michael S. Tsirkin 2023-10-26 14:06 ` Mateusz Guzik 2023-10-26 14:06 ` Mateusz Guzik 2023-10-26 14:51 ` Mathieu Desnoyers 2023-10-26 14:51 ` Mathieu Desnoyers 2023-10-26 15:07 ` Mateusz Guzik 2023-10-26 15:07 ` Mateusz Guzik 2023-10-26 15:17 ` Michael S. Tsirkin 2023-10-26 15:17 ` Michael S. Tsirkin 2023-10-26 15:37 ` Abhinav Singh 2023-10-26 15:37 ` Abhinav Singh 2023-10-26 15:47 ` Michael S. Tsirkin 2023-10-26 15:47 ` Michael S. Tsirkin 2023-10-27 6:37 ` [PATCH v3] " Abhinav Singh 2023-10-27 6:37 ` Abhinav Singh 2023-10-27 6:41 ` Abhinav Singh 2023-10-27 6:41 ` Abhinav Singh 2023-10-27 7:00 ` Michael S. Tsirkin 2023-10-27 7:00 ` Michael S. Tsirkin 2023-10-27 6:50 ` Michael S. Tsirkin 2023-10-27 6:50 ` Michael S. Tsirkin 2023-10-26 12:18 ` [PATCH] Fixing warning of directly dereferencing __rcu tagged Abhinav Singh 2023-10-26 12:18 ` Abhinav Singh 2023-10-26 12:27 ` [PATCH] Fixing directly deferencing a __rcu pointer warning Abhinav Singh 2023-10-26 12:27 ` Abhinav Singh 2023-10-26 15:03 ` Peter Zijlstra [this message] 2023-10-26 15:03 ` Peter Zijlstra 2023-10-27 12:22 ` Oleg Nesterov 2023-10-27 12:22 ` Oleg Nesterov 2023-10-28 10:22 ` [PATCH v3] " Abhinav Singh 2023-10-28 10:22 ` Abhinav Singh 2023-10-28 10:31 ` Abhinav Singh 2023-10-28 10:31 ` Abhinav Singh 2023-10-28 12:20 ` Michael S. Tsirkin 2023-10-28 12:20 ` Michael S. Tsirkin 2023-10-28 20:43 ` Abhinav Singh 2023-10-28 20:43 ` Abhinav Singh 2023-10-28 20:46 ` Abhinav Singh 2023-10-28 20:46 ` Abhinav Singh 2023-11-03 6:30 ` kernel test robot 2023-11-03 6:30 ` kernel test robot 2023-11-12 19:30 ` [PATCH v4] " Abhinav Singh 2023-11-12 19:56 ` Mathieu Desnoyers 2023-11-12 19:58 ` Peter Zijlstra 2023-11-12 20:18 ` Abhinav Singh
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20231026150326.GA33303@noisy.programming.kicks-ass.net \ --to=peterz@infradead.org \ --cc=akpm@linux-foundation.org \ --cc=brauner@kernel.org \ --cc=dhowells@redhat.com \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.desnoyers@efficios.com \ --cc=michael.christie@oracle.com \ --cc=mjguzik@gmail.com \ --cc=mst@redhat.com \ --cc=npiggin@gmail.com \ --cc=oleg@redhat.com \ --cc=shakeelb@google.com \ --cc=singhabhinav9051571833@gmail.com \ --cc=surenb@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.