On Thu, Nov 17, 2022 at 01:03:45PM -0800, John Fastabend wrote: > David Vernet wrote: > > Now that BPF supports adding new kernel functions with kfuncs, and > > storing kernel objects in maps with kptrs, we can add a set of kfuncs > > which allow struct task_struct objects to be stored in maps as > > referenced kptrs. > > > > The possible use cases for doing this are plentiful. During tracing, > > for example, it would be useful to be able to collect some tasks that > > performed a certain operation, and then periodically summarize who they > > are, which cgroup they're in, how much CPU time they've utilized, etc. > > Doing this now would require storing the tasks' pids along with some > > relevant data to be exported to user space, and later associating the > > pids to tasks in other event handlers where the data is recorded. > > Another useful by-product of this is that it allows a program to pin a > > task in a BPF program, and by proxy therefore also e.g. pin its task > > local storage. > > Sorry wasn't obvious to me (late to the party so if it was in some > other v* described apologies). Can we say something about the life > cycle of this acquired task_structs because they are incrementing > the ref cnt on the task struct they have potential to impact system. We should probably add an entire docs page which describes how kptrs work, and I am happy to do that (ideally in a follow-on patch set if that's OK with you). In general I think it would be useful to include docs for any general-purpose kfuncs like the ones proposed in this set. In regards to your specific question about the task lifecycle, nothing being proposed in this patch set differs from how kptr lifecycles work in general. The idea is that the BPF program: 1. Gets a "kptr_ref" kptr from an "acquire" kfunc. 2. Stores it in a map with bpf_kptr_xchg(). The program can then either later manually extract it from the map (again with bpf_kptr_xchg()) and release it, or if the reference is never removed from the map, let it be automatically released when the map is destroyed. See [0] and [1] for a bit more information. [0]: https://docs.kernel.org/bpf/kfuncs.html?highlight=kptr#kf-acquire-flag [1]: https://lwn.net/Articles/900749/ > I know at least we've had a few bugs in our task struct tracking > that has led to various bugs where we leak references. In our case > we didn't pin the kernel object so the leak is just BPF memory and > user space memory, still sort of bad because we would hit memory > limits and get OOM'd. Leaking kernel task structs is worse though. I don't think we need to worry about leaks. The verifier should fail to load any program that doesn't properly release a kptr, and if it fails to identify programs that improperly hold refcounts, that's a bug that needs to be fixed. Similarly, if any map implementation (described below) fails to properly free references at the appropriate time (when an element or the map itself is destroyed), those are just bugs that need to be fixed. I think the relevant tradeoff here is really the possible side effects of keeping a task pinned and avoiding it being reaped. I agree that's an important consideration, but I think that would arguably apply to any kptr (modulo the size of the object being pinned, which is certainly relevant as well). We already have kptrs for e.g. struct nf_conn [2]. Granted, struct task_struct is significantly larger, but bpf_kptr_xchg() is only enabled for privileged programs, so it seems like a reasonable operation to allow. [2]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/net/netfilter/nf_conntrack_bpf.c#n253 > quick question. If you put acquired task struct in a map what > happens if user side deletes the entry? Presumably this causes the > release to happen and the task_struct is good to go. Did I miss > the logic? I was thinking you would have something in bpf_map_free_kptrs > and a type callback to release() the refcnt? Someone else can chime in here to correct me if I'm wrong, but AFAIU this is handled by the map implementations calling out to bpf_obj_free_fields() to invoke the kptr destructor when the element is destroyed. See [3] and [4] for examples of where they're called from the arraymap and hashmap logic respectively. This is how the destructors are similarly invoked when the maps are destroyed. [3]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/arraymap.c#n431 [4]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/hashtab.c#n764 [...]
