[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <178d8e10-1dd8-48de-858f-1a04c419c331@grsecurity.net>
Date: Wed, 7 Aug 2024 22:19:46 +0200
From: Mathias Krause <minipli@...ecurity.net>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Ajay Kaher <ajay.kaher@...adcom.com>, linux-trace-kernel@...r.kernel.org,
linux-kernel@...r.kernel.org, Ilkka Naulapää
<digirigawa@...il.com>, Al Viro <viro@...iv.linux.org.uk>,
Brad Spengler <spender@...ecurity.net>
Subject: Re: [PATCH 2/2] tracefs: Don't overlay 'struct inode'
On 07.08.24 15:35, Steven Rostedt wrote:
> On Wed, 7 Aug 2024 13:51:39 +0200
> Mathias Krause <minipli@...ecurity.net> wrote:
>
>> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
>> index f704d8348357..a7769857962a 100644
>> --- a/fs/tracefs/internal.h
>> +++ b/fs/tracefs/internal.h
>> @@ -10,10 +10,8 @@ enum {
>> };
>>
>> struct tracefs_inode {
>> - union {
>> - struct inode vfs_inode;
>> - struct rcu_head rcu;
>> - };
>> + struct inode vfs_inode;
>> + struct rcu_head rcu;
>
> 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@...11-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.
>
>> /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
>> struct list_head list;
>> unsigned long flags;
>
> Perhaps:
>
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index f704d8348357..ab6d6c3d835d 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -10,12 +10,12 @@ enum {
> };
>
> struct tracefs_inode {
> + struct inode vfs_inode;
> + /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
> union {
> - struct inode vfs_inode;
> + struct list_head list;
> struct rcu_head rcu;
> };
> - /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
> - struct list_head list;
> unsigned long flags;
> 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
Powered by blists - more mailing lists