fs/exec.c | 3 +++ include/linux/time64.h | 2 +- kernel/time/posix-stubs.c | 3 ++- kernel/time/time.c | 4 ++-- 4 files changed, 8 insertions(+), 4 deletions(-)
Linus,
Please pull the latest timers/urgent git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-2022-08-13
# HEAD: e362359ace6f87c201531872486ff295df306d13 posix-cpu-timers: Cleanup CPU timers before freeing them during exec
Misc timer fixes:
- fix a potential use-after-free bug in posix timers
- correct a prototype
- address a build warning
Thanks,
Ingo
------------------>
Jiri Slaby (1):
posix-timers: Make do_clock_gettime() static
Thadeu Lima de Souza Cascardo (1):
posix-cpu-timers: Cleanup CPU timers before freeing them during exec
Youngmin Nam (1):
time: Correct the prototype of ns_to_kernel_old_timeval and ns_to_timespec64
fs/exec.c | 3 +++
include/linux/time64.h | 2 +-
kernel/time/posix-stubs.c | 3 ++-
kernel/time/time.c | 4 ++--
4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 5fd73915c62c..f793221f4eb6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1304,6 +1304,9 @@ int begin_new_exec(struct linux_binprm * bprm)
bprm->mm = NULL;
#ifdef CONFIG_POSIX_TIMERS
+ spin_lock_irq(&me->sighand->siglock);
+ posix_cpu_timers_exit(me);
+ spin_unlock_irq(&me->sighand->siglock);
exit_itimers(me);
flush_itimer_signals();
#endif
diff --git a/include/linux/time64.h b/include/linux/time64.h
index 2fb8232cff1d..f1bcea8c124a 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -145,7 +145,7 @@ static inline s64 timespec64_to_ns(const struct timespec64 *ts)
*
* Returns the timespec64 representation of the nsec parameter.
*/
-extern struct timespec64 ns_to_timespec64(const s64 nsec);
+extern struct timespec64 ns_to_timespec64(s64 nsec);
/**
* timespec64_add_ns - Adds nanoseconds to a timespec64
diff --git a/kernel/time/posix-stubs.c b/kernel/time/posix-stubs.c
index fcb3b21d8bdc..90ea5f373e50 100644
--- a/kernel/time/posix-stubs.c
+++ b/kernel/time/posix-stubs.c
@@ -70,7 +70,7 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
return do_sys_settimeofday64(&new_tp, NULL);
}
-int do_clock_gettime(clockid_t which_clock, struct timespec64 *tp)
+static int do_clock_gettime(clockid_t which_clock, struct timespec64 *tp)
{
switch (which_clock) {
case CLOCK_REALTIME:
@@ -90,6 +90,7 @@ int do_clock_gettime(clockid_t which_clock, struct timespec64 *tp)
return 0;
}
+
SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock,
struct __kernel_timespec __user *, tp)
{
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 29923b20e0e4..526257b3727c 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -449,7 +449,7 @@ time64_t mktime64(const unsigned int year0, const unsigned int mon0,
}
EXPORT_SYMBOL(mktime64);
-struct __kernel_old_timeval ns_to_kernel_old_timeval(const s64 nsec)
+struct __kernel_old_timeval ns_to_kernel_old_timeval(s64 nsec)
{
struct timespec64 ts = ns_to_timespec64(nsec);
struct __kernel_old_timeval tv;
@@ -503,7 +503,7 @@ EXPORT_SYMBOL(set_normalized_timespec64);
*
* Returns the timespec64 representation of the nsec parameter.
*/
-struct timespec64 ns_to_timespec64(const s64 nsec)
+struct timespec64 ns_to_timespec64(s64 nsec)
{
struct timespec64 ts = { 0, 0 };
s32 rem;The pull request you sent on Sat, 13 Aug 2022 12:25:51 +0200: > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-2022-08-13 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/f6eb0fed6a3957c0b93e3a00c1ffaad84d4ffc31 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Might wanna hold off on pulling that:
On Sat, Aug 13, 2022 at 12:25:51PM +0200, Ingo Molnar wrote:
> Linus,
>
> Please pull the latest timers/urgent git tree from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers-urgent-2022-08-13
>
> # HEAD: e362359ace6f87c201531872486ff295df306d13 posix-cpu-timers: Cleanup CPU timers before freeing them during exec
...
> Thadeu Lima de Souza Cascardo (1):
> posix-cpu-timers: Cleanup CPU timers before freeing them during exec
...
> diff --git a/fs/exec.c b/fs/exec.c
> index 5fd73915c62c..f793221f4eb6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1304,6 +1304,9 @@ int begin_new_exec(struct linux_binprm * bprm)
> bprm->mm = NULL;
>
> #ifdef CONFIG_POSIX_TIMERS
> + spin_lock_irq(&me->sighand->siglock);
> + posix_cpu_timers_exit(me);
> + spin_unlock_irq(&me->sighand->siglock);
sparse is not happy about this:
https://lore.kernel.org/r/202208140040.MMi4z6Ek-lkp@intel.com
That task_struct.sighand is marked __rcu and thus noderef and sparse
complains:
fs/exec.c:1307:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock
+@@ got struct spinlock [noderef] __rcu * @@
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, Aug 13, 2022 at 9:25 AM Borislav Petkov <bp@alien8.de> wrote:
>
> That task_struct.sighand is marked __rcu and thus noderef and sparse
> complains:
I think that RCU marking is misleading.
Doing a
git grep -e '->sighand'
shows that we basically never treat that as some kind of RCU pointer.
Adding a
grep -i rcu
to the above shows that we have a couple of places that do this
carefully, but they are the exception rather than the rule.
I think the issue is that "current->sighand" is always safe (and that
"me->sighand" is the same thing), and that sighand has RCU-delayed
freeing so that __lock_task_sighand() can safely try to take the lock
of another process' sighand.
And we have no real way to explain to sparse that *some* cases are
fine, others are not and need the sighand lock (after that careful
__lock_task_sighand thing).
Linus
On Sat, Aug 13, 2022 at 01:27:40PM -0700, Linus Torvalds wrote:
> On Sat, Aug 13, 2022 at 9:25 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > That task_struct.sighand is marked __rcu and thus noderef and sparse
> > complains:
>
> I think that RCU marking is misleading.
>
> Doing a
>
> git grep -e '->sighand'
>
> shows that we basically never treat that as some kind of RCU pointer.
>
> Adding a
>
> grep -i rcu
>
> to the above shows that we have a couple of places that do this
> carefully, but they are the exception rather than the rule.
>
> I think the issue is that "current->sighand" is always safe (and that
> "me->sighand" is the same thing), and that sighand has RCU-delayed
> freeing so that __lock_task_sighand() can safely try to take the lock
> of another process' sighand.
>
> And we have no real way to explain to sparse that *some* cases are
> fine, others are not and need the sighand lock (after that careful
> __lock_task_sighand thing).
Sounds to me like that sparse check was not such a good idea in the
first place. Especially since the 0day bot is probably warning about all
those cases where we try to lock ->sighand.
It was added by
913292c97d75 ("sched.h: Annotate sighand_struct with __rcu")
Lemme add the involved parties to Cc.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sun, Aug 14, 2022 at 02:41:54PM +0200, Borislav Petkov wrote:
> On Sat, Aug 13, 2022 at 01:27:40PM -0700, Linus Torvalds wrote:
> > On Sat, Aug 13, 2022 at 9:25 AM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > That task_struct.sighand is marked __rcu and thus noderef and sparse
> > > complains:
> >
> > I think that RCU marking is misleading.
> >
> > Doing a
> >
> > git grep -e '->sighand'
> >
> > shows that we basically never treat that as some kind of RCU pointer.
> >
> > Adding a
> >
> > grep -i rcu
> >
> > to the above shows that we have a couple of places that do this
> > carefully, but they are the exception rather than the rule.
> >
> > I think the issue is that "current->sighand" is always safe (and that
> > "me->sighand" is the same thing), and that sighand has RCU-delayed
> > freeing so that __lock_task_sighand() can safely try to take the lock
> > of another process' sighand.
> >
> > And we have no real way to explain to sparse that *some* cases are
> > fine, others are not and need the sighand lock (after that careful
> > __lock_task_sighand thing).
>
> Sounds to me like that sparse check was not such a good idea in the
> first place. Especially since the 0day bot is probably warning about all
> those cases where we try to lock ->sighand.
>
> It was added by
>
> 913292c97d75 ("sched.h: Annotate sighand_struct with __rcu")
>
> Lemme add the involved parties to Cc.
If it is causing more trouble than it is worth, then I have not objection
to taking a different approach.
Thanx, Paul
On Sun, Aug 14, 2022 at 10:24:45AM -0700, Paul E. McKenney wrote:
> If it is causing more trouble than it is worth, then I have not objection
> to taking a different approach.
Well, pretty much every file I tried in the output of
git grep -e '->sighand'
triggers that sparse warning. For example:
$ make C=1 kernel/signal.o
...
kernel/signal.c:195:31: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:195:31: expected struct spinlock [usertype] *lock
kernel/signal.c:195:31: got struct spinlock [noderef] __rcu *
kernel/signal.c:198:33: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:198:33: expected struct spinlock [usertype] *lock
kernel/signal.c:198:33: got struct spinlock [noderef] __rcu *
kernel/signal.c:480:9: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:480:9: expected struct spinlock [usertype] *lock
kernel/signal.c:480:9: got struct spinlock [noderef] __rcu *
kernel/signal.c:484:34: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:484:34: expected struct spinlock [usertype] *lock
kernel/signal.c:484:34: got struct spinlock [noderef] __rcu *
kernel/signal.c:517:9: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:517:9: expected struct spinlock [usertype] *lock
kernel/signal.c:517:9: got struct spinlock [noderef] __rcu *
kernel/signal.c:520:36: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:520:36: expected struct spinlock [usertype] *lock
kernel/signal.c:520:36: got struct spinlock [noderef] __rcu *
kernel/signal.c:542:53: warning: incorrect type in initializer (different address spaces)
kernel/signal.c:542:53: expected struct k_sigaction *ka
kernel/signal.c:542:53: got struct k_sigaction [noderef] __rcu *
./include/uapi/asm-generic/signal-defs.h:83:29: error: multiple address spaces given
kernel/signal.c:698:33: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:698:33: expected struct spinlock [usertype] *lock
kernel/signal.c:698:33: got struct spinlock [noderef] __rcu *
kernel/signal.c:700:31: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:700:31: expected struct spinlock [usertype] *lock
kernel/signal.c:700:31: got struct spinlock [noderef] __rcu *
kernel/signal.c:1328:9: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:1328:9: expected struct spinlock [usertype] *lock
kernel/signal.c:1328:9: got struct spinlock [noderef] __rcu *
kernel/signal.c:1329:16: warning: incorrect type in assignment (different address spaces)
kernel/signal.c:1329:16: expected struct k_sigaction *action
kernel/signal.c:1329:16: got struct k_sigaction [noderef] __rcu *
kernel/signal.c:1349:34: warning: incorrect type in argument 1 (different address spaces)
kernel/signal.c:1349:34: expected struct spinlock [usertype] *lock
kernel/signal.c:1349:34: got struct spinlock [noderef] __rcu *
kernel/signal.c:1938:36: warning: incorrect type in initializer (different address spaces)
kernel/signal.c:1938:36: expected struct spinlock [usertype] *lock
kernel/signal.c:1938:36: got struct spinlock [noderef] __rcu *
kernel/signal.c:2048:44: warning: cast removes address space '__rcu' of expression
kernel/signal.c:2067:65: warning: too many warnings
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
* Linus Torvalds <torvalds@linux-foundation.org> wrote: > I think the issue is that "current->sighand" is always safe (and that > "me->sighand" is the same thing), and that sighand has RCU-delayed > freeing so that __lock_task_sighand() can safely try to take the lock of > another process' sighand. Yeah - 'me' (== current) here can never go away from under the locking context which is 'me' as well. Thanks, Ingo
© 2016 - 2026 Red Hat, Inc.