Re: [PATCH bpf-next 0/4] Make inode storage available to tracing prog

From: Song Liu
Date: Thu Nov 14 2024 - 17:30:54 EST


Hi James,

Thanks for your input!

> On Nov 14, 2024, at 1:49 PM, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:

[...]

>>
>> Actually, I can understand the concern with union. Although,
>> the logic is set at kernel compile time, it is still possible
>> for kernel source code to use i_bpf_storage when
>> CONFIG_SECURITY is enabled. (Yes, I guess now I finally understand
>> the concern).
>>
>> 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.

Yes, as I described in another email in the thread [1], this turned
out to cause more troubles than I thought.

> 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?

I think this is a very good idea. If folks agree with this approach,
I am more than happy to draft patch for this.

Thanks again,

Song

> 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.

[1] https://lore.kernel.org/linux-fsdevel/86C65B85-8167-4D04-BFF5-40FD4F3407A4@xxxxxx/