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. 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. Please use hash map and consider wrapping rhashtable as a new bpf map type if fixed max_entries is problematic. pw-bot: cr
