From: Joel Fernandes <joel@joelfernandes.org> To: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> Cc: "Eric W. Biederman" <ebiederm@gmail.com>, ebiederm@xmission.com, oleg@redhat.com, christian.brauner@ubuntu.com, guro@fb.com, tj@kernel.org, linux-kernel@vger.kernel.org, paulmck@kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, frextrite@gmail.com Subject: Re: [PATCH] signal.c: Fix sparse warnings Date: Thu, 6 Feb 2020 15:25:11 -0500 [thread overview] Message-ID: <20200206202511.GC36876@google.com> (raw) In-Reply-To: <20200206110051.GA4531@madhuparna-HP-Notebook> On Thu, Feb 06, 2020 at 04:30:51PM +0530, Madhuparna Bhowmik wrote: > On Wed, Feb 05, 2020 at 04:59:52PM -0600, Eric W. Biederman wrote: > > madhuparnabhowmik10@gmail.com writes: > > > > > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > > > > > This patch fixes the following two sparse warnings caused due to > > > accessing RCU protected pointer tsk->parent without rcu primitives. > > > > > > kernel/signal.c:1948:65: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1948:65: expected struct task_struct *tsk > > > kernel/signal.c:1948:65: got struct task_struct [noderef] <asn:4> *parent > > > kernel/signal.c:1949:40: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1949:40: expected void const volatile *p > > > kernel/signal.c:1949:40: got struct cred const [noderef] <asn:4> *[noderef] <asn:4> * > > > kernel/signal.c:1949:40: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1949:40: expected void const volatile *p > > > kernel/signal.c:1949:40: got struct cred const [noderef] <asn:4> *[noderef] <asn:4> * > > > > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > > --- > > > kernel/signal.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index 9ad8dea93dbb..8227058ea8c4 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > > @@ -1945,8 +1945,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > > * correct to rely on this > > > */ > > > rcu_read_lock(); > > > - info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk->parent)); > > > - info.si_uid = from_kuid_munged(task_cred_xxx(tsk->parent, user_ns), > > > + info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(rcu_dereference(tsk->parent))); > > > + info.si_uid = from_kuid_munged(task_cred_xxx(rcu_dereference(tsk->parent), user_ns), > > > task_uid(tsk)); > > > rcu_read_unlock(); > > > > > > Still wrong because that access fundamentally depends upon the > > task_list_lock no the rcu_read_lock. Things need to be consistent for > > longer than the rcu_read_lock is held. > > > Okay, then how about something like rcu_dereference_protected(tsk->parent, lockdep_is_held(&tasklist_lock))? > Let me know if this looks fine to you. But then there are several other ->parent accesses in the function. What about something like the following? It removes the confusion Eric is referring to and fixes the sparse errors you mentioned. Thoughts? ---8<----------------------- diff --git a/kernel/signal.c b/kernel/signal.c index bcd46f547db39..92f0b7bf70bf3 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1909,6 +1909,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) struct sighand_struct *psig; bool autoreap = false; u64 utime, stime; + struct task_struct *tsk_parent; BUG_ON(sig == -1); @@ -1918,6 +1919,9 @@ bool do_notify_parent(struct task_struct *tsk, int sig) BUG_ON(!tsk->ptrace && (tsk->group_leader != tsk || !thread_group_empty(tsk))); + tsk_parent = rcu_dereference_protected(tsk->parent, + lockdep_is_held(&tasklist_lock)); + /* Wake up all pidfd waiters */ do_notify_pidfd(tsk); @@ -1926,7 +1930,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) * This is only possible if parent == real_parent. * Check if it has changed security domain. */ - if (tsk->parent_exec_id != tsk->parent->self_exec_id) + if (tsk->parent_exec_id != tsk_parent->self_exec_id) sig = SIGCHLD; } @@ -1945,8 +1949,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) * correct to rely on this */ rcu_read_lock(); - info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk->parent)); - info.si_uid = from_kuid_munged(task_cred_xxx(tsk->parent, user_ns), + info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk_parent)); + info.si_uid = from_kuid_munged(task_cred_xxx(tsk_parent, user_ns), task_uid(tsk)); rcu_read_unlock(); @@ -1964,7 +1968,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) info.si_status = tsk->exit_code >> 8; } - psig = tsk->parent->sighand; + psig = tsk_parent->sighand; spin_lock_irqsave(&psig->siglock, flags); if (!tsk->ptrace && sig == SIGCHLD && (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || @@ -1989,8 +1993,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) sig = 0; } if (valid_signal(sig) && sig) - __group_send_sig_info(sig, &info, tsk->parent); - __wake_up_parent(tsk, tsk->parent); + __group_send_sig_info(sig, &info, tsk_parent); + __wake_up_parent(tsk, tsk_parent); spin_unlock_irqrestore(&psig->siglock, flags); return autoreap; -- 2.25.0.341.g760bfbb309-goog
WARNING: multiple messages have this Message-ID (diff)
From: Joel Fernandes <joel@joelfernandes.org> To: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> Cc: paulmck@kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, oleg@redhat.com, linux-kernel@vger.kernel.org, ebiederm@xmission.com, tj@kernel.org, christian.brauner@ubuntu.com, "Eric W. Biederman" <ebiederm@gmail.com>, guro@fb.com Subject: Re: [Linux-kernel-mentees] [PATCH] signal.c: Fix sparse warnings Date: Thu, 6 Feb 2020 15:25:11 -0500 [thread overview] Message-ID: <20200206202511.GC36876@google.com> (raw) In-Reply-To: <20200206110051.GA4531@madhuparna-HP-Notebook> On Thu, Feb 06, 2020 at 04:30:51PM +0530, Madhuparna Bhowmik wrote: > On Wed, Feb 05, 2020 at 04:59:52PM -0600, Eric W. Biederman wrote: > > madhuparnabhowmik10@gmail.com writes: > > > > > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > > > > > This patch fixes the following two sparse warnings caused due to > > > accessing RCU protected pointer tsk->parent without rcu primitives. > > > > > > kernel/signal.c:1948:65: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1948:65: expected struct task_struct *tsk > > > kernel/signal.c:1948:65: got struct task_struct [noderef] <asn:4> *parent > > > kernel/signal.c:1949:40: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1949:40: expected void const volatile *p > > > kernel/signal.c:1949:40: got struct cred const [noderef] <asn:4> *[noderef] <asn:4> * > > > kernel/signal.c:1949:40: warning: incorrect type in argument 1 (different address spaces) > > > kernel/signal.c:1949:40: expected void const volatile *p > > > kernel/signal.c:1949:40: got struct cred const [noderef] <asn:4> *[noderef] <asn:4> * > > > > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com> > > > --- > > > kernel/signal.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index 9ad8dea93dbb..8227058ea8c4 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > > @@ -1945,8 +1945,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > > * correct to rely on this > > > */ > > > rcu_read_lock(); > > > - info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk->parent)); > > > - info.si_uid = from_kuid_munged(task_cred_xxx(tsk->parent, user_ns), > > > + info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(rcu_dereference(tsk->parent))); > > > + info.si_uid = from_kuid_munged(task_cred_xxx(rcu_dereference(tsk->parent), user_ns), > > > task_uid(tsk)); > > > rcu_read_unlock(); > > > > > > Still wrong because that access fundamentally depends upon the > > task_list_lock no the rcu_read_lock. Things need to be consistent for > > longer than the rcu_read_lock is held. > > > Okay, then how about something like rcu_dereference_protected(tsk->parent, lockdep_is_held(&tasklist_lock))? > Let me know if this looks fine to you. But then there are several other ->parent accesses in the function. What about something like the following? It removes the confusion Eric is referring to and fixes the sparse errors you mentioned. Thoughts? ---8<----------------------- diff --git a/kernel/signal.c b/kernel/signal.c index bcd46f547db39..92f0b7bf70bf3 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1909,6 +1909,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) struct sighand_struct *psig; bool autoreap = false; u64 utime, stime; + struct task_struct *tsk_parent; BUG_ON(sig == -1); @@ -1918,6 +1919,9 @@ bool do_notify_parent(struct task_struct *tsk, int sig) BUG_ON(!tsk->ptrace && (tsk->group_leader != tsk || !thread_group_empty(tsk))); + tsk_parent = rcu_dereference_protected(tsk->parent, + lockdep_is_held(&tasklist_lock)); + /* Wake up all pidfd waiters */ do_notify_pidfd(tsk); @@ -1926,7 +1930,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) * This is only possible if parent == real_parent. * Check if it has changed security domain. */ - if (tsk->parent_exec_id != tsk->parent->self_exec_id) + if (tsk->parent_exec_id != tsk_parent->self_exec_id) sig = SIGCHLD; } @@ -1945,8 +1949,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) * correct to rely on this */ rcu_read_lock(); - info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk->parent)); - info.si_uid = from_kuid_munged(task_cred_xxx(tsk->parent, user_ns), + info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk_parent)); + info.si_uid = from_kuid_munged(task_cred_xxx(tsk_parent, user_ns), task_uid(tsk)); rcu_read_unlock(); @@ -1964,7 +1968,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) info.si_status = tsk->exit_code >> 8; } - psig = tsk->parent->sighand; + psig = tsk_parent->sighand; spin_lock_irqsave(&psig->siglock, flags); if (!tsk->ptrace && sig == SIGCHLD && (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || @@ -1989,8 +1993,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) sig = 0; } if (valid_signal(sig) && sig) - __group_send_sig_info(sig, &info, tsk->parent); - __wake_up_parent(tsk, tsk->parent); + __group_send_sig_info(sig, &info, tsk_parent); + __wake_up_parent(tsk, tsk_parent); spin_unlock_irqrestore(&psig->siglock, flags); return autoreap; -- 2.25.0.341.g760bfbb309-goog _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-02-06 20:25 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-05 17:24 [PATCH] signal.c: Fix sparse warnings madhuparnabhowmik10 2020-02-05 17:24 ` [Linux-kernel-mentees] " madhuparnabhowmik10 2020-02-05 22:59 ` Eric W. Biederman 2020-02-05 22:59 ` [Linux-kernel-mentees] " Eric W. Biederman 2020-02-06 11:00 ` Madhuparna Bhowmik 2020-02-06 11:00 ` [Linux-kernel-mentees] " Madhuparna Bhowmik 2020-02-06 20:25 ` Joel Fernandes [this message] 2020-02-06 20:25 ` Joel Fernandes 2020-02-07 15:04 ` Madhuparna Bhowmik 2020-02-07 15:04 ` [Linux-kernel-mentees] " Madhuparna Bhowmik -- strict thread matches above, loose matches on Subject: below -- 2020-02-05 16:23 madhuparnabhowmik10 2020-02-05 16:51 ` Amol Grover 2020-02-05 17:09 ` Madhuparna Bhowmik
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=20200206202511.GC36876@google.com \ --to=joel@joelfernandes.org \ --cc=christian.brauner@ubuntu.com \ --cc=ebiederm@gmail.com \ --cc=ebiederm@xmission.com \ --cc=frextrite@gmail.com \ --cc=guro@fb.com \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=madhuparnabhowmik10@gmail.com \ --cc=oleg@redhat.com \ --cc=paulmck@kernel.org \ --cc=tj@kernel.org \ /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.