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

[Song Liu <songliubraving@meta.com>]

Hi James, 

Thanks for your input!

> On Nov 14, 2024, at 1:49 PM, James Bottomley <James.Bottomley@HansenPartnership.com> 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@fb.com/