Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules

From: Jiri Olsa <olsajiri@gmail.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Jiri Olsa <olsajiri@gmail.com>,
	bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yonghong.song@linux.dev, dan.carpenter@linaro.org
Subject: Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules
Date: Thu, 30 Nov 2023 23:34:05 +0100	[thread overview]
Message-ID: <ZWkN3QMnAGwFo7JF@krava> (raw)
In-Reply-To: <20231130185752.2fepd5wlospfzc53@erthalion.local>

On Thu, Nov 30, 2023 at 07:57:52PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 03:30:38PM +0100, Jiri Olsa wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 8e7b6072e3f4..31ffcffb7198 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -20109,6 +20109,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > >  	if (tgt_prog) {
> > >  		struct bpf_prog_aux *aux = tgt_prog->aux;
> > >
> > > +		if (aux->attach_depth >= 32) {
> > > +			bpf_log(log, "Target program attach depth is %d. Too large\n",
> > > +					aux->attach_depth);
> > > +			return -EINVAL;
> > > +		}
> >
> > IIUC the use case you have is to be able to attach fentry program on
> > another fentry program.. do you have use case that needs more than
> > that?
> >
> > could we allow just single nesting? that might perhaps endup in easier
> > code while still allowing your use case?
> 
> Right, there is no use case beyond attaching an fentry to another one,
> so having just a single nesting should be fine.
> 
> > > +			/*
> > > +			 * To avoid potential call chain cycles, prevent attaching programs
> > > +			 * of the same type. The only exception is standalone fentry/fexit
> > > +			 * programs that themselves are not attachment targets.
> > > +			 * That means:
> > > +			 *  - Cannot attach followed fentry/fexit to another
> > > +			 *    fentry/fexit program.
> > > +			 *  - Cannot attach program extension to another extension.
> > >  			 * It's ok to attach fentry/fexit to extension program.
> >
> > next condition below denies extension on fentry/fexit and the reason
> > is the possibility:
> >   "... to create long call chain * fentry->extension->fentry->extension
> >   beyond reasonable stack size ..."
> >
> > that might be problem also here with 32 allowed nesting
> 
> Reducing nesting to only one level probably will lift this question, but
> for posterity, what kind of problem similar to "fentry->extension->fentry->..."
> do you have in mind?

I meant that allowing that amount of nesting will make it easier to get
to a point where we use all the available stack size

jirka