Message-ID: <Y0TXs5Um506K+vSg@maniforge.dhcp.thefacebook.com>
Date:   Mon, 10 Oct 2022 21:40:51 -0500
From:   David Vernet <void@...ifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     Martin KaFai Lau <martin.lau@...ux.dev>, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, kernel-team@...com,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org, yhs@...com,
        song@...nel.org, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, tj@...nel.org
Subject: Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs

On Tue, Oct 11, 2022 at 07:59:29AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Tue, 4 Oct 2022 at 20:40, David Vernet <void@...ifault.com> wrote:
> >
> > On Tue, Oct 04, 2022 at 12:22:08AM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > Thanks for providing additional context, Kumar. So what do we want to do
> > > > for this patch set? IMO it doesn't seem useful to restrict
> > > > bpf_kfunc_acquire() to only be callable by non-sleepable programs if our
> > > > goal is to avoid crashes for nested task structs. We could easily
> > > > accidentally crash if e.g. those pointers are NULL, or someone is doing
> > > > something weird like stashing some extra flag bits in unused portions of
> > > > the pointer which are masked out when it's actually dereferenced
> > > > regardless of whether we're in RCU.  Trusting ctx loads sounds like the
> > > > right approach, barring some of the challenges you pointed out such as
> > > > dealing with fexit paths after free where the object may not be valid
> > > > anymore.
> > > >
> > > > In general, it seems like we should maybe decide on what our policy
> > > > should be for kfuncs until we can wire up whatever we need to properly
> > > > trust ctx.
> > >
> > > Well, we could add it now and work towards closing the gaps after
> > > this, especially if bpf_task_acquire is really only useful in
> > > sleepable programs where it works on the tracing args. A lot of other
> > > kfuncs need these fixes as well, so it's a general problem and not
> > > specific to this set. I am not very familiar with your exact use case.
> > > Hopefully when it is fixed this particular case won't really break, if
> > > you only use the tracepoint argument.
> >
> > I'm also interested in using this with struct_ops, not just tracing. I
> > think that struct_ops should be totally fine though, and easier to
> > reason about than tracing as we just have to make sure that a few
> > specific callbacks are always passed a valid, referenced task, rather
> > than e.g. worrying about fexit on __put_task_struct().
> >
> > I'm fine with adding this now and working towards closing the gaps
> > later, though I'd like to hear what Martin, Alexei, and the rest of the
> > BPF maintainers think. I think Martin asked if there was any preliminary
> > work you'd already done that we could try to tie into this patch set,
> > and I'm similarly curious.
> >
> 
> It's mostly a few experimental patches in my local tree, so nowhere
> close to completion. Ofcourse doing it properly will be a lot of work,
> but I will be happy to help with reviews if you want to focus on
> pointers loaded from ctx for now and make that part of this set, while
> not permitting any other cases. It should not be very difficult to add
> just that.

Sounds good, I'll include that in the v3 iteration of the patch set. We
can worry about black-listing certain unsafe tracing programs, and doing
whatever other fancier things we can think of as well later.

> So you can set KF_TRUSTED_ARGS for your kfunc, then make it work for
> PTR_TO_BTF_ID where it either has PTR_TRUSTED, ref_obj_id > 0, or
> both. Just that PTR_TRUSTED is lost for the destination reg as soon as
> btf_struct_access is used to walk pointers (unlike PTR_UNTRUSTED which
> is inherited). Note that you don't _set_ PTR_UNTRUSTED here for the
> dst_reg.

Makes sense, I think I know what needs to change and it should only be a
few small places in the verifier (plus the testing we'll add to validate
it).

> This should enable your immediate use case, while also being useful
> for future work that builds on it. It should also preserve backwards
> compatibility with existing stable helpers. You set PTR_TRUSTED when
> you access ctx of tracing or struct_ops etc. All this will be handled
> in is_valid_access callback/btf_ctx_access and setting the flag in
> bpf_insn_access_aux. Unless I missed some detail/corner case it won't
> be a lot of code.
> 
> If that turns out to be too restrictive/coarse for your use case, we
> can just go with this as is for now.

Alright, works for me. Let me mess around with this and see if it's as
easy as we think it is. If so, v3 will include those changes. If not,
it'll just include a couple of small fixes that Martin pointed out on
the v2 revision.

Thanks for reviewing these patches.

- David

Powered by blists - more mailing lists