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:   Tue, 4 Oct 2022 10:10:48 -0500
From:   David Vernet <void@manifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc:     Martin KaFai Lau <martin.lau@linux.dev>, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, kernel-team@fb.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org, yhs@fb.com,
        song@kernel.org, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org, tj@kernel.org
Subject: Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs
Message-ID: <YzxM+HSSqIDCPCUf@maniforge.lan>
References: <20221001144716.3403120-1-void@manifault.com>
 <20221001144716.3403120-3-void@manifault.com>
 <CAP01T74TtMARkfYWsYY0+cnsx2w4axB1LtvF-RFMAihW7v=LUw@mail.gmail.com>
 <YzsBSoGnPEIJADSH@maniforge.dhcp.thefacebook.com>
 <CAP01T76OR3J_P8YMq4ZgKHBpuZyA0zgsPy+tq9htbX=j6AVyOg@mail.gmail.com>
 <fb3e81b7-8360-5132-59ac-0e74483eb25f@linux.dev>
 <CAP01T77tCdKTJo=sByg5GsW1OrQmNXV4fmBDKUVtbnwEaJBpVA@mail.gmail.com>
 <YztbOo7TgOoN1bVB@maniforge.dhcp.thefacebook.com>
 <CAP01T76rCLdExKZ0AdP9L6e_g+sj9D7Ec59rr+ddMJ-KU+h8QQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP01T76rCLdExKZ0AdP9L6e_g+sj9D7Ec59rr+ddMJ-KU+h8QQ@mail.gmail.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk

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 is true that waiting for all the fixes will unnecessarily stall
> this, it is not clear how each of the issues will be addressed either.
> 
> Later its use can be made conditional in sleepable programs for
> trusted and rcu tagged pointers under appropriate RCU read lock. I
> will try to prioritize sending it out so that we resolve this soon.

Much appreciated!