On Sun, 29 Oct 2023 at 02:16, Steven Rostedt <[email protected]> wrote:
>
> From: "Steven Rostedt (Google)" <[email protected]>
>
> The eventfs_inode (ei) is protected by SRCU, but the ei->dentry is not. It
> is protected by the eventfs_mutex. Anytime the eventfs_mutex is released,
> and access to the ei->dentry needs to be done, it should first check if
> ei->is_freed is set under the eventfs_mutex. If it is, then the ei->dentry
> is invalid and must not be used. The ei->dentry must only be accessed
> under the eventfs_mutex and after checking if ei->is_freed is set.
>
> When the ei is being freed, it will (under the eventfs_mutex) set is_freed
> and at the same time move the dentry to a free list to be cleared after
> the eventfs_mutex is released. This means that any access to the
> ei->dentry must check first if ei->is_freed is set, because if it is, then
> the dentry is on its way to be freed.
>
> Also add comments to describe this better.
>
> Link: https://lore.kernel.org/all/CA+G9fYt6pY+tMZEOg=SoEywQOe19fGP3uR15SGowkdK+_X85Cg@mail.gmail.com/
> Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=6aA@mail.gmail.com/
>
> Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
> Reported-by: Linux Kernel Functional Testing <[email protected]>
> Reported-by: Naresh Kamboju <[email protected]>
> Reported-by: Beau Belgrave <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
Following build errors have been noticed.
fs/tracefs/event_inode.c:348:1: error: return type defaults to 'int'
[-Werror=implicit-int]
348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
| ^~~~~~~~~~~~~~~~~
In file included from include/uapi/linux/posix_types.h:5,
from include/uapi/linux/types.h:14,
from include/linux/types.h:6,
from include/linux/kasan-checks.h:5,
from include/asm-generic/rwonce.h:26,
from ./arch/x86/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:251,
from include/linux/build_bug.h:5,
from include/linux/bits.h:21,
from include/linux/bitops.h:6,
from include/linux/radix-tree.h:11,
from include/linux/idr.h:15,
from include/linux/fsnotify_backend.h:13,
from include/linux/fsnotify.h:15,
from fs/tracefs/event_inode.c:17:
fs/tracefs/event_inode.c: In function 'create_dir_dentry':
include/linux/stddef.h:8:14: error: returning 'void *' from a function
with return type 'int' makes integer from pointer without a cast
[-Werror=int-conversion]
8 | #define NULL ((void *)0)
| ^
fs/tracefs/event_inode.c:357:24: note: in expansion of macro 'NULL'
357 | return NULL;
| ^~~~
fs/tracefs/event_inode.c:366:24: error: returning 'struct dentry *'
from a function with return type 'int' makes integer from pointer
without a cast [-Werror=int-conversion]
366 | return dentry;
| ^~~~~~
fs/tracefs/event_inode.c:394:24: error: returning 'struct dentry *'
from a function with return type 'int' makes integer from pointer
without a cast [-Werror=int-conversion]
394 | return dentry;
| ^~~~~~
fs/tracefs/event_inode.c:416:34: error: returning 'struct dentry *'
from a function with return type 'int' makes integer from pointer
without a cast [-Werror=int-conversion]
416 | return invalidate ? NULL : dentry;
| ~~~~~~~~~~~~~~~~~~^~~~~~~~
fs/tracefs/event_inode.c: In function 'dcache_dir_open_wrapper':
fs/tracefs/event_inode.c:609:49: error: passing argument 2 of
'create_dir_dentry' from incompatible pointer type
[-Werror=incompatible-pointer-types]
609 | d = create_dir_dentry(ei_child, parent, false);
| ^~~~~~
| |
| struct dentry *
fs/tracefs/event_inode.c:348:68: note: expected 'struct eventfs_inode
*' but argument is of type 'struct dentry *'
348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
| ~~~~~~~~~~~~~~~~~~~~~~^~
fs/tracefs/event_inode.c:609:21: error: too few arguments to function
'create_dir_dentry'
609 | d = create_dir_dentry(ei_child, parent, false);
| ^~~~~~~~~~~~~~~~~
fs/tracefs/event_inode.c:348:1: note: declared here
348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
| ^~~~~~~~~~~~~~~~~
fs/tracefs/event_inode.c:625:19: error: assignment to 'struct dentry
*' from 'int' makes pointer from integer without a cast
[-Werror=int-conversion]
625 | d = create_dir_dentry(ei, ei_child, parent, false);
| ^
fs/tracefs/event_inode.c:626:46: error: left-hand operand of comma
expression has no effect [-Werror=unused-value]
626 | parent, name, mode,
cdata, fops, false);
| ^
fs/tracefs/event_inode.c:626:52: error: left-hand operand of comma
expression has no effect [-Werror=unused-value]
626 | parent, name, mode,
cdata, fops, false);
| ^
fs/tracefs/event_inode.c:626:58: error: left-hand operand of comma
expression has no effect [-Werror=unused-value]
626 | parent, name, mode,
cdata, fops, false);
| ^
fs/tracefs/event_inode.c:626:65: error: left-hand operand of comma
expression has no effect [-Werror=unused-value]
626 | parent, name, mode,
cdata, fops, false);
| ^
fs/tracefs/event_inode.c:626:71: error: left-hand operand of comma
expression has no effect [-Werror=unused-value]
626 | parent, name, mode,
cdata, fops, false);
| ^
fs/tracefs/event_inode.c:626:71: error: statement with no effect
[-Werror=unused-value]
fs/tracefs/event_inode.c:626:78: error: expected ';' before ')' token
626 | parent, name, mode,
cdata, fops, false);
|
^
|
;
fs/tracefs/event_inode.c:626:78: error: expected statement before ')' token
fs/tracefs/event_inode.c: In function 'eventfs_remove_dir':
fs/tracefs/event_inode.c:921:1: error: invalid use of void expression
921 | + call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
| ^
cc1: all warnings being treated as errors
Links:
- https://storage.tuxsuite.com/public/linaro/naresh/builds/2XQUK9V1Fm5uX0GdoaraRkzAN29/
> ---
>
> Changes since v1: https://lore.kernel.org/all/[email protected]/
>
> - Add comment about ei->is_freed is a union along with ei->rcu and
> ei->del_list so that others can find where ei->is_freed is set and
> not get confused about why ei->dentry is being removed but ei->is_freed
> isn't mentioned.
>
> - And fixed change log to remove the double "Reported-by".
>
> fs/tracefs/event_inode.c | 65 +++++++++++++++++++++++++++++++++-------
> fs/tracefs/internal.h | 3 +-
> 2 files changed, 56 insertions(+), 12 deletions(-)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 4d2da7480e5f..45bddce7c747 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -24,7 +24,20 @@
> #include <linux/delay.h>
> #include "internal.h"
>
> +/*
> + * eventfs_mutex protects the eventfs_inode (ei) dentry. Any access
> + * to the ei->dentry must be done under this mutex and after checking
> + * if ei->is_freed is not set. The ei->dentry is released under the
> + * mutex at the same time ei->is_freed is set. If ei->is_freed is set
> + * then the ei->dentry is invalid.
> + */
> static DEFINE_MUTEX(eventfs_mutex);
> +
> +/*
> + * The eventfs_inode (ei) itself is protected by SRCU. It is released from
> + * its parent's list and will have is_freed set (under eventfs_mutex).
> + * After the SRCU grace period is over, the ei may be freed.
> + */
> DEFINE_STATIC_SRCU(eventfs_srcu);
>
> static struct dentry *eventfs_root_lookup(struct inode *dir,
> @@ -234,6 +247,10 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
> bool invalidate = false;
>
> mutex_lock(&eventfs_mutex);
> + if (ei->is_freed) {
> + mutex_unlock(&eventfs_mutex);
> + return NULL;
> + }
> /* If the e_dentry already has a dentry, use it */
> if (*e_dentry) {
> /* lookup does not need to up the ref count */
> @@ -307,6 +324,8 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
> struct eventfs_inode *ei_child;
> struct tracefs_inode *ti;
>
> + lockdep_assert_held(&eventfs_mutex);
> +
> /* srcu lock already held */
> /* fill parent-child relation */
> list_for_each_entry_srcu(ei_child, &ei->children, list,
> @@ -320,6 +339,7 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
>
> /**
> * create_dir_dentry - Create a directory dentry for the eventfs_inode
> + * @pei: The eventfs_inode parent of ei.
> * @ei: The eventfs_inode to create the directory for
> * @parent: The dentry of the parent of this directory
> * @lookup: True if this is called by the lookup code
> @@ -327,12 +347,17 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
> * This creates and attaches a directory dentry to the eventfs_inode @ei.
> */
> static struct dentry *
> -create_dir_dentry(struct eventfs_inode *ei, struct dentry *parent, bool lookup)
> +create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
> + struct dentry *parent, bool lookup)
> {
> bool invalidate = false;
> struct dentry *dentry = NULL;
>
> mutex_lock(&eventfs_mutex);
> + if (pei->is_freed || ei->is_freed) {
> + mutex_unlock(&eventfs_mutex);
> + return NULL;
> + }
> if (ei->dentry) {
> /* If the dentry already has a dentry, use it */
> dentry = ei->dentry;
> @@ -435,7 +460,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> */
> mutex_lock(&eventfs_mutex);
> ei = READ_ONCE(ti->private);
> - if (ei)
> + if (ei && !ei->is_freed)
> ei_dentry = READ_ONCE(ei->dentry);
> mutex_unlock(&eventfs_mutex);
>
> @@ -449,7 +474,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> if (strcmp(ei_child->name, name) != 0)
> continue;
> ret = simple_lookup(dir, dentry, flags);
> - create_dir_dentry(ei_child, ei_dentry, true);
> + create_dir_dentry(ei, ei_child, ei_dentry, true);
> created = true;
> break;
> }
> @@ -583,7 +608,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>
> list_for_each_entry_srcu(ei_child, &ei->children, list,
> srcu_read_lock_held(&eventfs_srcu)) {
> - d = create_dir_dentry(ei_child, parent, false);
> + d = create_dir_dentry(ei, ei_child, parent, false);
> if (d) {
> ret = add_dentries(&dentries, d, cnt);
> if (ret < 0)
> @@ -637,6 +662,13 @@ static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx)
> return ret;
> }
>
> +static void free_ei(struct eventfs_inode *ei)
> +{
> + kfree_const(ei->name);
> + kfree(ei->d_children);
> + kfree(ei);
> +}
> +
> /**
> * eventfs_create_dir - Create the eventfs_inode for this directory
> * @name: The name of the directory to create.
> @@ -700,12 +732,20 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
> ei->nr_entries = size;
> ei->data = data;
> INIT_LIST_HEAD(&ei->children);
> + INIT_LIST_HEAD(&ei->list);
>
> mutex_lock(&eventfs_mutex);
> - list_add_tail(&ei->list, &parent->children);
> - ei->d_parent = parent->dentry;
> + if (!parent->is_freed) {
> + list_add_tail(&ei->list, &parent->children);
> + ei->d_parent = parent->dentry;
> + }
> mutex_unlock(&eventfs_mutex);
>
> + /* Was the parent freed? */
> + if (list_empty(&ei->list)) {
> + free_ei(ei);
> + ei = NULL;
> + }
> return ei;
> }
>
> @@ -787,13 +827,11 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
> return ERR_PTR(-ENOMEM);
> }
>
> -static void free_ei(struct rcu_head *head)
> +static void free_rcu_ei(struct rcu_head *head)
> {
> struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu);
>
> - kfree_const(ei->name);
> - kfree(ei->d_children);
> - kfree(ei);
> + free_ei(ei);
> }
>
> /**
> @@ -880,7 +918,12 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
> for (i = 0; i < ei->nr_entries; i++)
> unhook_dentry(&ei->d_children[i], &dentry_list);
> unhook_dentry(&ei->dentry, &dentry_list);
> - call_srcu(&eventfs_srcu, &ei->rcu, free_ei);
> + /*
> + * Note, ei->is_freed is a union along with ei->rcu
> + * and ei->del_list. When the ei is added to either
> + * of those lists, it automatically sets ei->is_freed.
> + */
> + call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
> }
> mutex_unlock(&eventfs_mutex);
>
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index 64fde9490f52..21a1fa682b74 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -30,7 +30,7 @@ struct eventfs_inode {
> const struct eventfs_entry *entries;
> const char *name;
> struct list_head children;
> - struct dentry *dentry;
> + struct dentry *dentry; /* Check is_freed to access */
> struct dentry *d_parent;
> struct dentry **d_children;
> void *data;
> @@ -39,6 +39,7 @@ struct eventfs_inode {
> * @del_list: list of eventfs_inode to delete
> * @rcu: eventfs_inode to delete in RCU
> * @is_freed: node is freed if one of the above is set
> + * Note if is_freed is set, then dentry is corrupted.
> */
> union {
> struct list_head del_list;
> --
> 2.42.0
>
On Sun, 29 Oct 2023 12:31:54 +0530
Naresh Kamboju <[email protected]> wrote:
> Following build errors have been noticed.
>
> fs/tracefs/event_inode.c:348:1: error: return type defaults to 'int'
> [-Werror=implicit-int]
> 348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
> | ^~~~~~~~~~~~~~~~~
> In file included from include/uapi/linux/posix_types.h:5,
> from include/uapi/linux/types.h:14,
> from include/linux/types.h:6,
> from include/linux/kasan-checks.h:5,
> from include/asm-generic/rwonce.h:26,
> from ./arch/x86/include/generated/asm/rwonce.h:1,
This patch passed all my tests, I have no idea how the above happened.
The "return type defaults to int" tells me the "static struct dentry *"
part of the patch may have been clipped.
Are you sure it was applied correctly? Perhaps check out the branch I
have and let me know if you get the same errors.
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/core
-- Steve
Hi Steven,
> Are you sure it was applied correctly?
Please ignore the build warnings / errors it was due to apply patch was
not successful.
> Perhaps check out the branch I
> have and let me know if you get the same errors.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/core
I have tested the linux-trace.git trace/core and run selftests ftrace
the reported kernel panic [1] & [2] has been fixed but found
"general protection fault" at kernel/trace/trace_events.c:2439.
[1] https://lore.kernel.org/all/CA+G9fYt6pY+tMZEOg=SoEywQOe19fGP3uR15SGowkdK+_X85Cg@mail.gmail.com/
[2] https://lore.kernel.org/linux-trace-kernel/[email protected]/
Reported-by: Linux Kernel Functional Testing <[email protected]>
Test log:
==========
kselftest: Running tests in ftrace
TAP version 13
1..1
# timeout set to 0
# selftests: ftrace: ftracetest-ktap
...
# ok 46 Test creation and deletion of trace instances while setting an event
[ 332.783872] general protection fault, probably for non-canonical
address 0x6b6b6b6b6b6b6bcb: 0000 [#1] PREEMPT SMP PTI
[ 332.794585] CPU: 1 PID: 5165 Comm: ls Not tainted 6.6.0-rc4 #1
[ 332.800429] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS
2.7 12/07/2021
[ 332.807820] RIP: 0010:event_callback (kernel/trace/trace_events.c:2439)
[ 332.812353] Code: f6 48 c7 c6 09 78 6b a0 41 55 49 89 cd 41 54 49 89
d4 53 48 8b 02 48 89 fb 4c 8b 78 10 e8 10 8b 19 01 85 c0 0f 84 d2 00
00 00 <41> 8b 47 60 a8 08 0f 85 9c 00 00 00 49 8b 47 10 48 83 78 18 00
74
All code
========
0: f6 48 c7 c6 testb $0xc6,-0x39(%rax)
4: 09 78 6b or %edi,0x6b(%rax)
7: a0 41 55 49 89 cd 41 movabs 0x495441cd89495541,%al
e: 54 49
10: 89 d4 mov %edx,%esp
12: 53 push %rbx
13: 48 8b 02 mov (%rdx),%rax
16: 48 89 fb mov %rdi,%rbx
19: 4c 8b 78 10 mov 0x10(%rax),%r15
1d: e8 10 8b 19 01 callq 0x1198b32
22: 85 c0 test %eax,%eax
24: 0f 84 d2 00 00 00 je 0xfc
2a:* 41 8b 47 60 mov 0x60(%r15),%eax <-- trapping instruction
2e: a8 08 test $0x8,%al
30: 0f 85 9c 00 00 00 jne 0xd2
36: 49 8b 47 10 mov 0x10(%r15),%rax
3a: 48 83 78 18 00 cmpq $0x0,0x18(%rax)
3f: 74 .byte 0x74
Code starting with the faulting instruction
===========================================
0: 41 8b 47 60 mov 0x60(%r15),%eax
4: a8 08 test $0x8,%al
6: 0f 85 9c 00 00 00 jne 0xa8
c: 49 8b 47 10 mov 0x10(%r15),%rax
10: 48 83 78 18 00 cmpq $0x0,0x18(%rax)
15: 74 .byte 0x74
[ 332.831089] RSP: 0018:ffffba5700967b50 EFLAGS: 00010202
[ 332.836315] RAX: 0000000000000001 RBX: ffffffffa06fb5d7 RCX: ffffba5700967bc8
[ 332.843439] RDX: 0000000000000069 RSI: ffffffffa06b7809 RDI: ffffffffa06fb5d7
[ 332.850563] RBP: ffffba5700967b78 R08: ffff8dbe12250134 R09: 0000000000000000
[ 332.857722] R10: ffff8dbe122500e0 R11: ffff8dbe0d8f8fc8 R12: ffffba5700967bd0
[ 332.864846] R13: ffffba5700967bc8 R14: ffffba5700967bc6 R15: 6b6b6b6b6b6b6b6b
[ 332.871971] FS: 00007fc5a5062d00(0000) GS:ffff8dc167a80000(0000)
knlGS:0000000000000000
[ 332.880057] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 332.885801] CR2: 00007f7f8dc50a1c CR3: 0000000113c84003 CR4: 00000000003706e0
[ 332.892928] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 332.900058] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 332.907183] Call Trace:
[ 332.909658] <TASK>
[ 332.911760] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[ 332.915166] ? die_addr (arch/x86/kernel/dumpstack.c:421
arch/x86/kernel/dumpstack.c:460)
[ 332.918486] ? exc_general_protection (arch/x86/kernel/traps.c:697
arch/x86/kernel/traps.c:642)
[ 332.923191] ? asm_exc_general_protection
(arch/x86/include/asm/idtentry.h:564)
[ 332.928073] ? event_callback (kernel/trace/trace_events.c:2439)
[ 332.931996] dcache_dir_open_wrapper (fs/tracefs/event_inode.c:745)
[ 332.936636] ? __pfx_dcache_dir_open_wrapper (fs/tracefs/event_inode.c:683)
[ 332.941772] do_dentry_open (fs/open.c:929)
[ 332.945646] vfs_open (fs/open.c:1064)
[ 332.948800] path_openat (fs/namei.c:3639 fs/namei.c:3796)
[ 332.952465] ? ___slab_alloc (mm/slub.c:810 mm/slub.c:3265)
[ 332.956384] ? nfs_ctx_key_to_expire (fs/nfs/write.c:1281)
[ 332.960917] ? trace_preempt_on (kernel/trace/trace_preemptirq.c:105)
[ 332.964929] do_filp_open (fs/namei.c:3823)
[ 332.968511] do_sys_openat2 (fs/open.c:1422)
[ 332.972173] __x64_sys_openat (fs/open.c:1448)
[ 332.976014] do_syscall_64 (arch/x86/entry/common.c:50
arch/x86/entry/common.c:80)
[ 332.979601] ? trace_hardirqs_on_prepare
(kernel/trace/trace_preemptirq.c:47 (discriminator 16)
kernel/trace/trace_preemptirq.c:42 (discriminator 16))
[ 332.984436] ? syscall_exit_to_user_mode (kernel/entry/common.c:299)
[ 332.989219] ? do_syscall_64 (arch/x86/entry/common.c:87)
[ 332.992964] ? do_syscall_64 (arch/x86/entry/common.c:87)
[ 332.996709] ? do_syscall_64 (arch/x86/entry/common.c:87)
[ 333.000460] ? do_syscall_64 (arch/x86/entry/common.c:87)
[ 333.004204] ? do_syscall_64 (arch/x86/entry/common.c:87)
[ 333.007948] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[ 333.012994] RIP: 0033:0x7fc5a51fadf1
[ 333.016600] Code: 44 24 18 31 c0 41 83 e2 40 75 3e 89 f0 25 00 00 41
00 3d 00 00 41 00 74 30 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff ff
0f 05 <48> 3d 00 f0 ff ff 77 3f 48 8b 54 24 18 64 48 2b 14 25 28 00 00
00
All code
========
0: 44 24 18 rex.R and $0x18,%al
3: 31 c0 xor %eax,%eax
5: 41 83 e2 40 and $0x40,%r10d
9: 75 3e jne 0x49
b: 89 f0 mov %esi,%eax
d: 25 00 00 41 00 and $0x410000,%eax
12: 3d 00 00 41 00 cmp $0x410000,%eax
17: 74 30 je 0x49
19: 89 f2 mov %esi,%edx
1b: b8 01 01 00 00 mov $0x101,%eax
20: 48 89 fe mov %rdi,%rsi
23: bf 9c ff ff ff mov $0xffffff9c,%edi
28: 0f 05 syscall
2a:* 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax <--
trapping instruction
30: 77 3f ja 0x71
32: 48 8b 54 24 18 mov 0x18(%rsp),%rdx
37: 64 48 2b 14 25 28 00 sub %fs:0x28,%rdx
3e: 00 00
Code starting with the faulting instruction
===========================================
0: 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax
6: 77 3f ja 0x47
8: 48 8b 54 24 18 mov 0x18(%rsp),%rdx
d: 64 48 2b 14 25 28 00 sub %fs:0x28,%rdx
14: 00 00
[ 333.035361] RSP: 002b:00007ffebef93680 EFLAGS: 00000287 ORIG_RAX:
0000000000000101
[ 333.042919] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fc5a51fadf1
[ 333.050042] RDX: 0000000000090800 RSI: 0000562c92f2cc20 RDI: 00000000ffffff9c
[ 333.057166] RBP: 0000000000000000 R08: 0000000000000007 R09: 0000562c92f2cc90
[ 333.064290] R10: 0000000000000000 R11: 0000000000000287 R12: 00007fc5a5062b98
[ 333.071415] R13: 00007ffebef93ad0 R14: 0000562c92f2cc20 R15: 0000562c9122ffc8
[ 333.078541] </TASK>
[ 333.080733] Modules linked in: x86_pkg_temp_thermal fuse configfs
ip_tables x_tables [last unloaded: trace_printk]
[ 333.091109] ---[ end trace 0000000000000000 ]---
[ 333.095757] RIP: 0010:event_callback (kernel/trace/trace_events.c:2439)
[ 333.100295] Code: f6 48 c7 c6 09 78 6b a0 41 55 49 89 cd 41 54 49 89
d4 53 48 8b 02 48 89 fb 4c 8b 78 10 e8 10 8b 19 01 85 c0 0f 84 d2 00
00 00 <41> 8b 47 60 a8 08 0f 85 9c 00 00 00 49 8b 47 10 48 83 78 18 00
74
All code
========
0: f6 48 c7 c6 testb $0xc6,-0x39(%rax)
4: 09 78 6b or %edi,0x6b(%rax)
7: a0 41 55 49 89 cd 41 movabs 0x495441cd89495541,%al
e: 54 49
10: 89 d4 mov %edx,%esp
12: 53 push %rbx
13: 48 8b 02 mov (%rdx),%rax
16: 48 89 fb mov %rdi,%rbx
19: 4c 8b 78 10 mov 0x10(%rax),%r15
1d: e8 10 8b 19 01 callq 0x1198b32
22: 85 c0 test %eax,%eax
24: 0f 84 d2 00 00 00 je 0xfc
2a:* 41 8b 47 60 mov 0x60(%r15),%eax <-- trapping instruction
2e: a8 08 test $0x8,%al
30: 0f 85 9c 00 00 00 jne 0xd2
36: 49 8b 47 10 mov 0x10(%r15),%rax
3a: 48 83 78 18 00 cmpq $0x0,0x18(%rax)
3f: 74 .byte 0x74
Code starting with the faulting instruction
===========================================
0: 41 8b 47 60 mov 0x60(%r15),%eax
4: a8 08 test $0x8,%al
6: 0f 85 9c 00 00 00 jne 0xa8
c: 49 8b 47 10 mov 0x10(%r15),%rax
10: 48 83 78 18 00 cmpq $0x0,0x18(%rax)
15: 74 .byte 0x74
[ 333.119040] RSP: 0018:ffffba5700967b50 EFLAGS: 00010202
[ 333.124290] RAX: 0000000000000001 RBX: ffffffffa06fb5d7 RCX: ffffba5700967bc8
[ 333.131451] RDX: 0000000000000069 RSI: ffffffffa06b7809 RDI: ffffffffa06fb5d7
[ 333.138623] RBP: ffffba5700967b78 R08: ffff8dbe12250134 R09: 0000000000000000
[ 333.145811] R10: ffff8dbe122500e0 R11: ffff8dbe0d8f8fc8 R12: ffffba5700967bd0
[ 333.152969] R13: ffffba5700967bc8 R14: ffffba5700967bc6 R15: 6b6b6b6b6b6b6b6b
[ 333.160127] FS: 00007fc5a5062d00(0000) GS:ffff8dc167a80000(0000)
knlGS:0000000000000000
[ 333.168215] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 333.173961] CR2: 00007f7f8dc50a1c CR3: 0000000113c84003 CR4: 00000000003706e0
[ 333.181092] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 333.188251] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
# ok 47 Test creation and deletion of trace instances
Log link:
- https://lkft.validation.linaro.org/scheduler/job/6983932#L1655
- https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/tuxmake_reproducer.sh
metadata:
git_ref: master
git_repo: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
git_ref: trace/core
kernel-config:
https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/config
artifact-location:
https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/
toolchain: gcc-13
--
Linaro LKFT
https://lkft.linaro.org
> -- Steve
On Mon, 30 Oct 2023 12:37:08 +0530
Naresh Kamboju <[email protected]> wrote:
>
> I have tested the linux-trace.git trace/core and run selftests ftrace
> the reported kernel panic [1] & [2] has been fixed but found
Good to know. Can I add "Tested-by" from you for that bug fix?
> "general protection fault" at kernel/trace/trace_events.c:2439.
Can you test with the below patch?
Also, can I ask what are you testing this on that makes it trigger so
easily? As I'm not able to trigger these in my tests, even though they are
indeed bugs.
-- Steve
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 7ad7496bd597..7a0b54ddda24 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
entry = &ei->entries[i];
if (strcmp(name, entry->name) == 0) {
void *cdata = data;
- r = entry->callback(name, &mode, &cdata, &fops);
+ mutex_lock(&eventfs_mutex);
+ /* If ei->is_freed, then the event itself may be too */
+ if (!ei->is_freed)
+ r = entry->callback(name, &mode, &cdata, &fops);
+ else
+ r = -1;
+ mutex_unlock(&eventfs_mutex);
if (r <= 0)
continue;
ret = simple_lookup(dir, dentry, flags);
@@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
void *cdata = data;
entry = &ei->entries[i];
name = entry->name;
- r = entry->callback(name, &mode, &cdata, &fops);
+ mutex_lock(&eventfs_mutex);
+ /* If ei->is_freed, then the event itself may be too */
+ if (!ei->is_freed)
+ r = entry->callback(name, &mode, &cdata, &fops);
+ else
+ r = -1;
+ mutex_unlock(&eventfs_mutex);
if (r <= 0)
continue;
d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);
On Mon, 30 Oct 2023 at 20:11, Steven Rostedt <[email protected]> wrote:
>
> On Mon, 30 Oct 2023 12:37:08 +0530
> Naresh Kamboju <[email protected]> wrote:
>
> >
> > I have tested the linux-trace.git trace/core and run selftests ftrace
> > the reported kernel panic [1] & [2] has been fixed but found
>
> Good to know. Can I add "Tested-by" from you for that bug fix?
Please add,
Tested-by: Linux Kernel Functional Testing <[email protected]>
Tested-by: Naresh Kamboju <[email protected]>
>
> > "general protection fault" at kernel/trace/trace_events.c:2439.
>
> Can you test with the below patch?
Applied this patch on top of linux-trace.git trace/core and test passed.
All the reported issues have been resolved.
>
> Also, can I ask what are you testing this on that makes it trigger so
> easily? As I'm not able to trigger these in my tests, even though they are
> indeed bugs.
I use following build artifacts and running selftests: ftrace: ftracetest-ktap
kernel:
url:
https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/bzImage
modules:
url:
https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/modules.tar.xz
compression: xz
nfsrootfs:
url: https://storage.tuxboot.com/debian/bookworm/amd64/rootfs.tar.xz
kselftest:
url:
https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/kselftest.tar.xz
Steps to test:
==========
1) Boot x86 machine with above bzImage, modules, rootfs and
copy kselftest.tar.xz from above url and unzip.
2) cd /opt/kselftests/default-in-kernel/ftrace
3) ./run_kselftest.sh -c ftrace
# selftests: ftrace: ftracetest-ktap
# unlink: cannot unlink
'/opt/kselftests/default-in-kernel/ftrace/logs/latest': No such file
or directory
# TAP version 13
# 1..129
# ok 1 Basic trace file check
...
# # Totals: pass:126 faii:0 xfail:1 xpass:0 skip:1 error:1
Test logs:
https://lkft.validation.linaro.org/scheduler/job/6985018#L4742
>
> -- Steve
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 7ad7496bd597..7a0b54ddda24 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> entry = &ei->entries[i];
> if (strcmp(name, entry->name) == 0) {
> void *cdata = data;
> - r = entry->callback(name, &mode, &cdata, &fops);
> + mutex_lock(&eventfs_mutex);
> + /* If ei->is_freed, then the event itself may be too */
> + if (!ei->is_freed)
> + r = entry->callback(name, &mode, &cdata, &fops);
> + else
> + r = -1;
> + mutex_unlock(&eventfs_mutex);
> if (r <= 0)
> continue;
> ret = simple_lookup(dir, dentry, flags);
> @@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
> void *cdata = data;
> entry = &ei->entries[i];
> name = entry->name;
> - r = entry->callback(name, &mode, &cdata, &fops);
> + mutex_lock(&eventfs_mutex);
> + /* If ei->is_freed, then the event itself may be too */
> + if (!ei->is_freed)
> + r = entry->callback(name, &mode, &cdata, &fops);
> + else
> + r = -1;
> + mutex_unlock(&eventfs_mutex);
> if (r <= 0)
> continue;
> d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);