The use of C11 is mainstream in the kernel [1]. There was some
confusion on volatile and signal handlers in [2]. Switch to using
stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
Yan <[email protected]> for the suggestions.
[1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Ian Rogers (8):
perf build: Update to C standard to gnu11
perf record: Use sig_atomic_t for signal handlers
perf daemon: Use sig_atomic_t to avoid UB
perf ftrace: Use sig_atomic_t to avoid UB
perf session: Change type to avoid UB
perf stat: Use sig_atomic_t to avoid UB
perf top: Use sig_atomic_t to avoid UB
perf trace: Use sig_atomic_t to avoid UB
tools/perf/Makefile.config | 2 +-
tools/perf/builtin-daemon.c | 3 ++-
tools/perf/builtin-ftrace.c | 4 ++--
tools/perf/builtin-record.c | 9 +++++----
tools/perf/builtin-stat.c | 9 +++++----
tools/perf/builtin-top.c | 4 ++--
tools/perf/builtin-trace.c | 4 ++--
tools/perf/util/session.c | 3 ++-
8 files changed, 21 insertions(+), 17 deletions(-)
--
2.38.0.135.g90850a2211-goog
Use sig_atomic_t for variables written/accessed in signal
handlers. This is undefined behavior as per:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-top.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 4b3ff7687236..bb5bd241246b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -87,8 +87,8 @@
#include <linux/ctype.h>
#include <perf/mmap.h>
-static volatile int done;
-static volatile int resize;
+static volatile sig_atomic_t done;
+static volatile sig_atomic_t resize;
#define HEADER_LINE_NR 5
--
2.38.0.135.g90850a2211-goog
Use sig_atomic_t for variables written/accessed in signal
handlers. This is undefined behavior as per:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-trace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d3c757769b96..72991528687e 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1535,8 +1535,8 @@ static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
}
static pid_t workload_pid = -1;
-static bool done = false;
-static bool interrupted = false;
+static volatile sig_atomic_t done = false;
+static volatile sig_atomic_t interrupted = false;
static void sighandler_interrupt(int sig __maybe_unused)
{
--
2.38.0.135.g90850a2211-goog
Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> The use of C11 is mainstream in the kernel [1]. There was some
> confusion on volatile and signal handlers in [2]. Switch to using
> stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> Yan <[email protected]> for the suggestions.
>
> [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> [2] https://lore.kernel.org/lkml/[email protected]/
> [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
I think I'll apply this to perf/core, i.e. for 6.3, ok?
- Arnaldo
>
> Ian Rogers (8):
> perf build: Update to C standard to gnu11
> perf record: Use sig_atomic_t for signal handlers
> perf daemon: Use sig_atomic_t to avoid UB
> perf ftrace: Use sig_atomic_t to avoid UB
> perf session: Change type to avoid UB
> perf stat: Use sig_atomic_t to avoid UB
> perf top: Use sig_atomic_t to avoid UB
> perf trace: Use sig_atomic_t to avoid UB
>
> tools/perf/Makefile.config | 2 +-
> tools/perf/builtin-daemon.c | 3 ++-
> tools/perf/builtin-ftrace.c | 4 ++--
> tools/perf/builtin-record.c | 9 +++++----
> tools/perf/builtin-stat.c | 9 +++++----
> tools/perf/builtin-top.c | 4 ++--
> tools/perf/builtin-trace.c | 4 ++--
> tools/perf/util/session.c | 3 ++-
> 8 files changed, 21 insertions(+), 17 deletions(-)
>
> --
> 2.38.0.135.g90850a2211-goog
--
- Arnaldo
This removes undefined behavior as described in:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Suggested-by: Leo Yan <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-record.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index e128b855ddde..5332b9c05f28 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -61,6 +61,7 @@
#include <locale.h>
#include <poll.h>
#include <pthread.h>
+#include <stdatomic.h>
#include <unistd.h>
#ifndef HAVE_GETTID
#include <syscall.h>
@@ -646,10 +647,10 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
return record__write(rec, map, bf, size);
}
-static volatile int signr = -1;
-static volatile int child_finished;
+static volatile sig_atomic_t signr = -1;
+static volatile sig_atomic_t child_finished;
#ifdef HAVE_EVENTFD_SUPPORT
-static volatile int done_fd = -1;
+static volatile sig_atomic_t done_fd = -1;
#endif
static void sig_handler(int sig)
@@ -1926,7 +1927,7 @@ static void record__read_lost_samples(struct record *rec)
}
-static volatile int workload_exec_errno;
+static volatile sig_atomic_t workload_exec_errno;
/*
* evlist__prepare_workload will send a SIGUSR1
--
2.38.0.135.g90850a2211-goog
session_done is written to inside the signal handler of perf report
and script. Switch its type to avoid undefined behavior.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/session.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 1a4f10de29ff..ba52cc4092dc 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <errno.h>
+#include <stdatomic.h>
#include <inttypes.h>
#include <linux/err.h>
#include <linux/kernel.h>
@@ -2022,7 +2023,7 @@ static int perf_session__flush_thread_stacks(struct perf_session *session)
NULL);
}
-volatile int session_done;
+volatile sig_atomic_t session_done;
static int __perf_session__process_decomp_events(struct perf_session *session);
--
2.38.0.135.g90850a2211-goog
Use sig_atomic_t for a variable written to in a signal handler and
read elsewhere. This is undefined behavior as per:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-ftrace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 7de07bb16d23..d7fe00f66b83 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -36,8 +36,8 @@
#define DEFAULT_TRACER "function_graph"
-static volatile int workload_exec_errno;
-static bool done;
+static volatile sig_atomic_t workload_exec_errno;
+static volatile sig_atomic_t done;
static void sig_handler(int sig __maybe_unused)
{
--
2.38.0.135.g90850a2211-goog
On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > The use of C11 is mainstream in the kernel [1]. There was some
> > confusion on volatile and signal handlers in [2]. Switch to using
> > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > Yan <[email protected]> for the suggestions.
> >
> > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > [2] https://lore.kernel.org/lkml/[email protected]/
> > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
>
> I think I'll apply this to perf/core, i.e. for 6.3, ok?
Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
this and to the iterators (from C11) that can be done.
Thanks,
Ian
> - Arnaldo
>
> >
> > Ian Rogers (8):
> > perf build: Update to C standard to gnu11
> > perf record: Use sig_atomic_t for signal handlers
> > perf daemon: Use sig_atomic_t to avoid UB
> > perf ftrace: Use sig_atomic_t to avoid UB
> > perf session: Change type to avoid UB
> > perf stat: Use sig_atomic_t to avoid UB
> > perf top: Use sig_atomic_t to avoid UB
> > perf trace: Use sig_atomic_t to avoid UB
> >
> > tools/perf/Makefile.config | 2 +-
> > tools/perf/builtin-daemon.c | 3 ++-
> > tools/perf/builtin-ftrace.c | 4 ++--
> > tools/perf/builtin-record.c | 9 +++++----
> > tools/perf/builtin-stat.c | 9 +++++----
> > tools/perf/builtin-top.c | 4 ++--
> > tools/perf/builtin-trace.c | 4 ++--
> > tools/perf/util/session.c | 3 ++-
> > 8 files changed, 21 insertions(+), 17 deletions(-)
> >
> > --
> > 2.38.0.135.g90850a2211-goog
>
> --
>
> - Arnaldo
Use sig_atomic_t for a variable written to in a signal handler and
read elsewhere. This is undefined behavior as per:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-daemon.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 6cb3f6cc36d0..d05084952c09 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -9,6 +9,7 @@
#include <string.h>
#include <sys/file.h>
#include <signal.h>
+#include <stdatomic.h>
#include <stdlib.h>
#include <time.h>
#include <stdio.h>
@@ -105,7 +106,7 @@ static const char * const daemon_usage[] = {
NULL
};
-static bool done;
+static volatile sig_atomic_t done;
static void sig_handler(int sig __maybe_unused)
{
--
2.38.0.135.g90850a2211-goog
Use sig_atomic_t for variables written/accessed in signal
handlers. This is undefined behavior as per:
https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-stat.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 265b05157972..13bb8508d228 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -83,6 +83,7 @@
#include <inttypes.h>
#include <locale.h>
#include <math.h>
+#include <stdatomic.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
@@ -173,7 +174,7 @@ static struct target target = {
#define METRIC_ONLY_LEN 20
-static volatile pid_t child_pid = -1;
+static volatile sig_atomic_t child_pid = -1;
static int detailed_run = 0;
static bool transaction_run;
static bool topdown_run = false;
@@ -208,7 +209,7 @@ struct perf_stat {
static struct perf_stat perf_stat;
#define STAT_RECORD perf_stat.record
-static volatile int done = 0;
+static volatile sig_atomic_t done = 0;
static struct perf_stat_config stat_config = {
.aggr_mode = AGGR_GLOBAL,
@@ -569,7 +570,7 @@ static void disable_counters(void)
}
}
-static volatile int workload_exec_errno;
+static volatile sig_atomic_t workload_exec_errno;
/*
* evlist__prepare_workload will send a SIGUSR1
@@ -1029,7 +1030,7 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
evlist__print_counters(evsel_list, &stat_config, &target, ts, argc, argv);
}
-static volatile int signr = -1;
+static volatile sig_atomic_t signr = -1;
static void skip_signal(int signo)
{
--
2.38.0.135.g90850a2211-goog
On Mon, Oct 24, 2022 at 10:59 AM Ian Rogers <[email protected]> wrote:
>
> On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > The use of C11 is mainstream in the kernel [1]. There was some
> > > confusion on volatile and signal handlers in [2]. Switch to using
> > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > Yan <[email protected]> for the suggestions.
> > >
> > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > > [2] https://lore.kernel.org/lkml/[email protected]/
> > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
> >
> > I think I'll apply this to perf/core, i.e. for 6.3, ok?
>
> Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> this and to the iterators (from C11) that can be done.
>
> Thanks,
> Ian
So I noticed a few changes missing #include-ing stdatomic.h and
sig_atomic_t is actually in signal.h. I'm not sure we need the C11
change then, but it seems like the right thing to do anyway. I'll do a
v2 to drop the unneeded (currently) include of stdatomic.h.
Thanks,
Ian
> > - Arnaldo
> >
> > >
> > > Ian Rogers (8):
> > > perf build: Update to C standard to gnu11
> > > perf record: Use sig_atomic_t for signal handlers
> > > perf daemon: Use sig_atomic_t to avoid UB
> > > perf ftrace: Use sig_atomic_t to avoid UB
> > > perf session: Change type to avoid UB
> > > perf stat: Use sig_atomic_t to avoid UB
> > > perf top: Use sig_atomic_t to avoid UB
> > > perf trace: Use sig_atomic_t to avoid UB
> > >
> > > tools/perf/Makefile.config | 2 +-
> > > tools/perf/builtin-daemon.c | 3 ++-
> > > tools/perf/builtin-ftrace.c | 4 ++--
> > > tools/perf/builtin-record.c | 9 +++++----
> > > tools/perf/builtin-stat.c | 9 +++++----
> > > tools/perf/builtin-top.c | 4 ++--
> > > tools/perf/builtin-trace.c | 4 ++--
> > > tools/perf/util/session.c | 3 ++-
> > > 8 files changed, 21 insertions(+), 17 deletions(-)
> > >
> > > --
> > > 2.38.0.135.g90850a2211-goog
> >
> > --
> >
> > - Arnaldo
Em Mon, Oct 24, 2022 at 10:59:03AM -0700, Ian Rogers escreveu:
> On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > The use of C11 is mainstream in the kernel [1]. There was some
> > > confusion on volatile and signal handlers in [2]. Switch to using
> > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > Yan <[email protected]> for the suggestions.
> > >
> > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-vLY6vpESDLj6kGUTKO3khGtVfipHqwewh2HQ@mail.gmail.com/
> > > [2] https://lore.kernel.org/lkml/[email protected]/
> > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers
> >
> > I think I'll apply this to perf/core, i.e. for 6.3, ok?
>
> Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> this and to the iterators (from C11) that can be done.
oops, 6.2, sure, 6.1 is the current one, merge window closed. :-)
- Arnaldo
From: Ian Rogers
> Sent: 24 October 2022 19:12
>
> On Mon, Oct 24, 2022 at 10:59 AM Ian Rogers <[email protected]> wrote:
> >
> > On Mon, Oct 24, 2022 at 10:51 AM Arnaldo Carvalho de Melo
> > <[email protected]> wrote:
> > >
> > > Em Mon, Oct 24, 2022 at 10:35:15AM -0700, Ian Rogers escreveu:
> > > > The use of C11 is mainstream in the kernel [1]. There was some
> > > > confusion on volatile and signal handlers in [2]. Switch to using
> > > > stdatomic.h (requires C11) and sig_atomic_t as per [3]. Thanks to Leo
> > > > Yan <[email protected]> for the suggestions.
> > > >
> > > > [1] https://lore.kernel.org/lkml/CAHk-=whWbENRz-
> [email protected]/
> > > > [2] https://lore.kernel.org/lkml/[email protected]/
> > > > [3] https://wiki.sei.cmu.edu/confluence/display/c/SIG31-
> C.+Do+not+access+shared+objects+in+signal+handlers
> > >
> > > I think I'll apply this to perf/core, i.e. for 6.3, ok?
> >
> > Sounds good to me. 6.3 or 6.2? I suspect there is more cleanup like
> > this and to the iterators (from C11) that can be done.
> >
> > Thanks,
> > Ian
>
> So I noticed a few changes missing #include-ing stdatomic.h and
> sig_atomic_t is actually in signal.h. I'm not sure we need the C11
> change then, but it seems like the right thing to do anyway. I'll do a
> v2 to drop the unneeded (currently) include of stdatomic.h.
While the C standard requires you use sig_atomic_t (to avoid
wider RMW being done for writes) the kernel very much requires
that volatiles accesses are atomic.
So the compilers used will do that even when the C standard
would allow them to do otherwise.
Pretty much the last mainstream cpu that couldn't do atomic
byte writes was an old alpha.
So for anything that Linux is going to run on these changes
aren't really needed.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Oct 25, 2022 at 10:36:16AM +0000, David Laight wrote:
[...]
> > So I noticed a few changes missing #include-ing stdatomic.h and
> > sig_atomic_t is actually in signal.h. I'm not sure we need the C11
> > change then, but it seems like the right thing to do anyway. I'll do a
> > v2 to drop the unneeded (currently) include of stdatomic.h.
>
> While the C standard requires you use sig_atomic_t (to avoid
> wider RMW being done for writes) the kernel very much requires
> that volatiles accesses are atomic.
> So the compilers used will do that even when the C standard
> would allow them to do otherwise.
>
> Pretty much the last mainstream cpu that couldn't do atomic
> byte writes was an old alpha.
One case that it might break atomicity for the data with int type,
I.e. on Arm CPU when CPU access data without alignment, then it
cannot promise atomicity. I am not sure if the data with int type
will be always compiled with 4-byte alignment, if it's packed
without alignment then sig_atomic_t is still required.
Clarify that I don't have deep understanding for compiler, so sorry
if I introduce any noise.
Thanks,
Leo
> So for anything that Linux is going to run on these changes
> aren't really needed.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)