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

[Christian Brauner <brauner@kernel.org>]

On Wed, Nov 20, 2024 at 12:19:51PM +0100, Amir Goldstein wrote:
> On Wed, Nov 20, 2024 at 10:28 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, Nov 19, 2024 at 09:53:20PM +0000, Song Liu wrote:
> > > Hi Jeff and Amir,
> > >
> > > Thanks for your inputs!
> > >
> > > > On Nov 19, 2024, at 7:30 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Nov 19, 2024 at 4:25 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >>
> > > >> On Tue, Nov 19, 2024 at 3:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > >>>
> > >
> > > [...]
> > >
> > > >>> Longer term, I think it may be beneficial to come up with a way to attach
> > > >>>>> private info to the inode in a way that doesn't cost us one pointer per
> > > >>>>> funcionality that may possibly attach info to the inode. We already have
> > > >>>>> i_crypt_info, i_verity_info, i_flctx, i_security, etc. It's always a tough
> > > >>>>> call where the space overhead for everybody is worth the runtime &
> > > >>>>> complexity overhead for users using the functionality...
> > > >>>>
> > > >>>> It does seem to be the right long term solution, and I am willing to
> > > >>>> work on it. However, I would really appreciate some positive feedback
> > > >>>> on the idea, so that I have better confidence my weeks of work has a
> > > >>>> better chance to worth it.
> > > >>>>
> > > >>>> Thanks,
> > > >>>> Song
> > > >>>>
> > > >>>> [1] https://github.com/systemd/systemd/blob/main/src/core/bpf/restrict_fs/restrict-fs.bpf.c
> > > >>>
> > > >>> fsnotify is somewhat similar to file locking in that few inodes on the
> > > >>> machine actually utilize these fields.
> > > >>>
> > > >>> For file locking, we allocate and populate the inode->i_flctx field on
> > > >>> an as-needed basis. The kernel then hangs on to that struct until the
> > > >>> inode is freed.
> > >
> > > If we have some universal on-demand per-inode memory allocator,
> > > I guess we can move i_flctx to it?
> > >
> > > >>> We could do something similar here. We have this now:
> > > >>>
> > > >>> #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
> > >
> > > And maybe some fsnotify fields too?
> > >
> > > With a couple users, I think it justifies to have some universal
> > > on-demond allocator.
> > >
> > > >>> What if you were to turn these fields into a pointer to a new struct:
> > > >>>
> > > >>>        struct fsnotify_inode_context {
> > > >>>                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 */
> > > >>>        };
> > > >>>
> > > >>
> > > >> The extra indirection is going to hurt for i_fsnotify_mask
> > > >> it is being accessed frequently in fsnotify hooks, so I wouldn't move it
> > > >> into a container, but it could be moved to the hole after i_state.
> > >
> > > >>> 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?
> >
> 
> Maybe, but as I wrote, i_fsnotify_mask should not be moved out
> of inode struct, because it is accessed in fast paths of fsnotify vfs
> hooks, where we do not want to have to deref another context,
> but i_fsnotify_mask can be moved to the hole after i_state.
> 
> And why stop at i_fsnotify/i_bfp?
> If you go to "addons" why not also move i_security/i_crypt/i_verify?
> Need to have some common rationale behind those decisions.

The rationale is that we need a mechanism to stop bloating our
structures with ever more stuff somehow. What happens to older members
of struct inode is a cleanup matter and then it needs to be seen what
can be moved into a substruct and not mind the additional pointer chase.

It's just a generalization of your proposal in a way because I don't
understand why you would move the bpf stuff into fsnotify specific
parts.