Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Nov 2022 15:54:17 -0600
From:   David Vernet <void@manifault.com>
To:     John Fastabend <john.fastabend@gmail.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
        daniel@iogearbox.net, martin.lau@linux.dev, memxor@gmail.com,
        yhs@fb.com, song@kernel.org, sdf@google.com, kpsingh@kernel.org,
        jolsa@kernel.org, haoluo@google.com, tj@kernel.org,
        kernel-team@fb.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v7 0/3] Support storing struct task_struct
 objects as kptrs
Message-ID: <Y3atifGs0DM9to8z@maniforge.lan>
References: <20221117032402.2356776-1-void@manifault.com>
 <6376a1b12bb4d_4101208d@john.notmuch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6376a1b12bb4d_4101208d@john.notmuch>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk

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

[...]