* [PATCH] signal.c: Fix sparse warnings
@ 2020-02-05 16:23 madhuparnabhowmik10
2020-02-05 16:51 ` Amol Grover
0 siblings, 1 reply; 8+ messages in thread
From: madhuparnabhowmik10 @ 2020-02-05 16:23 UTC (permalink / raw)
To: ebiederm, oleg, christian.brauner, guro, tj
Cc: linux-kernel, paulmck, joel, linux-kernel-mentees, frextrite,
Madhuparna Bhowmik
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..3d59e5652d94 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_access_pointer(tsk->parent)));
+ info.si_uid = from_kuid_munged(task_cred_xxx(rcu_access_pointer(tsk->parent), user_ns),
task_uid(tsk));
rcu_read_unlock();
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-05 16:23 [PATCH] signal.c: Fix sparse warnings madhuparnabhowmik10
@ 2020-02-05 16:51 ` Amol Grover
2020-02-05 17:09 ` Madhuparna Bhowmik
0 siblings, 1 reply; 8+ messages in thread
From: Amol Grover @ 2020-02-05 16:51 UTC (permalink / raw)
To: madhuparnabhowmik10
Cc: ebiederm, oleg, christian.brauner, guro, tj, linux-kernel,
paulmck, joel, linux-kernel-mentees
On Wed, Feb 05, 2020 at 09:53:19PM +0530, madhuparnabhowmik10@gmail.com wrote:
> 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..3d59e5652d94 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_access_pointer(tsk->parent)));
> + info.si_uid = from_kuid_munged(task_cred_xxx(rcu_access_pointer(tsk->parent), user_ns),
Shouldn't rcu_dereference() OR rcu_dereference_check() be better suited
here? Since, rcu_access_pointer() omits all lockdep checks.
Thanks
Amol
> task_uid(tsk));
> rcu_read_unlock();
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-05 16:51 ` Amol Grover
@ 2020-02-05 17:09 ` Madhuparna Bhowmik
0 siblings, 0 replies; 8+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-05 17:09 UTC (permalink / raw)
To: Amol Grover
Cc: madhuparnabhowmik10, ebiederm, oleg, christian.brauner, guro, tj,
linux-kernel, paulmck, joel, linux-kernel-mentees
On Wed, Feb 05, 2020 at 10:21:38PM +0530, Amol Grover wrote:
> On Wed, Feb 05, 2020 at 09:53:19PM +0530, madhuparnabhowmik10@gmail.com wrote:
> > 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..3d59e5652d94 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_access_pointer(tsk->parent)));
> > + info.si_uid = from_kuid_munged(task_cred_xxx(rcu_access_pointer(tsk->parent), user_ns),
>
> Shouldn't rcu_dereference() OR rcu_dereference_check() be better suited
> here? Since, rcu_access_pointer() omits all lockdep checks.
>
I used rcu_access_pointer() because I thought the pointer is not
dereferenced. But it is dereferenced in task_pid() and task_cred_xxx().
Thank you for pointing out, I will send the updated patch.
Thanks,
Madhuparna
> Thanks
> Amol
>
> > task_uid(tsk));
> > rcu_read_unlock();
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-06 20:25 ` Joel Fernandes
@ 2020-02-07 15:04 ` Madhuparna Bhowmik
0 siblings, 0 replies; 8+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-07 15:04 UTC (permalink / raw)
To: Joel Fernandes
Cc: Madhuparna Bhowmik, Eric W. Biederman, ebiederm, oleg,
christian.brauner, guro, tj, linux-kernel, paulmck,
linux-kernel-mentees, frextrite
On Thu, Feb 06, 2020 at 03:25:11PM -0500, Joel Fernandes wrote:
> 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?
>
Yes, I agree this should remove the confusion.
Thank you,
Madhuparna
> ---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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-06 11:00 ` Madhuparna Bhowmik
@ 2020-02-06 20:25 ` Joel Fernandes
2020-02-07 15:04 ` Madhuparna Bhowmik
0 siblings, 1 reply; 8+ messages in thread
From: Joel Fernandes @ 2020-02-06 20:25 UTC (permalink / raw)
To: Madhuparna Bhowmik
Cc: Eric W. Biederman, ebiederm, oleg, christian.brauner, guro, tj,
linux-kernel, paulmck, linux-kernel-mentees, frextrite
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-05 22:59 ` Eric W. Biederman
@ 2020-02-06 11:00 ` Madhuparna Bhowmik
2020-02-06 20:25 ` Joel Fernandes
0 siblings, 1 reply; 8+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-06 11:00 UTC (permalink / raw)
To: Eric W. Biederman
Cc: madhuparnabhowmik10, ebiederm, oleg, christian.brauner, guro, tj,
linux-kernel, paulmck, joel, linux-kernel-mentees, frextrite
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.
Thank you,
Madhuparna
> This patch makes sparse happy and confuses programmers who are trying to
> read the code.
>
>
> Eric
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] signal.c: Fix sparse warnings
2020-02-05 17:24 madhuparnabhowmik10
@ 2020-02-05 22:59 ` Eric W. Biederman
2020-02-06 11:00 ` Madhuparna Bhowmik
0 siblings, 1 reply; 8+ messages in thread
From: Eric W. Biederman @ 2020-02-05 22:59 UTC (permalink / raw)
To: madhuparnabhowmik10
Cc: ebiederm, oleg, christian.brauner, guro, tj, linux-kernel,
paulmck, joel, linux-kernel-mentees, frextrite
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.
This patch makes sparse happy and confuses programmers who are trying to
read the code.
Eric
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] signal.c: Fix sparse warnings
@ 2020-02-05 17:24 madhuparnabhowmik10
2020-02-05 22:59 ` Eric W. Biederman
0 siblings, 1 reply; 8+ messages in thread
From: madhuparnabhowmik10 @ 2020-02-05 17:24 UTC (permalink / raw)
To: ebiederm, oleg, christian.brauner, guro, tj
Cc: linux-kernel, paulmck, joel, linux-kernel-mentees, frextrite,
Madhuparna Bhowmik
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();
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-07 15:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 16:23 [PATCH] signal.c: Fix sparse warnings madhuparnabhowmik10
2020-02-05 16:51 ` Amol Grover
2020-02-05 17:09 ` Madhuparna Bhowmik
2020-02-05 17:24 madhuparnabhowmik10
2020-02-05 22:59 ` Eric W. Biederman
2020-02-06 11:00 ` Madhuparna Bhowmik
2020-02-06 20:25 ` Joel Fernandes
2020-02-07 15:04 ` Madhuparna Bhowmik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).