[PATCH bpf-next 2/4] bpf: Make bpf inode storage available to tracing program

[Song Liu]

> On Nov 20, 2024, at 1:28 AM, Christian Brauner <brauner at kernel.org> wrote:

[...]

>>>>> Then whenever you have to populate any of these fields, you just
>>>>> allocate one of these structs and set the inode up to point to it.
>>>>> They're tiny too, so don't bother freeing it until the inode is
>>>>> deallocated.
>>>>> 
>>>>> It'd mean rejiggering a fair bit of fsnotify code, but it would give
>>>>> the fsnotify code an easier way to expand per-inode info in the future.
>>>>> It would also slightly shrink struct inode too.
>> 
>> I am hoping to make i_bpf_storage available to tracing programs. 
>> Therefore, I would rather not limit it to fsnotify context. We can
>> still use the universal on-demand allocator.
> 
> Can't we just do something like:
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7e29433c5ecc..cc05a5485365 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -627,6 +627,12 @@ is_uncached_acl(struct posix_acl *acl)
> #define IOP_DEFAULT_READLINK   0x0010
> #define IOP_MGTIME     0x0020
> 
> +struct inode_addons {
> +        struct fsnotify_mark_connector __rcu    *i_fsnotify_marks;
> +        struct bpf_local_storage __rcu          *i_bpf_storage;
> +        __u32                                   i_fsnotify_mask; /* all events this inode cares about */
> +};
> +
> /*
>  * Keep mostly read-only and often accessed (especially for
>  * the RCU path lookup and 'stat' data) fields at the beginning
> @@ -731,12 +737,7 @@ struct inode {
>                unsigned                i_dir_seq;
>        };
> 
> -
> -#ifdef CONFIG_FSNOTIFY
> -       __u32                   i_fsnotify_mask; /* all events this inode cares about */
> -       /* 32-bit hole reserved for expanding i_fsnotify_mask */
> -       struct fsnotify_mark_connector __rcu    *i_fsnotify_marks;
> -#endif
> +       struct inode_addons *i_addons;
> 
> #ifdef CONFIG_FS_ENCRYPTION
>        struct fscrypt_inode_info       *i_crypt_info;
> 
> Then when either fsnotify or bpf needs that storage they can do a
> cmpxchg() based allocation for struct inode_addons just like I did with
> f_owner:
> 
> int file_f_owner_allocate(struct file *file)
> {
> struct fown_struct *f_owner;
> 
> f_owner = file_f_owner(file);
> if (f_owner)
> return 0;
> 
> f_owner = kzalloc(sizeof(struct fown_struct), GFP_KERNEL);
> if (!f_owner)
> return -ENOMEM;
> 
> rwlock_init(&f_owner->lock);
> f_owner->file = file;
> /* If someone else raced us, drop our allocation. */
> if (unlikely(cmpxchg(&file->f_owner, NULL, f_owner)))
> kfree(f_owner);
> return 0;
> }
> 
> The internal allocations for specific fields are up to the subsystem
> ofc. Does that make sense?

This works for fsnotify/fanotify. However, for tracing use cases, 
this is not as reliable as other (task, cgroup, sock) local storage. 
BPF tracing programs need to work in any contexts, including NMI. 
Therefore, doing kzalloc(GFP_KERNEL) is not always safe for 
tracing use cases. OTOH, bpf local storage works in NMI. If we have 
a i_bpf_storage pointer in struct inode, bpf inode storage will work 
in NMI. 

Thanks,
Song