Hi Steven,
We ran into yet another tracefs related bug but, fortunately, were able
to root cause it ourselves.
The problem only occurs when CONFIG_RANDSTRUCT is enabled and one gets
(un)lucky to hit a random seed that'll overlay the 'rcu' member of the
union with a list_head in 'vfs_inode' -- quite unlikely but, apparently,
we're exceptional "lucky" with our testing ;)
The first patch is more of an API correctness fix, to bring the tracefs
inode cache in line with all the other filesystems. The second patch
actually fixes the bug, which, I think, may also be the cause for what
Ilkka is seeing[1].
Please apply!
Thanks,
Mathias
[1] https://lore.kernel.org/all/CAE4VaREzY+a2PvQJYJbfh8DwB4OP7kucZG-e28H22xyWob1w_A@mail.gmail.com/
Mathias Krause (2):
tracefs: Fix inode allocation
tracefs: Don't overlay 'struct inode'
fs/tracefs/inode.c | 2 +-
fs/tracefs/internal.h | 6 ++----
2 files changed, 3 insertions(+), 5 deletions(-)
--
2.43.0
The leading comment above alloc_inode_sb() is pretty explicit about it:
/*
* This must be used for allocating filesystems specific inodes to set
* up the inode reclaim context correctly.
*/
Switch tracefs over to alloc_inode_sb() to make sure inodes are properly
linked.
Cc: Ajay Kaher <ajay.kaher@broadcom.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Fixes: ba37ff75e04b ("eventfs: Implement tracefs_inode_cache")
Signed-off-by: Mathias Krause <redacted>
---
fs/tracefs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-08-07 13:33:28
On Wed, 7 Aug 2024 13:51:37 +0200
Mathias Krause [off-list ref] wrote:
Hi Steven,
We ran into yet another tracefs related bug but, fortunately, were able
to root cause it ourselves.
The problem only occurs when CONFIG_RANDSTRUCT is enabled and one gets
(un)lucky to hit a random seed that'll overlay the 'rcu' member of the
union with a list_head in 'vfs_inode' -- quite unlikely but, apparently,
we're exceptional "lucky" with our testing ;)
The first patch is more of an API correctness fix, to bring the tracefs
inode cache in line with all the other filesystems. The second patch
actually fixes the bug, which, I think, may also be the cause for what
Ilkka is seeing[1].
Ah, that would explain it, and why I never triggered it.
@@ -10,12 +10,12 @@ enum {};structtracefs_inode{+structinodevfs_inode;+/* The below gets initialized with memset_after(ti, 0, vfs_inode) */union{-structinodevfs_inode;+structlist_headlist;structrcu_headrcu;};-/* The below gets initialized with memset_after(ti, 0, vfs_inode) */-structlist_headlist;unsignedlongflags;void*private;};
Your current variant gives you an RCU-delayed call of
tracefs_free_inode(), which schedules an RCU-delayed call of
tracefs_free_inode_rcu().
Do you really need that double RCU delay to start with?
Because if you do not, just do that list_del_rcu() in ->destroy_inode()
(which is called without an RCU delay) and have kmem_cache_free()
in ->free_inode() (which is called *with* RCU delay started after
the call of ->destroy_inode()).
@@ -10,12 +10,12 @@ enum {};structtracefs_inode{+structinodevfs_inode;+/* The below gets initialized with memset_after(ti, 0, vfs_inode) */union{-structinodevfs_inode;+structlist_headlist;structrcu_headrcu;};-/* The below gets initialized with memset_after(ti, 0, vfs_inode) */-structlist_headlist;unsignedlongflags;void*private;};
Your current variant gives you an RCU-delayed call of
tracefs_free_inode(), which schedules an RCU-delayed call of
tracefs_free_inode_rcu().
Do you really need that double RCU delay to start with?
Because if you do not, just do that list_del_rcu() in ->destroy_inode()
(which is called without an RCU delay) and have kmem_cache_free()
in ->free_inode() (which is called *with* RCU delay started after
the call of ->destroy_inode()).
Thanks, I didn't know about these.
So I could use destroy_inode() for the removing of the link list, and then
free_inode to free it. Something like:
@@ -10,10 +10,7 @@ enum {};structtracefs_inode{-union{-structinodevfs_inode;-structrcu_headrcu;-};+structinodevfs_inode;/* The below gets initialized with memset_after(ti, 0, vfs_inode) */structlist_headlist;unsignedlongflags;
I'll run this under some more tests and see if it doesn't crash.
I'll apply the first patch of this series too, and then probably use this
one.
-- Steve
I rather not make this structure any bigger for the rcu element that is not
used until freed.
Uhm, at least for my config, it won't consume more memory, as the slab
object is big enough to cover up for the additional two machine words:
root@deb11-amd64:~# slabinfo tracefs_inode_cache
Slabcache: tracefs_inode_cache Aliases: 0 Order : 3 Objects: 144
** Reclaim accounting active
Sizes (bytes) Slabs Debug Memory
------------------------------------------------------------------------
Object : 1200 Total : 6 Sanity Checks : Off Total: 196608
SlabObj: 1328 Full : 4 Redzoning : Off Used : 172800
SlabSiz: 32768 Partial: 0 Poisoning : Off Loss : 23808
Loss : 128 CpuSlab: 2 Tracking : Off Lalig: 18432
Align : 8 Objects: 24 Tracing : Off Lpadd: 5376
[...]
While the size of 'struct tracefs_inode' is 1200 bytes for my kernel
build (LOCKDEP bloats it quite a lot), the slab object size is 1328
bytes, i.e. 128 bytes wasted per object which can, for sure, cover up
for these additional members.
quoted hunk
quoted
/* The below gets initialized with memset_after(ti, 0, vfs_inode) */
struct list_head list;
unsigned long flags;
@@ -10,12 +10,12 @@ enum {};structtracefs_inode{+structinodevfs_inode;+/* The below gets initialized with memset_after(ti, 0, vfs_inode) */union{-structinodevfs_inode;+structlist_headlist;structrcu_headrcu;};-/* The below gets initialized with memset_after(ti, 0, vfs_inode) */-structlist_headlist;unsignedlongflags;void*private;};
I'd rather not exchange trashing one RCU-walked list for another. Or how
will this play out for the RCU walk in tracefs_apply_options() if
there's a concurrent call to tracefs_free_inode() which will now trash
the list_head tracefs_apply_options() is walking over?
Thanks,
Mathias
@@ -10,12 +10,12 @@ enum {};structtracefs_inode{+structinodevfs_inode;+/* The below gets initialized with memset_after(ti, 0, vfs_inode) */union{-structinodevfs_inode;+structlist_headlist;structrcu_headrcu;};-/* The below gets initialized with memset_after(ti, 0, vfs_inode) */-structlist_headlist;unsignedlongflags;void*private;};
Your current variant gives you an RCU-delayed call of
tracefs_free_inode(), which schedules an RCU-delayed call of
tracefs_free_inode_rcu().
Do you really need that double RCU delay to start with?
Because if you do not, just do that list_del_rcu() in ->destroy_inode()
(which is called without an RCU delay) and have kmem_cache_free()
in ->free_inode() (which is called *with* RCU delay started after
the call of ->destroy_inode()).
Jepp, sounds much better indeed and doesn't require 'struct
tracefs_inode' to have its own 'struct rcu_head' member.
Thanks,
Mathias