| Message ID | YLAXfvZ+rObEOdc/@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | afs: fix tracepoint string placement with built-in AFS | expand |
ping! Currently "perf record -e [tracepoint]" is unusable if AFS is builtin. On Fri, May 28, 2021 at 01:04:46AM +0300, Alexey Dobriyan wrote: > I was adding custom tracepoint to the kernel, grabbed full F34 kernel > .config, disabled modules and booted whole shebang as VM kernel. > > Then did > > perf record -a -e ... > > It crashed: > > general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI > CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > RIP: 0010:t_show+0x22/0xd0 > > Then reproducer was narrowed to > > # cat /sys/kernel/tracing/printk_formats > > Original F34 kernel with modules didn't crash. > > So I started to disable options and after disabling AFS everything > started working again. > > The root cause is that AFS was placing char arrays content into a section > full of _pointers_ to strings with predictable consequences. > > Non canonical address 435f5346592e4243 is "CB.YFS_" which came from > CM_NAME macro. > > The fix is to create char array and pointer to it separatedly. > > Steps to reproduce: > > CONFIG_AFS=y > CONFIG_TRACING=y > > # cat /sys/kernel/tracing/printk_formats > > Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com> > --- > > fs/afs/cmservice.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > --- a/fs/afs/cmservice.c > +++ b/fs/afs/cmservice.c > @@ -30,8 +30,9 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *); > static int afs_deliver_yfs_cb_callback(struct afs_call *); > > #define CM_NAME(name) \ > - char afs_SRXCB##name##_name[] __tracepoint_string = \ > - "CB." #name > + const char afs_SRXCB##name##_name[] = "CB." #name; \ > + static const char *_afs_SRXCB##name##_name __tracepoint_string =\ > + afs_SRXCB##name##_name > > /* > * CB.CallBack operation type
On Fri, 28 May 2021 01:04:46 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote: > I was adding custom tracepoint to the kernel, grabbed full F34 kernel > .config, disabled modules and booted whole shebang as VM kernel. > > Then did > > perf record -a -e ... > > It crashed: > > general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI > CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > RIP: 0010:t_show+0x22/0xd0 > > Then reproducer was narrowed to > > # cat /sys/kernel/tracing/printk_formats > > Original F34 kernel with modules didn't crash. > > So I started to disable options and after disabling AFS everything > started working again. > > The root cause is that AFS was placing char arrays content into a section > full of _pointers_ to strings with predictable consequences. > > Non canonical address 435f5346592e4243 is "CB.YFS_" which came from > CM_NAME macro. > > The fix is to create char array and pointer to it separatedly. > > Steps to reproduce: > > CONFIG_AFS=y > CONFIG_TRACING=y > > # cat /sys/kernel/tracing/printk_formats I'll add Fixes: 8e8d7f13b6d5a9 ("afs: Add some tracepoints") although Andi's d2abfa86ff373 gets in the way a bit. > --- a/fs/afs/cmservice.c > +++ b/fs/afs/cmservice.c > @@ -30,8 +30,9 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *); > static int afs_deliver_yfs_cb_callback(struct afs_call *); > > #define CM_NAME(name) \ > - char afs_SRXCB##name##_name[] __tracepoint_string = \ > - "CB." #name > + const char afs_SRXCB##name##_name[] = "CB." #name; \ > + static const char *_afs_SRXCB##name##_name __tracepoint_string =\ > + afs_SRXCB##name##_name Should/can afs_SRXCB##name##_name[] be static? __tracepoint_string is very rarely used. I wonder if there's much point in it existing? kernel/rcu/tree.h does static char rcu_name[] = RCU_NAME_RAW; static const char *tp_rcu_varname __used __tracepoint_string = rcu_name; which is asking the compiler to place a copy of these into each compilation unit which includes tree.h, which probably isn't what was intended.
On Mon, 14 Jun 2021 16:47:52 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > Should/can afs_SRXCB##name##_name[] be static? Probably. > > > __tracepoint_string is very rarely used. I wonder if there's much > point in it existing? There's a few places that the "tracepoint_string()" couldn't be used, and this was a workaround for those locations. Yes, it's not used much, but what's the other solution should we use to replace it? > > > kernel/rcu/tree.h does > > static char rcu_name[] = RCU_NAME_RAW; > static const char *tp_rcu_varname __used __tracepoint_string = rcu_name; > > which is asking the compiler to place a copy of these into each > compilation unit which includes tree.h, which probably isn't what was > intended. Yeah, there's a couple of duplicates. Perhaps we can move them into a C file, and have it simply exported. -- Steve
Alexey Dobriyan <adobriyan@gmail.com> wrote: > - char afs_SRXCB##name##_name[] __tracepoint_string = \ > - "CB." #name I seem to remember that when I did this, it couldn't be a const string for some reason, though I don't remember exactly why now if that was indeed the case. I wonder if it's better just to turn it into an enum-string table in linux/events/afs.h. David
David Howells <dhowells@redhat.com> wrote: > Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > - char afs_SRXCB##name##_name[] __tracepoint_string = \ > > - "CB." #name > > I seem to remember that when I did this, it couldn't be a const string for > some reason, though I don't remember exactly why now if that was indeed the > case. > > I wonder if it's better just to turn it into an enum-string table in > linux/events/afs.h. Hmmm... It's not necessarily quite that simple - at least if I want to use the operation ID as the key to the table - as there are at least three separate services involved and they can have overlapping op IDs. Is it possible to switch the table passed to __print_symbolic()? For example, in the afs_call tracepoint, could I do: TP_printk("c=%08x %s u=%d o=%d sp=%pSR", __entry->call, __print_symbolic(__entry->op, __entry->is_vl ? afs_vl_call_traces : __entry->is_yfs ? afs_yfs_call_traces : afs_fs_call_traces), __entry->usage, __entry->outstanding, __entry->where) David
Here's my take on fixing this.
David
---
afs: Fix tracepoint string placement with built-in AFS
To quote Alexey[1]:
I was adding custom tracepoint to the kernel, grabbed full F34 kernel
.config, disabled modules and booted whole shebang as VM kernel.
Then did
perf record -a -e ...
It crashed:
general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI
CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014
RIP: 0010:t_show+0x22/0xd0
Then reproducer was narrowed to
# cat /sys/kernel/tracing/printk_formats
Original F34 kernel with modules didn't crash.
So I started to disable options and after disabling AFS everything
started working again.
The root cause is that AFS was placing char arrays content into a
section full of _pointers_ to strings with predictable consequences.
Non canonical address 435f5346592e4243 is "CB.YFS_" which came from
CM_NAME macro.
Steps to reproduce:
CONFIG_AFS=y
CONFIG_TRACING=y
# cat /sys/kernel/tracing/printk_formats
Fix this by the following means:
(1) Add enum->string translation tables in the event header with the AFS
and YFS cache/callback manager operations listed by RPC operation ID.
(2) Modify the afs_cb_call tracepoint to print the string from the
translation table rather than using the string at the afs_call name
pointer.
(3) Switch translation table depending on the service we're being accessed
as (AFS or YFS) in the tracepoint print clause. Will this cause
problems to userspace utilities?
Note that the symbolic representation of the YFS service ID isn't
available to this header, so I've put it in as a number. I'm not sure
if this is the best way to do this.
(4) Remove the name wrangling (CM_NAME) macro and put the names directly
into the afs_call_type structs in cmservice.c.
Fixes: 8e8d7f13b6d5a9 ("afs: Add some tracepoints")
Reported-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Andrew Morton <akpm@linux-foundation.org>
cc: Steven Rostedt <rostedt@goodmis.org>
cc: linux-afs@lists.infradead.org
Link: https://lore.kernel.org/linux-fsdevel/YLAXfvZ+rObEOdc%2F@localhost.localdomain/ [1]
---
fs/afs/cmservice.c | 25 ++++------------
include/trace/events/afs.h | 67 +++++++++++++++++++++++++++++++++++++++++----
2 files changed, 69 insertions(+), 23 deletions(-)
diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c
index d3c6bb22c5f4..a3f5de28be79 100644
--- a/fs/afs/cmservice.c
+++ b/fs/afs/cmservice.c
@@ -29,16 +29,11 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *);
static int afs_deliver_yfs_cb_callback(struct afs_call *);
-#define CM_NAME(name) \
- char afs_SRXCB##name##_name[] __tracepoint_string = \
- "CB." #name
-
/*
* CB.CallBack operation type
*/
-static CM_NAME(CallBack);
static const struct afs_call_type afs_SRXCBCallBack = {
- .name = afs_SRXCBCallBack_name,
+ .name = "CB.CallBack",
.deliver = afs_deliver_cb_callback,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_CallBack,
@@ -47,9 +42,8 @@ static const struct afs_call_type afs_SRXCBCallBack = {
/*
* CB.InitCallBackState operation type
*/
-static CM_NAME(InitCallBackState);
static const struct afs_call_type afs_SRXCBInitCallBackState = {
- .name = afs_SRXCBInitCallBackState_name,
+ .name = "CB.InitCallBackState",
.deliver = afs_deliver_cb_init_call_back_state,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_InitCallBackState,
@@ -58,9 +52,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState = {
/*
* CB.InitCallBackState3 operation type
*/
-static CM_NAME(InitCallBackState3);
static const struct afs_call_type afs_SRXCBInitCallBackState3 = {
- .name = afs_SRXCBInitCallBackState3_name,
+ .name = "CB.InitCallBackState3",
.deliver = afs_deliver_cb_init_call_back_state3,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_InitCallBackState,
@@ -69,9 +62,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState3 = {
/*
* CB.Probe operation type
*/
-static CM_NAME(Probe);
static const struct afs_call_type afs_SRXCBProbe = {
- .name = afs_SRXCBProbe_name,
+ .name = "CB.Probe",
.deliver = afs_deliver_cb_probe,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_Probe,
@@ -80,9 +72,8 @@ static const struct afs_call_type afs_SRXCBProbe = {
/*
* CB.ProbeUuid operation type
*/
-static CM_NAME(ProbeUuid);
static const struct afs_call_type afs_SRXCBProbeUuid = {
- .name = afs_SRXCBProbeUuid_name,
+ .name = "CB.ProbeUuid",
.deliver = afs_deliver_cb_probe_uuid,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_ProbeUuid,
@@ -91,9 +82,8 @@ static const struct afs_call_type afs_SRXCBProbeUuid = {
/*
* CB.TellMeAboutYourself operation type
*/
-static CM_NAME(TellMeAboutYourself);
static const struct afs_call_type afs_SRXCBTellMeAboutYourself = {
- .name = afs_SRXCBTellMeAboutYourself_name,
+ .name = "CB.TellMeAboutYourself",
.deliver = afs_deliver_cb_tell_me_about_yourself,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_TellMeAboutYourself,
@@ -102,9 +92,8 @@ static const struct afs_call_type afs_SRXCBTellMeAboutYourself = {
/*
* YFS CB.CallBack operation type
*/
-static CM_NAME(YFS_CallBack);
static const struct afs_call_type afs_SRXYFSCB_CallBack = {
- .name = afs_SRXCBYFS_CallBack_name,
+ .name = "YFSCB.CallBack",
.deliver = afs_deliver_yfs_cb_callback,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_CallBack,
diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h
index 3ccf591b2374..9f73ed2cf061 100644
--- a/include/trace/events/afs.h
+++ b/include/trace/events/afs.h
@@ -174,6 +174,34 @@ enum afs_vl_operation {
afs_VL_GetCapabilities = 65537, /* AFS Get VL server capabilities */
};
+enum afs_cm_operation {
+ afs_CB_CallBack = 204, /* AFS break callback promises */
+ afs_CB_InitCallBackState = 205, /* AFS initialise callback state */
+ afs_CB_Probe = 206, /* AFS probe client */
+ afs_CB_GetLock = 207, /* AFS get contents of CM lock table */
+ afs_CB_GetCE = 208, /* AFS get cache file description */
+ afs_CB_GetXStatsVersion = 209, /* AFS get version of extended statistics */
+ afs_CB_GetXStats = 210, /* AFS get contents of extended statistics data */
+ afs_CB_InitCallBackState3 = 213, /* AFS initialise callback state, version 3 */
+ afs_CB_ProbeUuid = 214, /* AFS check the client hasn't rebooted */
+};
+
+enum yfs_cm_operation {
+ yfs_CB_Probe = 206, /* YFS probe client */
+ yfs_CB_GetLock = 207, /* YFS get contents of CM lock table */
+ yfs_CB_XStatsVersion = 209, /* YFS get version of extended statistics */
+ yfs_CB_GetXStats = 210, /* YFS get contents of extended statistics data */
+ yfs_CB_InitCallBackState3 = 213, /* YFS initialise callback state, version 3 */
+ yfs_CB_ProbeUuid = 214, /* YFS check the client hasn't rebooted */
+ yfs_CB_GetServerPrefs = 215,
+ yfs_CB_GetCellServDV = 216,
+ yfs_CB_GetLocalCell = 217,
+ yfs_CB_GetCacheConfig = 218,
+ yfs_CB_GetCellByNum = 65537,
+ yfs_CB_TellMeAboutYourself = 65538, /* get client capabilities */
+ yfs_CB_CallBack = 64204,
+};
+
enum afs_edit_dir_op {
afs_edit_dir_create,
afs_edit_dir_create_error,
@@ -436,6 +464,32 @@ enum afs_cb_break_reason {
EM(afs_YFSVL_GetCellName, "YFSVL.GetCellName") \
E_(afs_VL_GetCapabilities, "VL.GetCapabilities")
+#define afs_cm_operations \
+ EM(afs_CB_CallBack, "CB.CallBack") \
+ EM(afs_CB_InitCallBackState, "CB.InitCallBackState") \
+ EM(afs_CB_Probe, "CB.Probe") \
+ EM(afs_CB_GetLock, "CB.GetLock") \
+ EM(afs_CB_GetCE, "CB.GetCE") \
+ EM(afs_CB_GetXStatsVersion, "CB.GetXStatsVersion") \
+ EM(afs_CB_GetXStats, "CB.GetXStats") \
+ EM(afs_CB_InitCallBackState3, "CB.InitCallBackState3") \
+ E_(afs_CB_ProbeUuid, "CB.ProbeUuid")
+
+#define yfs_cm_operations \
+ EM(yfs_CB_Probe, "YFSCB.Probe") \
+ EM(yfs_CB_GetLock, "YFSCB.GetLock") \
+ EM(yfs_CB_XStatsVersion, "YFSCB.XStatsVersion") \
+ EM(yfs_CB_GetXStats, "YFSCB.GetXStats") \
+ EM(yfs_CB_InitCallBackState3, "YFSCB.InitCallBackState3") \
+ EM(yfs_CB_ProbeUuid, "YFSCB.ProbeUuid") \
+ EM(yfs_CB_GetServerPrefs, "YFSCB.GetServerPrefs") \
+ EM(yfs_CB_GetCellServDV, "YFSCB.GetCellServDV") \
+ EM(yfs_CB_GetLocalCell, "YFSCB.GetLocalCell") \
+ EM(yfs_CB_GetCacheConfig, "YFSCB.GetCacheConfig") \
+ EM(yfs_CB_GetCellByNum, "YFSCB.GetCellByNum") \
+ EM(yfs_CB_TellMeAboutYourself, "YFSCB.TellMeAboutYourself") \
+ E_(yfs_CB_CallBack, "YFSCB.CallBack")
+
#define afs_edit_dir_ops \
EM(afs_edit_dir_create, "create") \
EM(afs_edit_dir_create_error, "c_fail") \
@@ -569,6 +623,8 @@ afs_server_traces;
afs_cell_traces;
afs_fs_operations;
afs_vl_operations;
+afs_cm_operations;
+yfs_cm_operations;
afs_edit_dir_ops;
afs_edit_dir_reasons;
afs_eproto_causes;
@@ -649,20 +705,21 @@ TRACE_EVENT(afs_cb_call,
TP_STRUCT__entry(
__field(unsigned int, call )
- __field(const char *, name )
__field(u32, op )
+ __field(u16, service_id )
),
TP_fast_assign(
__entry->call = call->debug_id;
- __entry->name = call->type->name;
__entry->op = call->operation_ID;
+ __entry->service_id = call->service_id;
),
- TP_printk("c=%08x %s o=%u",
+ TP_printk("c=%08x %s",
__entry->call,
- __entry->name,
- __entry->op)
+ __entry->service_id == 2501 ?
+ __print_symbolic(__entry->op, yfs_cm_operations) :
+ __print_symbolic(__entry->op, afs_cm_operations))
);
TRACE_EVENT(afs_call,
On Tue, 15 Jun 2021 11:58:19 +0100 David Howells <dhowells@redhat.com> wrote: > @@ -649,20 +705,21 @@ TRACE_EVENT(afs_cb_call, > > TP_STRUCT__entry( > __field(unsigned int, call ) > - __field(const char *, name ) > __field(u32, op ) > + __field(u16, service_id ) > ), > > TP_fast_assign( > __entry->call = call->debug_id; > - __entry->name = call->type->name; > __entry->op = call->operation_ID; > + __entry->service_id = call->service_id; > ), > > - TP_printk("c=%08x %s o=%u", > + TP_printk("c=%08x %s", > __entry->call, > - __entry->name, > - __entry->op) > + __entry->service_id == 2501 ? > + __print_symbolic(__entry->op, yfs_cm_operations) : > + __print_symbolic(__entry->op, afs_cm_operations)) > ); > > TRACE_EVENT(afs_call, Looks fine to me, and even saves 4 bytes on 64 bit machines (events are rounded up to 4 byte increments, so the u16 is no different than a u32 here). -- Steve
Steven Rostedt <rostedt@goodmis.org> wrote: > Looks fine to me, and even saves 4 bytes on 64 bit machines (events are > rounded up to 4 byte increments, so the u16 is no different than a u32 > here). Can I put that down as a Reviewed-by? David
On Thu, 17 Jun 2021 13:04:49 +0100 David Howells <dhowells@redhat.com> wrote: > Steven Rostedt <rostedt@goodmis.org> wrote: > > > Looks fine to me, and even saves 4 bytes on 64 bit machines (events are > > rounded up to 4 byte increments, so the u16 is no different than a u32 > > here). > > Can I put that down as a Reviewed-by? Sure. I was going to recommend consolidating the mappings a bit more to have the enums and numbers defined in a single macro, but then I saw that it matches the rest of the header file, and to do the consolidation would require modifying the existing code, which I thought wasn't worth the effort. Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve
--- a/fs/afs/cmservice.c +++ b/fs/afs/cmservice.c @@ -30,8 +30,9 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *); static int afs_deliver_yfs_cb_callback(struct afs_call *); #define CM_NAME(name) \ - char afs_SRXCB##name##_name[] __tracepoint_string = \ - "CB." #name + const char afs_SRXCB##name##_name[] = "CB." #name; \ + static const char *_afs_SRXCB##name##_name __tracepoint_string =\ + afs_SRXCB##name##_name /* * CB.CallBack operation type
I was adding custom tracepoint to the kernel, grabbed full F34 kernel .config, disabled modules and booted whole shebang as VM kernel. Then did perf record -a -e ... It crashed: general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 RIP: 0010:t_show+0x22/0xd0 Then reproducer was narrowed to # cat /sys/kernel/tracing/printk_formats Original F34 kernel with modules didn't crash. So I started to disable options and after disabling AFS everything started working again. The root cause is that AFS was placing char arrays content into a section full of _pointers_ to strings with predictable consequences. Non canonical address 435f5346592e4243 is "CB.YFS_" which came from CM_NAME macro. The fix is to create char array and pointer to it separatedly. Steps to reproduce: CONFIG_AFS=y CONFIG_TRACING=y # cat /sys/kernel/tracing/printk_formats Signed-off-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com> --- fs/afs/cmservice.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)