Message-ID: <561687f7-b7f3-4d56-a54c-944c52ed18b7@schaufler-ca.com>
Date: Tue, 19 Nov 2024 10:14:29 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: "Dr. Greg" <greg@...ellic.com>, Song Liu <songliubraving@...a.com>
Cc: James Bottomley <James.Bottomley@...senPartnership.com>,
 "jack@...e.cz" <jack@...e.cz>, "brauner@...nel.org" <brauner@...nel.org>,
 Song Liu <song@...nel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
 "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-security-module@...r.kernel.org"
 <linux-security-module@...r.kernel.org>, Kernel Team <kernel-team@...a.com>,
 "andrii@...nel.org" <andrii@...nel.org>,
 "eddyz87@...il.com" <eddyz87@...il.com>, "ast@...nel.org" <ast@...nel.org>,
 "daniel@...earbox.net" <daniel@...earbox.net>,
 "martin.lau@...ux.dev" <martin.lau@...ux.dev>,
 "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
 "kpsingh@...nel.org" <kpsingh@...nel.org>,
 "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>,
 "amir73il@...il.com" <amir73il@...il.com>,
 "repnop@...gle.com" <repnop@...gle.com>,
 "jlayton@...nel.org" <jlayton@...nel.org>, Josef Bacik
 <josef@...icpanda.com>, "mic@...ikod.net" <mic@...ikod.net>,
 "gnoack@...gle.com" <gnoack@...gle.com>,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH bpf-next 0/4] Make inode storage available to tracing prog

On 11/19/2024 4:27 AM, Dr. Greg wrote:
> On Sun, Nov 17, 2024 at 10:59:18PM +0000, Song Liu wrote:
>
>> Hi Christian, James and Jan, 
> Good morning, I hope the day is starting well for everyone.
>
>>> On Nov 14, 2024, at 1:49???PM, James Bottomley <James.Bottomley@...senPartnership.com> wrote:
>> [...]
>>
>>>> We can address this with something like following:
>>>>
>>>> #ifdef CONFIG_SECURITY
>>>>         void                    *i_security;
>>>> #elif CONFIG_BPF_SYSCALL
>>>>         struct bpf_local_storage __rcu *i_bpf_storage;
>>>> #endif
>>>>
>>>> This will help catch all misuse of the i_bpf_storage at compile
>>>> time, as i_bpf_storage doesn't exist with CONFIG_SECURITY=y. 
>>>>
>>>> Does this make sense?
>>> Got to say I'm with Casey here, this will generate horrible and failure
>>> prone code.
>>>
>>> Since effectively you're making i_security always present anyway,
>>> simply do that and also pull the allocation code out of security.c in a
>>> way that it's always available?  That way you don't have to special
>>> case the code depending on whether CONFIG_SECURITY is defined. 
>>> Effectively this would give everyone a generic way to attach some
>>> memory area to an inode.  I know it's more complex than this because
>>> there are LSM hooks that run from security_inode_alloc() but if you can
>>> make it work generically, I'm sure everyone will benefit.
>> On a second thought, I think making i_security generic is not 
>> the right solution for "BPF inode storage in tracing use cases". 
>>
>> This is because i_security serves a very specific use case: it 
>> points to a piece of memory whose size is calculated at system 
>> boot time. If some of the supported LSMs is not enabled by the 
>> lsm= kernel arg, the kernel will not allocate memory in 
>> i_security for them. The only way to change lsm= is to reboot 
>> the system. BPF LSM programs can be disabled at the boot time, 
>> which fits well in i_security. However, BPF tracing programs 
>> cannot be disabled at boot time (even we change the code to 
>> make it possible, we are not likely to disable BPF tracing). 
>> IOW, as long as CONFIG_BPF_SYSCALL is enabled, we expect some 
>> BPF tracing programs to load at some point of time, and these 
>> programs may use BPF inode storage. 
>>
>> Therefore, with CONFIG_BPF_SYSCALL enabled, some extra memory 
>> always will be attached to i_security (maybe under a different 
>> name, say, i_generic) of every inode. In this case, we should 
>> really add i_bpf_storage directly to the inode, because another 
>> pointer jump via i_generic gives nothing but overhead. 
>>
>> Does this make sense? Or did I misunderstand the suggestion?
> There is a colloquialism that seems relevant here: "Pick your poison".
>
> In the greater interests of the kernel, it seems that a generic
> mechanism for attaching per inode information is the only realistic
> path forward, unless Christian changes his position on expanding
> the size of struct inode.
>
> There are two pathways forward.
>
> 1.) Attach a constant size 'blob' of storage to each inode.
>
> This is a similar approach to what the LSM uses where each blob is
> sized as follows:
>
> S = U * sizeof(void *)
>
> Where U is the number of sub-systems that have a desire to use inode
> specific storage.

I can't tell for sure, but it looks like you don't understand how
LSM i_security blobs are used. It is *not* the case that each LSM
gets a pointer in the i_security blob. Each LSM that wants storage
tells the infrastructure at initialization time how much space it
wants in the blob. That can be a pointer, but usually it's a struct
with flags, pointers and even lists.

> Each sub-system uses it's pointer slot to manage any additional
> storage that it desires to attach to the inode.

Again, an LSM may choose to do it that way, but most don't.
SELinux and Smack need data on every inode. It makes much more sense
to put it directly in the blob than to allocate a separate chunk
for every inode.

> This has the obvious advantage of O(1) cost complexity for any
> sub-system that wants to access its inode specific storage.
>
> The disadvantage, as you note, is that it wastes memory if a
> sub-system does not elect to attach per inode information, for example
> the tracing infrastructure.

To be clear, that disadvantage only comes up if the sub-system uses
inode data on an occasional basis. If it never uses inode data there
is no need to have a pointer to it.

> This disadvantage is parried by the fact that it reduces the size of
> the inode proper by 24 bytes (4 pointers down to 1) and allows future
> extensibility without colliding with the interests and desires of the
> VFS maintainers.

You're adding a level of indirection. Even I would object based on
the performance impact.

> 2.) Implement key/value mapping for inode specific storage.
>
> The key would be a sub-system specific numeric value that returns a
> pointer the sub-system uses to manage its inode specific memory for a
> particular inode.
>
> A participating sub-system in turn uses its identifier to register an
> inode specific pointer for its sub-system.
>
> This strategy loses O(1) lookup complexity but reduces total memory
> consumption and only imposes memory costs for inodes when a sub-system
> desires to use inode specific storage.

SELinux and Smack use an inode blob for every inode. The performance
regression boggles the mind. Not to mention the additional complexity
of managing the memory.

> Approach 2 requires the introduction of generic infrastructure that
> allows an inode's key/value mappings to be located, presumably based
> on the inode's pointer value.  We could probably just resurrect the
> old IMA iint code for this purpose.
>
> In the end it comes down to a rather standard trade-off in this
> business, memory vs. execution cost.
>
> We would posit that option 2 is the only viable scheme if the design
> metric is overall good for the Linux kernel eco-system.

No. Really, no. You need look no further than secmarks to understand
how a key based blob allocation scheme leads to tears. Keys are fine
in the case where use of data is sparse. They have no place when data
use is the norm.

>> Thanks,
>> Song
> Have a good day.
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity
>               https://github.com/Quixote-Project
>

Powered by blists - more mailing lists