On Sun, Sep 14, 2025 at 9:10 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Sat, Sep 13, 2025 at 3:27 PM David Windsor <dwindsor@xxxxxxxxx> wrote: > > > > > > > > On Sat, Sep 13, 2025 at 5:58 PM Song Liu <song@xxxxxxxxxx> wrote: > >> > >> On Fri, Sep 12, 2025 at 5:27 PM David Windsor <dwindsor@xxxxxxxxx> wrote: > >> [...] > >> > > > >> > > Maybe I missed something, but I think you haven't addressed Alexei's > >> > > question in v1: why this is needed and why hash map is not sufficient. > >> > > > >> > > Other local storage types (task, inode, sk storage) may get a large > >> > > number of entries in a system, and thus would benefit from object > >> > > local storage. I don't think we expect too many creds in a system. > >> > > hash map of a smallish size should be good in most cases, and be > >> > > faster than cred local storage. > >> > > > >> > > Did I get this right? > >> > > > >> > > Thanks, > >> > > Song > >> > > > >> > > >> > Yes I think I addressed in the cover letter of -v2: > >> > > >> > "Like other local storage types (task, inode, sk), this provides automatic > >> > lifecycle management and is useful for LSM programs tracking credential > >> > state across LSM calls. Lifetime management is necessary for detecting > >> > credential leaks and enforcing time-based security policies." > >> > > >> > You're right it's faster and there aren't many creds, but I feel like > >> > in this case, it'll be a nightmare to manual cleanup with hashmaps. I > >> > think the correctness we get with lifetime management is worth it in > >> > this case, but could be convinced otherwise. Many cred usage patterns > >> > are short lived and a hash map could quickly become stale... > >> > >> We can clean up the hashmap in hook cred_free, no? The following > >> check in security_cred_free() seems problematic: > >> > >> if (unlikely(cred->security == NULL)) > >> return; > >> > >> But as far as I can tell, it is not really useful, and can be removed. > >> With this removed, hash map will work just as well. Did I miss > >> something? > > > > > > No I think actually this is easier. > > > > I will prepare a patch for the race in cleanup I stumbled on earlier which is still there and could affect other users. > > > > That said, is there any use case for local storage for these structs: > > > > - struct file > > - struct msg_msg > > - struct ipc > > > > I can off the top of my head think of some security use cases for these but not sure if hashmaps are needed, perhaps struct file > > Sorry, no. This is not a copy paste territory. no i get it's not copy/paste but I have the series for struct file ready for submission, with selftests. this is also a performance critical use case and there will be numerous struct file on edge servers. > The existing local storage maps were added because > performance was critical for those use cases, > but we made a few mistakes. There is a performance > cliff that has to be fixed before we adopt it to > other kernel objects. ahh wasn't aware of this. > Please use hash map and consider wrapping rhashtable > as a new bpf map type if fixed max_entries is problematic. > makes sense thanks > pw-bot: cr
