[PATCH] signal: Remove the __rcu annotation from task_struct->sighand

[Bradley Morgan]

sighand is not protected by RCU. It is protected by sighand->siglock,
and by tasklist_lock on the exit path, so the __rcu annotation on
task_struct->sighand describes neither, and sparse complains about
every direct ->siglock use because of it. That is exactly what the
kbuild robot is reporting now for posix-cpu-timers.

What actually keeps lock_task_sighand() safe is SLAB_TYPESAFE_BY_RCU
plus the siglock recheck, not this annotation. Oleg suggested just
dropping the annotation, so that is what this does, and the few rcu_*
accessors
get switched over to the READ_ONCE/WRITE_ONCE/smp_store_release they
already expand to.

This cannot really be split... with the annotation gone the leftover
rcu_* sites are sparse errors, and converting them first while the
annotation stays leaves warnings, so there is no file by file split
that keeps every commit clean. With CONFIG_PROVE_LOCKING off the
generated code is identical, with it on the only difference is the RCU
lockdep assertions that came with the removed accessors.

Reported-by: kernel test robot <lkp@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-kbuild-all/202608130320.o3cNs7Lr-lkp@xxxxxxxxx/
Suggested-by: Oleg Nesterov <oleg@xxxxxxxxxx>
Link: https://lore.kernel.org/r/ak5wjJSOyqpXKBdj@xxxxxxxxxx
Fixes: 913292c97d75 ("sched.h: Annotate sighand_struct with __rcu")
Signed-off-by: Bradley Morgan <include@xxxxxxxxx>
---
 fs/exec.c                | 7 ++++ +-
 include/linux/sched.h    | 2 +-
 kernel/exit.c            | 3 +--
 kernel/fork.c            | 2 +-
 kernel/signal.c          | 6 +++---
 net/sunrpc/svc.c         | 2 +-
 security/selinux/hooks.c | 4 ++--
 7 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a14f28b15607..9e3d703329ab 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1078,7 +1078,12 @@ static int unshare_sighand(struct task_struct *me)
 		spin_lock(&oldsighand->siglock);
 		memcpy(newsighand->action, oldsighand->action,
 		       sizeof(newsighand->action));
-		rcu_assign_pointer(me->sighand, newsighand);
+		/*
+		 * Pairs with the READ_ONCE() in lock_task_sighand(): the
+		 * ->action copy above has to be visible before anyone can
+		 * see the new ->sighand.
+		 */
+		smp_store_release(&me->sighand, newsighand);
 		spin_unlock(&oldsighand->siglock);
 		write_unlock_irq(&tasklist_lock);
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 499943987c1a..ef5bcc8caf74 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1216,7 +1216,7 @@ struct task_struct {
 
 	/* Signal handlers: */
 	struct signal_struct		*signal;
-	struct sighand_struct __rcu		*sighand;
+	struct sighand_struct		*sighand;
 	sigset_t			blocked;
 	sigset_t			real_blocked;
 	/* Restored if set_restore_sigmask() was used: */
diff --git a/kernel/exit.c b/kernel/exit.c
index 182c06671c78..45a9354a6a77 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -163,8 +163,7 @@ static void __exit_signal(struct release_task_post *post, struct task_struct *ts
 	struct tty_struct *tty;
 	u64 utime, stime;
 
-	sighand = rcu_dereference_check(tsk->sighand,
-					lockdep_tasklist_lock_is_held());
+	sighand = READ_ONCE(tsk->sighand);
 	spin_lock(&sighand->siglock);
 
 #ifdef CONFIG_POSIX_TIMERS
diff --git a/kernel/fork.c b/kernel/fork.c
index 86b6351ed120..202915e85b4e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1702,7 +1702,7 @@ static int copy_sighand(u64 clone_flags, struct task_struct *tsk)
 		return 0;
 	}
 	sig = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
-	RCU_INIT_POINTER(tsk->sighand, sig);
+	WRITE_ONCE(tsk->sighand, sig);
 	if (!sig)
 		return -ENOMEM;
 
diff --git a/kernel/signal.c b/kernel/signal.c
index a5e15bf09d31..04cfefe86208 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1354,7 +1354,7 @@ struct sighand_struct *lock_task_sighand(struct task_struct *tsk,
 
 	rcu_read_lock();
 	for (;;) {
-		sighand = rcu_dereference(tsk->sighand);
+		sighand = READ_ONCE(tsk->sighand);
 		if (unlikely(sighand == NULL)) {
 			/*
 			 * Pairs with the smp_store_release() in
@@ -1378,7 +1378,7 @@ struct sighand_struct *lock_task_sighand(struct task_struct *tsk,
 		 * must see ->sighand == NULL.
 		 */
 		spin_lock_irqsave(&sighand->siglock, *flags);
-		if (likely(sighand == rcu_access_pointer(tsk->sighand)))
+		if (likely(sighand == READ_ONCE(tsk->sighand)))
 			break;
 		spin_unlock_irqrestore(&sighand->siglock, *flags);
 	}
@@ -1393,7 +1393,7 @@ void lockdep_assert_task_sighand_held(struct task_struct *task)
 	struct sighand_struct *sighand;
 
 	rcu_read_lock();
-	sighand = rcu_dereference(task->sighand);
+	sighand = READ_ONCE(task->sighand);
 	if (sighand)
 		lockdep_assert_held(&sighand->siglock);
 	else
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 05c3e6e2f659..7c681b9f829b 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1276,7 +1276,7 @@ static void svc_unregister(const struct svc_serv *serv, struct net *net)
 	}
 
 	rcu_read_lock();
-	sighand = rcu_dereference(current->sighand);
+	sighand = READ_ONCE(current->sighand);
 	spin_lock_irqsave(&sighand->siglock, flags);
 	recalc_sigpending();
 	spin_unlock_irqrestore(&sighand->siglock, flags);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 035aaf113d1d..d6423a05ca6c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2586,7 +2586,7 @@ static void selinux_bprm_committed_creds(const struct linux_binprm *bprm)
 	if (rc) {
 		clear_itimer();
 
-		spin_lock_irq(&unrcu_pointer(current->sighand)->siglock);
+		spin_lock_irq(&current->sighand->siglock);
 		if (!fatal_signal_pending(current)) {
 			flush_sigqueue(&current->pending);
 			flush_sigqueue(&current->signal->shared_pending);
@@ -2594,7 +2594,7 @@ static void selinux_bprm_committed_creds(const struct linux_binprm *bprm)
 			sigemptyset(&current->blocked);
 			recalc_sigpending();
 		}
-		spin_unlock_irq(&unrcu_pointer(current->sighand)->siglock);
+		spin_unlock_irq(&current->sighand->siglock);
 	}
 
 	/* Wake up the parent if it is waiting so that it can recheck
-- 
2.47.3


I'm feeling it's a bit, broad, but sparse still cries if I just remove the
annotation from sched.h.. honestly this is odd.

Thanks!