Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED_STORAGE map type and kfuncs

[Song Liu]

On Tue, Sep 16, 2025 at 11:49 AM David Windsor <dwindsor@xxxxxxxxx> wrote:
>
>
>
> On Tue, Sep 16, 2025 at 1:47 PM Song Liu <song@xxxxxxxxxx> wrote:
> >
> > On Tue, Sep 16, 2025 at 9:36 AM David Windsor <dwindsor@xxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 16, 2025 at 12:16 PM Song Liu <song@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Sep 16, 2025 at 8:25 AM David Windsor <dwindsor@xxxxxxxxx> wrote:
> > > > [...]
> > > > > >
> > > > > > makes sense thanks
> > > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thinking about this more, hashmaps are still problematic for this case.
> > > > >
> > > > > Meaning, placing a hook on security_cred_free alone for garbage
> > > > > collection / end-of-life processing isn't enough - we still have to
> > > > > deal with prepare/commit_creds. This flow works by having
> > > > > prepare_creds clone an existing cred object, then commit_creds works
> > > > > by swapping old creds with new one atomically, then later freeing the
> > > > > original cred. If we are not very careful there will be a period of
> > > > > time during which both cred objects could be valid, and I think this
> > > > > is worth the feature alone.
> > > >
> > > > With cred local storage, we still need to deal with prepare/commit creds,
> > > > right? cred local storage only makes sure the storage is allocated and
> > > > freed. The BPF LSM programs still need to initiate the data properly
> > > > based on the policy. IOW, whether we have cred local storage or not,
> > > > it is necessary to handle all the paths that alloc/free the cred. Did I miss
> > > > something here?
> > > >
> > >
> > > Yes each LSM will have to do whatever it feels it should. Some will
> > > initialize their blob's data with one type of data, some another,
> > > depends on the LSM's use case. We're just here to provide the storage
> > > - bpf cannot use the "classic" LSM storage blob.
> > >
> > > I was referring to the fact that if we use a hashmap to track state on
> > > a per-cred basis there may be a period of time when it could be come
> > > stale during the state change from commit -> prepare_creds.
> >
> > I still don't see how cred local storage will make a difference here. If the
> > cred is stale, the data attached to it is also stale. As long as we free the
> > attached data together with the cred, it should just work, no?
> >
>
> If we use local storage, the cred being stale will still be possible but its attached object will at least be consistent.
>
> In this case, a cred object has been replaced with a new instance, but under RCU readers may still legally see the old pointer for some time.
>
> If we're tracking state in a side map keyed by the pointer and you’ve already removed the old entry (after copying state to the new one), those readers will get a miss even though the old object is still valid:
>
> CPU0 (commit_creds)        CPU1 (BPF hook)         Hashmap
> -------------------------  ----------------------  ----------------------------
> task->cred = old_pointer                           entry[old_pointer] = blob_old
>
> rcu_assign_pointer(new_pointer)
>
>                            still sees old_pointer
>                            lookup(old_pointer) -> blob_old
>
> map_update(new_pointer, blob_old)                  entry[new_pointer] = blob_old
> map_delete(old_pointer)                            del entry[old_pointer]
>
>                            still sees old_pointer
>                            lookup(old_pointer) -> NULL (this is a failure)
>

This is a problem because we are calling map_delete(old_pointer)
at commit_creds time. If we instead calls map_delete(old_pointer)
at security_cred_free, it will be the same as cred local storage.

Does this make sense?

Thanks,
Song