Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 6 Feb 2020 15:25:11 -0500
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
Message-ID: <20200206202511.GC36876@google.com>
References: <20200205172437.10113-1-madhuparnabhowmik10@gmail.com>
 <87wo90myhj.fsf@x220.int.ebiederm.org>
 <20200206110051.GA4531@madhuparna-HP-Notebook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200206110051.GA4531@madhuparna-HP-Notebook>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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