[bpf-next,0/3] bpf: Add bpf_perf_event_read_sample() helper (v1)

  Hello,

I'd like to add bpf_perf_event_read_sample() helper to get the sample data
of an perf_event from BPF programs.  This enables more sophistigated filtering
for the perf samples.  Initially I'm thinking of code and data address based
filtering.

The original discussion can be seen here:
  https://lore.kernel.org/r/20220823210354.1407473-1-namhyung@kernel.org

The bpf_perf_event_read_sample() will take a buffer and size to save the data
as well as a flag to specify perf sample type.  The flag argument should have
a single value in the enum perf_event_sample_format like PERF_SAMPLE_IP.  If
the buffer is NULL, it will return the size of data instead.  This is to
support variable length data in the future.

The first patch adds bpf_prepare_sample() to setup necessary perf sample data
before calling the bpf overflow handler for the perf event.  The existing
logic for callchain moved to the function and it sets IP and ADDR data if
they are not set already.

The second patch actually adds the bpf_perf_event_read_sample() helper and
supports IP and ADDR data.  The last patch adds a test code for this.

The code is available at 'bpf/perf-sample-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (3):
  perf/core: Prepare sample data before calling BPF
  bpf: Add bpf_perf_event_read_sample() helper
  bpf: Add perf_event_read_sample test cases

 include/uapi/linux/bpf.h                      |  23 +++
 kernel/events/core.c                          |  40 +++-
 kernel/trace/bpf_trace.c                      |  49 +++++
 tools/include/uapi/linux/bpf.h                |  23 +++
 .../selftests/bpf/prog_tests/perf_sample.c    | 172 ++++++++++++++++++
 .../selftests/bpf/progs/test_perf_sample.c    |  28 +++
 6 files changed, 326 insertions(+), 9 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/perf_sample.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_perf_sample.c


base-commit: e39e739ab57399f46167d453bbdb8ef8d57c6488
  

On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> The bpf_perf_event_read_sample() helper is to get the specified sample
> data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> PERF_SAMPLE_DATA flags are supported only.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
>  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 94659f6b3395..cba501de9373 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5481,6 +5481,28 @@ union bpf_attr {
>   *		0 on success.
>   *
>   *		**-ENOENT** if the bpf_local_storage cannot be found.
> + *
> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> + *	Description
> + *		For an eBPF program attached to a perf event, retrieve the
> + *		sample data associated to *ctx*	and store it in the buffer
> + *		pointed by *buf* up to size *size* bytes.
> + *
> + *		The *sample_flags* should contain a single value in the
> + *		**enum perf_event_sample_format**.
> + *	Return
> + *		On success, number of bytes written to *buf*. On error, a
> + *		negative value.
> + *
> + *		The *buf* can be set to **NULL** to return the number of bytes
> + *		required to store the requested sample data.
> + *
> + *		**-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> + *
> + *		**-ENOENT** if the associated perf event doesn't have the data.
> + *
> + *		**-ENOSYS** if system doesn't support the sample data to be
> + *		retrieved.
>   */
>  #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>  	FN(unspec, 0, ##ctx)				\
> @@ -5695,6 +5717,7 @@ union bpf_attr {
>  	FN(user_ringbuf_drain, 209, ##ctx)		\
>  	FN(cgrp_storage_get, 210, ##ctx)		\
>  	FN(cgrp_storage_delete, 211, ##ctx)		\
> +	FN(perf_event_read_sample, 212, ##ctx)		\
>  	/* */
>  
>  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ce0228c72a93..befd937afa3c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -28,6 +28,7 @@
>  
>  #include <uapi/linux/bpf.h>
>  #include <uapi/linux/btf.h>
> +#include <uapi/linux/perf_event.h>
>  
>  #include <asm/tlb.h>
>  
> @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
>  	.arg4_type      = ARG_ANYTHING,
>  };
>  
> +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> +	   void *, buf, u32, size, u64, flags)
> +{

I wonder we could add perf_btf (like we have tp_btf) program type that
could access ctx->data directly without helpers

> +	struct perf_sample_data *sd = ctx->data;
> +	void *data;
> +	u32 to_copy = sizeof(u64);
> +
> +	/* only allow a single sample flag */
> +	if (!is_power_of_2(flags))
> +		return -EINVAL;
> +
> +	/* support reading only already populated info */
> +	if (flags & ~sd->sample_flags)
> +		return -ENOENT;
> +
> +	switch (flags) {
> +	case PERF_SAMPLE_IP:
> +		data = &sd->ip;
> +		break;
> +	case PERF_SAMPLE_ADDR:
> +		data = &sd->addr;
> +		break;

AFAICS from pe_prog_convert_ctx_access you should be able to read addr
directly from context right? same as sample_period.. so I think if this
will be generic way to read sample data, should we add sample_period
as well?


> +	default:
> +		return -ENOSYS;
> +	}
> +
> +	if (!buf)
> +		return to_copy;
> +
> +	if (size < to_copy)
> +		to_copy = size;

should we fail in here instead? is there any point in returning
not complete data?

jirka


> +
> +	memcpy(buf, data, to_copy);
> +	return to_copy;
> +}
> +
> +static const struct bpf_func_proto bpf_perf_event_read_sample_proto = {
> +	.func           = bpf_perf_event_read_sample,
> +	.gpl_only       = true,
> +	.ret_type       = RET_INTEGER,
> +	.arg1_type      = ARG_PTR_TO_CTX,
> +	.arg2_type      = ARG_PTR_TO_MEM_OR_NULL,
> +	.arg3_type      = ARG_CONST_SIZE_OR_ZERO,
> +	.arg4_type      = ARG_ANYTHING,
> +};
> +
>  static const struct bpf_func_proto *
>  pe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
> @@ -1759,6 +1806,8 @@ pe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_read_branch_records_proto;
>  	case BPF_FUNC_get_attach_cookie:
>  		return &bpf_get_attach_cookie_proto_pe;
> +	case BPF_FUNC_perf_event_read_sample:
> +		return &bpf_perf_event_read_sample_proto;
>  	default:
>  		return bpf_tracing_func_proto(func_id, prog);
>  	}
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 94659f6b3395..cba501de9373 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5481,6 +5481,28 @@ union bpf_attr {
>   *		0 on success.
>   *
>   *		**-ENOENT** if the bpf_local_storage cannot be found.
> + *
> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> + *	Description
> + *		For an eBPF program attached to a perf event, retrieve the
> + *		sample data associated to *ctx*	and store it in the buffer
> + *		pointed by *buf* up to size *size* bytes.
> + *
> + *		The *sample_flags* should contain a single value in the
> + *		**enum perf_event_sample_format**.
> + *	Return
> + *		On success, number of bytes written to *buf*. On error, a
> + *		negative value.
> + *
> + *		The *buf* can be set to **NULL** to return the number of bytes
> + *		required to store the requested sample data.
> + *
> + *		**-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> + *
> + *		**-ENOENT** if the associated perf event doesn't have the data.
> + *
> + *		**-ENOSYS** if system doesn't support the sample data to be
> + *		retrieved.
>   */
>  #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>  	FN(unspec, 0, ##ctx)				\
> @@ -5695,6 +5717,7 @@ union bpf_attr {
>  	FN(user_ringbuf_drain, 209, ##ctx)		\
>  	FN(cgrp_storage_get, 210, ##ctx)		\
>  	FN(cgrp_storage_delete, 211, ##ctx)		\
> +	FN(perf_event_read_sample, 212, ##ctx)		\
>  	/* */
>  
>  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> -- 
> 2.38.1.273.g43a17bfeac-goog
>
  

On Tue, Nov 1, 2022 at 3:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> > The bpf_perf_event_read_sample() helper is to get the specified sample
> > data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> > decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> > PERF_SAMPLE_DATA flags are supported only.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> >  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> >  3 files changed, 95 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 94659f6b3395..cba501de9373 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5481,6 +5481,28 @@ union bpf_attr {
> >   *           0 on success.
> >   *
> >   *           **-ENOENT** if the bpf_local_storage cannot be found.
> > + *
> > + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> > + *   Description
> > + *           For an eBPF program attached to a perf event, retrieve the
> > + *           sample data associated to *ctx* and store it in the buffer
> > + *           pointed by *buf* up to size *size* bytes.
> > + *
> > + *           The *sample_flags* should contain a single value in the
> > + *           **enum perf_event_sample_format**.
> > + *   Return
> > + *           On success, number of bytes written to *buf*. On error, a
> > + *           negative value.
> > + *
> > + *           The *buf* can be set to **NULL** to return the number of bytes
> > + *           required to store the requested sample data.
> > + *
> > + *           **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> > + *
> > + *           **-ENOENT** if the associated perf event doesn't have the data.
> > + *
> > + *           **-ENOSYS** if system doesn't support the sample data to be
> > + *           retrieved.
> >   */
> >  #define ___BPF_FUNC_MAPPER(FN, ctx...)                       \
> >       FN(unspec, 0, ##ctx)                            \
> > @@ -5695,6 +5717,7 @@ union bpf_attr {
> >       FN(user_ringbuf_drain, 209, ##ctx)              \
> >       FN(cgrp_storage_get, 210, ##ctx)                \
> >       FN(cgrp_storage_delete, 211, ##ctx)             \
> > +     FN(perf_event_read_sample, 212, ##ctx)          \
> >       /* */
> >
> >  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index ce0228c72a93..befd937afa3c 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -28,6 +28,7 @@
> >
> >  #include <uapi/linux/bpf.h>
> >  #include <uapi/linux/btf.h>
> > +#include <uapi/linux/perf_event.h>
> >
> >  #include <asm/tlb.h>
> >
> > @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> >       .arg4_type      = ARG_ANYTHING,
> >  };
> >
> > +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> > +        void *, buf, u32, size, u64, flags)
> > +{
>
> I wonder we could add perf_btf (like we have tp_btf) program type that
> could access ctx->data directly without helpers
>
> > +     struct perf_sample_data *sd = ctx->data;
> > +     void *data;
> > +     u32 to_copy = sizeof(u64);
> > +
> > +     /* only allow a single sample flag */
> > +     if (!is_power_of_2(flags))
> > +             return -EINVAL;
> > +
> > +     /* support reading only already populated info */
> > +     if (flags & ~sd->sample_flags)
> > +             return -ENOENT;
> > +
> > +     switch (flags) {
> > +     case PERF_SAMPLE_IP:
> > +             data = &sd->ip;
> > +             break;
> > +     case PERF_SAMPLE_ADDR:
> > +             data = &sd->addr;
> > +             break;
>
> AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> directly from context right? same as sample_period.. so I think if this
> will be generic way to read sample data, should we add sample_period
> as well?

+1
Let's avoid new stable helpers for this.
Pls use CORE and read perf_sample_data directly.
  

On Tue, Nov 1, 2022 at 11:26 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 1, 2022 at 3:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> > > The bpf_perf_event_read_sample() helper is to get the specified sample
> > > data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> > > decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> > > PERF_SAMPLE_DATA flags are supported only.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> > >  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> > >  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 94659f6b3395..cba501de9373 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -5481,6 +5481,28 @@ union bpf_attr {
> > >   *           0 on success.
> > >   *
> > >   *           **-ENOENT** if the bpf_local_storage cannot be found.
> > > + *
> > > + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> > > + *   Description
> > > + *           For an eBPF program attached to a perf event, retrieve the
> > > + *           sample data associated to *ctx* and store it in the buffer
> > > + *           pointed by *buf* up to size *size* bytes.
> > > + *
> > > + *           The *sample_flags* should contain a single value in the
> > > + *           **enum perf_event_sample_format**.
> > > + *   Return
> > > + *           On success, number of bytes written to *buf*. On error, a
> > > + *           negative value.
> > > + *
> > > + *           The *buf* can be set to **NULL** to return the number of bytes
> > > + *           required to store the requested sample data.
> > > + *
> > > + *           **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> > > + *
> > > + *           **-ENOENT** if the associated perf event doesn't have the data.
> > > + *
> > > + *           **-ENOSYS** if system doesn't support the sample data to be
> > > + *           retrieved.
> > >   */
> > >  #define ___BPF_FUNC_MAPPER(FN, ctx...)                       \
> > >       FN(unspec, 0, ##ctx)                            \
> > > @@ -5695,6 +5717,7 @@ union bpf_attr {
> > >       FN(user_ringbuf_drain, 209, ##ctx)              \
> > >       FN(cgrp_storage_get, 210, ##ctx)                \
> > >       FN(cgrp_storage_delete, 211, ##ctx)             \
> > > +     FN(perf_event_read_sample, 212, ##ctx)          \
> > >       /* */
> > >
> > >  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index ce0228c72a93..befd937afa3c 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -28,6 +28,7 @@
> > >
> > >  #include <uapi/linux/bpf.h>
> > >  #include <uapi/linux/btf.h>
> > > +#include <uapi/linux/perf_event.h>
> > >
> > >  #include <asm/tlb.h>
> > >
> > > @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> > >       .arg4_type      = ARG_ANYTHING,
> > >  };
> > >
> > > +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> > > +        void *, buf, u32, size, u64, flags)
> > > +{
> >
> > I wonder we could add perf_btf (like we have tp_btf) program type that
> > could access ctx->data directly without helpers
> >
> > > +     struct perf_sample_data *sd = ctx->data;
> > > +     void *data;
> > > +     u32 to_copy = sizeof(u64);
> > > +
> > > +     /* only allow a single sample flag */
> > > +     if (!is_power_of_2(flags))
> > > +             return -EINVAL;
> > > +
> > > +     /* support reading only already populated info */
> > > +     if (flags & ~sd->sample_flags)
> > > +             return -ENOENT;
> > > +
> > > +     switch (flags) {
> > > +     case PERF_SAMPLE_IP:
> > > +             data = &sd->ip;
> > > +             break;
> > > +     case PERF_SAMPLE_ADDR:
> > > +             data = &sd->addr;
> > > +             break;
> >
> > AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> > directly from context right? same as sample_period.. so I think if this
> > will be generic way to read sample data, should we add sample_period
> > as well?
>
> +1
> Let's avoid new stable helpers for this.
> Pls use CORE and read perf_sample_data directly.

We have legacy ways to access sample_period and addr with
struct bpf_perf_event_data and struct bpf_perf_event_data_kern. I
think mixing that
with CORE makes it confusing for the user. And a helper or a kfunc would make it
easier to follow. perf_btf might also be a good approach for this.

Thanks,
Song
  

On Tue, Nov 1, 2022 at 11:47 AM Song Liu <song@kernel.org> wrote:
>
> On Tue, Nov 1, 2022 at 11:26 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Nov 1, 2022 at 3:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> > > > The bpf_perf_event_read_sample() helper is to get the specified sample
> > > > data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> > > > decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> > > > PERF_SAMPLE_DATA flags are supported only.
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> > > >  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> > > >  3 files changed, 95 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index 94659f6b3395..cba501de9373 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -5481,6 +5481,28 @@ union bpf_attr {
> > > >   *           0 on success.
> > > >   *
> > > >   *           **-ENOENT** if the bpf_local_storage cannot be found.
> > > > + *
> > > > + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> > > > + *   Description
> > > > + *           For an eBPF program attached to a perf event, retrieve the
> > > > + *           sample data associated to *ctx* and store it in the buffer
> > > > + *           pointed by *buf* up to size *size* bytes.
> > > > + *
> > > > + *           The *sample_flags* should contain a single value in the
> > > > + *           **enum perf_event_sample_format**.
> > > > + *   Return
> > > > + *           On success, number of bytes written to *buf*. On error, a
> > > > + *           negative value.
> > > > + *
> > > > + *           The *buf* can be set to **NULL** to return the number of bytes
> > > > + *           required to store the requested sample data.
> > > > + *
> > > > + *           **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> > > > + *
> > > > + *           **-ENOENT** if the associated perf event doesn't have the data.
> > > > + *
> > > > + *           **-ENOSYS** if system doesn't support the sample data to be
> > > > + *           retrieved.
> > > >   */
> > > >  #define ___BPF_FUNC_MAPPER(FN, ctx...)                       \
> > > >       FN(unspec, 0, ##ctx)                            \
> > > > @@ -5695,6 +5717,7 @@ union bpf_attr {
> > > >       FN(user_ringbuf_drain, 209, ##ctx)              \
> > > >       FN(cgrp_storage_get, 210, ##ctx)                \
> > > >       FN(cgrp_storage_delete, 211, ##ctx)             \
> > > > +     FN(perf_event_read_sample, 212, ##ctx)          \
> > > >       /* */
> > > >
> > > >  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index ce0228c72a93..befd937afa3c 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -28,6 +28,7 @@
> > > >
> > > >  #include <uapi/linux/bpf.h>
> > > >  #include <uapi/linux/btf.h>
> > > > +#include <uapi/linux/perf_event.h>
> > > >
> > > >  #include <asm/tlb.h>
> > > >
> > > > @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> > > >       .arg4_type      = ARG_ANYTHING,
> > > >  };
> > > >
> > > > +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> > > > +        void *, buf, u32, size, u64, flags)
> > > > +{
> > >
> > > I wonder we could add perf_btf (like we have tp_btf) program type that
> > > could access ctx->data directly without helpers
> > >
> > > > +     struct perf_sample_data *sd = ctx->data;
> > > > +     void *data;
> > > > +     u32 to_copy = sizeof(u64);
> > > > +
> > > > +     /* only allow a single sample flag */
> > > > +     if (!is_power_of_2(flags))
> > > > +             return -EINVAL;
> > > > +
> > > > +     /* support reading only already populated info */
> > > > +     if (flags & ~sd->sample_flags)
> > > > +             return -ENOENT;
> > > > +
> > > > +     switch (flags) {
> > > > +     case PERF_SAMPLE_IP:
> > > > +             data = &sd->ip;
> > > > +             break;
> > > > +     case PERF_SAMPLE_ADDR:
> > > > +             data = &sd->addr;
> > > > +             break;
> > >
> > > AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> > > directly from context right? same as sample_period.. so I think if this
> > > will be generic way to read sample data, should we add sample_period
> > > as well?
> >
> > +1
> > Let's avoid new stable helpers for this.
> > Pls use CORE and read perf_sample_data directly.
>
> We have legacy ways to access sample_period and addr with
> struct bpf_perf_event_data and struct bpf_perf_event_data_kern. I
> think mixing that
> with CORE makes it confusing for the user. And a helper or a kfunc would make it
> easier to follow. perf_btf might also be a good approach for this.

imo that's a counter argument to non-CORE style.
struct bpf_perf_event_data has sample_period and addr,
and as soon as we pushed the boundaries it turned out it's not enough.
Now we're proposing to extend uapi a bit with sample_ip.
That will repeat the same mistake.
Just use CORE and read everything that is there today
and will be there in the future.
  

On Tue, Nov 1, 2022 at 11:53 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 1, 2022 at 11:47 AM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, Nov 1, 2022 at 11:26 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Nov 1, 2022 at 3:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> > > > > The bpf_perf_event_read_sample() helper is to get the specified sample
> > > > > data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> > > > > decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> > > > > PERF_SAMPLE_DATA flags are supported only.
> > > > >
> > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > ---
> > > > >  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> > > > >  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> > > > >  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> > > > >  3 files changed, 95 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > > index 94659f6b3395..cba501de9373 100644
> > > > > --- a/include/uapi/linux/bpf.h
> > > > > +++ b/include/uapi/linux/bpf.h
> > > > > @@ -5481,6 +5481,28 @@ union bpf_attr {
> > > > >   *           0 on success.
> > > > >   *
> > > > >   *           **-ENOENT** if the bpf_local_storage cannot be found.
> > > > > + *
> > > > > + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> > > > > + *   Description
> > > > > + *           For an eBPF program attached to a perf event, retrieve the
> > > > > + *           sample data associated to *ctx* and store it in the buffer
> > > > > + *           pointed by *buf* up to size *size* bytes.
> > > > > + *
> > > > > + *           The *sample_flags* should contain a single value in the
> > > > > + *           **enum perf_event_sample_format**.
> > > > > + *   Return
> > > > > + *           On success, number of bytes written to *buf*. On error, a
> > > > > + *           negative value.
> > > > > + *
> > > > > + *           The *buf* can be set to **NULL** to return the number of bytes
> > > > > + *           required to store the requested sample data.
> > > > > + *
> > > > > + *           **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> > > > > + *
> > > > > + *           **-ENOENT** if the associated perf event doesn't have the data.
> > > > > + *
> > > > > + *           **-ENOSYS** if system doesn't support the sample data to be
> > > > > + *           retrieved.
> > > > >   */
> > > > >  #define ___BPF_FUNC_MAPPER(FN, ctx...)                       \
> > > > >       FN(unspec, 0, ##ctx)                            \
> > > > > @@ -5695,6 +5717,7 @@ union bpf_attr {
> > > > >       FN(user_ringbuf_drain, 209, ##ctx)              \
> > > > >       FN(cgrp_storage_get, 210, ##ctx)                \
> > > > >       FN(cgrp_storage_delete, 211, ##ctx)             \
> > > > > +     FN(perf_event_read_sample, 212, ##ctx)          \
> > > > >       /* */
> > > > >
> > > > >  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > > index ce0228c72a93..befd937afa3c 100644
> > > > > --- a/kernel/trace/bpf_trace.c
> > > > > +++ b/kernel/trace/bpf_trace.c
> > > > > @@ -28,6 +28,7 @@
> > > > >
> > > > >  #include <uapi/linux/bpf.h>
> > > > >  #include <uapi/linux/btf.h>
> > > > > +#include <uapi/linux/perf_event.h>
> > > > >
> > > > >  #include <asm/tlb.h>
> > > > >
> > > > > @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> > > > >       .arg4_type      = ARG_ANYTHING,
> > > > >  };
> > > > >
> > > > > +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> > > > > +        void *, buf, u32, size, u64, flags)
> > > > > +{
> > > >
> > > > I wonder we could add perf_btf (like we have tp_btf) program type that
> > > > could access ctx->data directly without helpers
> > > >
> > > > > +     struct perf_sample_data *sd = ctx->data;
> > > > > +     void *data;
> > > > > +     u32 to_copy = sizeof(u64);
> > > > > +
> > > > > +     /* only allow a single sample flag */
> > > > > +     if (!is_power_of_2(flags))
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     /* support reading only already populated info */
> > > > > +     if (flags & ~sd->sample_flags)
> > > > > +             return -ENOENT;
> > > > > +
> > > > > +     switch (flags) {
> > > > > +     case PERF_SAMPLE_IP:
> > > > > +             data = &sd->ip;
> > > > > +             break;
> > > > > +     case PERF_SAMPLE_ADDR:
> > > > > +             data = &sd->addr;
> > > > > +             break;
> > > >
> > > > AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> > > > directly from context right? same as sample_period.. so I think if this
> > > > will be generic way to read sample data, should we add sample_period
> > > > as well?
> > >
> > > +1
> > > Let's avoid new stable helpers for this.
> > > Pls use CORE and read perf_sample_data directly.
> >
> > We have legacy ways to access sample_period and addr with
> > struct bpf_perf_event_data and struct bpf_perf_event_data_kern. I
> > think mixing that
> > with CORE makes it confusing for the user. And a helper or a kfunc would make it
> > easier to follow. perf_btf might also be a good approach for this.
>
> imo that's a counter argument to non-CORE style.
> struct bpf_perf_event_data has sample_period and addr,
> and as soon as we pushed the boundaries it turned out it's not enough.
> Now we're proposing to extend uapi a bit with sample_ip.
> That will repeat the same mistake.
> Just use CORE and read everything that is there today
> and will be there in the future.

Another work of this effort is that we need the perf_event to prepare
required fields before calling the BPF program. I think we will need
some logic in addition to CORE to get that right. How about we add
perf_btf where the perf_event prepare all fields before calling the
BPF program? perf_btf + CORE will be able to read all fields in the
sample.

Thanks,
Song
  

Hi,

On Tue, Nov 1, 2022 at 1:04 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Nov 1, 2022 at 11:53 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Nov 1, 2022 at 11:47 AM Song Liu <song@kernel.org> wrote:
> > >
> > > On Tue, Nov 1, 2022 at 11:26 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Tue, Nov 1, 2022 at 3:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > > >
> > > > > On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> > > > > > The bpf_perf_event_read_sample() helper is to get the specified sample
> > > > > > data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> > > > > > decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> > > > > > PERF_SAMPLE_DATA flags are supported only.
> > > > > >
> > > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > > ---
> > > > > >  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> > > > > >  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> > > > > >  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> > > > > >  3 files changed, 95 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > > > index 94659f6b3395..cba501de9373 100644
> > > > > > --- a/include/uapi/linux/bpf.h
> > > > > > +++ b/include/uapi/linux/bpf.h
> > > > > > @@ -5481,6 +5481,28 @@ union bpf_attr {
> > > > > >   *           0 on success.
> > > > > >   *
> > > > > >   *           **-ENOENT** if the bpf_local_storage cannot be found.
> > > > > > + *
> > > > > > + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> > > > > > + *   Description
> > > > > > + *           For an eBPF program attached to a perf event, retrieve the
> > > > > > + *           sample data associated to *ctx* and store it in the buffer
> > > > > > + *           pointed by *buf* up to size *size* bytes.
> > > > > > + *
> > > > > > + *           The *sample_flags* should contain a single value in the
> > > > > > + *           **enum perf_event_sample_format**.
> > > > > > + *   Return
> > > > > > + *           On success, number of bytes written to *buf*. On error, a
> > > > > > + *           negative value.
> > > > > > + *
> > > > > > + *           The *buf* can be set to **NULL** to return the number of bytes
> > > > > > + *           required to store the requested sample data.
> > > > > > + *
> > > > > > + *           **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> > > > > > + *
> > > > > > + *           **-ENOENT** if the associated perf event doesn't have the data.
> > > > > > + *
> > > > > > + *           **-ENOSYS** if system doesn't support the sample data to be
> > > > > > + *           retrieved.
> > > > > >   */
> > > > > >  #define ___BPF_FUNC_MAPPER(FN, ctx...)                       \
> > > > > >       FN(unspec, 0, ##ctx)                            \
> > > > > > @@ -5695,6 +5717,7 @@ union bpf_attr {
> > > > > >       FN(user_ringbuf_drain, 209, ##ctx)              \
> > > > > >       FN(cgrp_storage_get, 210, ##ctx)                \
> > > > > >       FN(cgrp_storage_delete, 211, ##ctx)             \
> > > > > > +     FN(perf_event_read_sample, 212, ##ctx)          \
> > > > > >       /* */
> > > > > >
> > > > > >  /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> > > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > > > index ce0228c72a93..befd937afa3c 100644
> > > > > > --- a/kernel/trace/bpf_trace.c
> > > > > > +++ b/kernel/trace/bpf_trace.c
> > > > > > @@ -28,6 +28,7 @@
> > > > > >
> > > > > >  #include <uapi/linux/bpf.h>
> > > > > >  #include <uapi/linux/btf.h>
> > > > > > +#include <uapi/linux/perf_event.h>
> > > > > >
> > > > > >  #include <asm/tlb.h>
> > > > > >
> > > > > > @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> > > > > >       .arg4_type      = ARG_ANYTHING,
> > > > > >  };
> > > > > >
> > > > > > +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> > > > > > +        void *, buf, u32, size, u64, flags)
> > > > > > +{
> > > > >
> > > > > I wonder we could add perf_btf (like we have tp_btf) program type that
> > > > > could access ctx->data directly without helpers
> > > > >
> > > > > > +     struct perf_sample_data *sd = ctx->data;
> > > > > > +     void *data;
> > > > > > +     u32 to_copy = sizeof(u64);
> > > > > > +
> > > > > > +     /* only allow a single sample flag */
> > > > > > +     if (!is_power_of_2(flags))
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     /* support reading only already populated info */
> > > > > > +     if (flags & ~sd->sample_flags)
> > > > > > +             return -ENOENT;
> > > > > > +
> > > > > > +     switch (flags) {
> > > > > > +     case PERF_SAMPLE_IP:
> > > > > > +             data = &sd->ip;
> > > > > > +             break;
> > > > > > +     case PERF_SAMPLE_ADDR:
> > > > > > +             data = &sd->addr;
> > > > > > +             break;
> > > > >
> > > > > AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> > > > > directly from context right? same as sample_period.. so I think if this
> > > > > will be generic way to read sample data, should we add sample_period
> > > > > as well?
> > > >
> > > > +1
> > > > Let's avoid new stable helpers for this.
> > > > Pls use CORE and read perf_sample_data directly.
> > >
> > > We have legacy ways to access sample_period and addr with
> > > struct bpf_perf_event_data and struct bpf_perf_event_data_kern. I
> > > think mixing that
> > > with CORE makes it confusing for the user. And a helper or a kfunc would make it
> > > easier to follow. perf_btf might also be a good approach for this.
> >
> > imo that's a counter argument to non-CORE style.
> > struct bpf_perf_event_data has sample_period and addr,
> > and as soon as we pushed the boundaries it turned out it's not enough.
> > Now we're proposing to extend uapi a bit with sample_ip.
> > That will repeat the same mistake.
> > Just use CORE and read everything that is there today
> > and will be there in the future.
>
> Another work of this effort is that we need the perf_event to prepare
> required fields before calling the BPF program. I think we will need
> some logic in addition to CORE to get that right. How about we add
> perf_btf where the perf_event prepare all fields before calling the
> BPF program? perf_btf + CORE will be able to read all fields in the
> sample.

IIUC we want something like below to access sample data directly,
right?

  BPF_CORE_READ(ctx, data, ip);

Some fields like raw and callchains will have variable length data
so it'd be hard to check the boundary at load time.  Also it's possible
that some fields are not set (according to sample type), and it'd be
the user's (or programmer's) responsibility to check if the data is
valid.  If these are not the concerns, I think I'm good.

Thanks,
Namhyung
  

On Tue, Nov 1, 2022 at 3:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > +1
> > > > > Let's avoid new stable helpers for this.
> > > > > Pls use CORE and read perf_sample_data directly.
> > > >
> > > > We have legacy ways to access sample_period and addr with
> > > > struct bpf_perf_event_data and struct bpf_perf_event_data_kern. I
> > > > think mixing that
> > > > with CORE makes it confusing for the user. And a helper or a kfunc would make it
> > > > easier to follow. perf_btf might also be a good approach for this.
> > >
> > > imo that's a counter argument to non-CORE style.
> > > struct bpf_perf_event_data has sample_period and addr,
> > > and as soon as we pushed the boundaries it turned out it's not enough.
> > > Now we're proposing to extend uapi a bit with sample_ip.
> > > That will repeat the same mistake.
> > > Just use CORE and read everything that is there today
> > > and will be there in the future.
> >
> > Another work of this effort is that we need the perf_event to prepare
> > required fields before calling the BPF program. I think we will need
> > some logic in addition to CORE to get that right. How about we add
> > perf_btf where the perf_event prepare all fields before calling the
> > BPF program? perf_btf + CORE will be able to read all fields in the
> > sample.
>
> IIUC we want something like below to access sample data directly,
> right?
>
>   BPF_CORE_READ(ctx, data, ip);
>

I haven't tried this, but I guess we may need something like

data = ctx->data;
BPF_CORE_READ(data, ip);

> Some fields like raw and callchains will have variable length data
> so it'd be hard to check the boundary at load time.

I think we are fine as long as we can check boundaries at run time.

> Also it's possible
> that some fields are not set (according to sample type), and it'd be
> the user's (or programmer's) responsibility to check if the data is
> valid.  If these are not the concerns, I think I'm good.

So we still need 1/3 of the set to make sure the data is valid?

Thanks,
Song
  

On Tue, Nov 1, 2022 at 5:13 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Nov 1, 2022 at 3:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > IIUC we want something like below to access sample data directly,
> > right?
> >
> >   BPF_CORE_READ(ctx, data, ip);
> >
>
> I haven't tried this, but I guess we may need something like
>
> data = ctx->data;
> BPF_CORE_READ(data, ip);

Ok, will try.

>
> > Some fields like raw and callchains will have variable length data
> > so it'd be hard to check the boundary at load time.
>
> I think we are fine as long as we can check boundaries at run time.

Sure, that means it's the responsibility of BPF writers, right?

>
> > Also it's possible
> > that some fields are not set (according to sample type), and it'd be
> > the user's (or programmer's) responsibility to check if the data is
> > valid.  If these are not the concerns, I think I'm good.
>
> So we still need 1/3 of the set to make sure the data is valid?

Of course, I'll keep it in the v2.

Thanks,
Namhyung
  

On Wed, Nov 2, 2022 at 3:18 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Nov 1, 2022 at 5:13 PM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, Nov 1, 2022 at 3:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > IIUC we want something like below to access sample data directly,
> > > right?
> > >
> > >   BPF_CORE_READ(ctx, data, ip);
> > >
> >
> > I haven't tried this, but I guess we may need something like
> >
> > data = ctx->data;
> > BPF_CORE_READ(data, ip);
>
> Ok, will try.
>
> >
> > > Some fields like raw and callchains will have variable length data
> > > so it'd be hard to check the boundary at load time.
> >
> > I think we are fine as long as we can check boundaries at run time.
>
> Sure, that means it's the responsibility of BPF writers, right?

Right, the author of the BPF program could check whether the data
is valid.

Song

>
> >
> > > Also it's possible
> > > that some fields are not set (according to sample type), and it'd be
> > > the user's (or programmer's) responsibility to check if the data is
> > > valid.  If these are not the concerns, I think I'm good.
> >
> > So we still need 1/3 of the set to make sure the data is valid?
>
> Of course, I'll keep it in the v2.
>
> Thanks,
> Namhyung
  

On 11/1/22 3:02 AM, Jiri Olsa wrote:
> On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
>> The bpf_perf_event_read_sample() helper is to get the specified sample
>> data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
>> decision for filtering on samples.  Currently PERF_SAMPLE_IP and
>> PERF_SAMPLE_DATA flags are supported only.
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>   include/uapi/linux/bpf.h       | 23 ++++++++++++++++
>>   kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
>>   tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
>>   3 files changed, 95 insertions(+)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 94659f6b3395..cba501de9373 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -5481,6 +5481,28 @@ union bpf_attr {
>>    *		0 on success.
>>    *
>>    *		**-ENOENT** if the bpf_local_storage cannot be found.
>> + *
>> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
>> + *	Description
>> + *		For an eBPF program attached to a perf event, retrieve the
>> + *		sample data associated to *ctx*	and store it in the buffer
>> + *		pointed by *buf* up to size *size* bytes.
>> + *
>> + *		The *sample_flags* should contain a single value in the
>> + *		**enum perf_event_sample_format**.
>> + *	Return
>> + *		On success, number of bytes written to *buf*. On error, a
>> + *		negative value.
>> + *
>> + *		The *buf* can be set to **NULL** to return the number of bytes
>> + *		required to store the requested sample data.
>> + *
>> + *		**-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
>> + *
>> + *		**-ENOENT** if the associated perf event doesn't have the data.
>> + *
>> + *		**-ENOSYS** if system doesn't support the sample data to be
>> + *		retrieved.
>>    */
>>   #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>>   	FN(unspec, 0, ##ctx)				\
>> @@ -5695,6 +5717,7 @@ union bpf_attr {
>>   	FN(user_ringbuf_drain, 209, ##ctx)		\
>>   	FN(cgrp_storage_get, 210, ##ctx)		\
>>   	FN(cgrp_storage_delete, 211, ##ctx)		\
>> +	FN(perf_event_read_sample, 212, ##ctx)		\
>>   	/* */
>>   
>>   /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index ce0228c72a93..befd937afa3c 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -28,6 +28,7 @@
>>   
>>   #include <uapi/linux/bpf.h>
>>   #include <uapi/linux/btf.h>
>> +#include <uapi/linux/perf_event.h>
>>   
>>   #include <asm/tlb.h>
>>   
>> @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
>>   	.arg4_type      = ARG_ANYTHING,
>>   };
>>   
>> +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
>> +	   void *, buf, u32, size, u64, flags)
>> +{
> 
> I wonder we could add perf_btf (like we have tp_btf) program type that
> could access ctx->data directly without helpers

Martin and I have discussed an idea to introduce a generic helper like
     bpf_get_kern_ctx(void *ctx)
Given a context, the helper will return a PTR_TO_BTF_ID representing the
corresponding kernel ctx. So in the above example, user could call

     struct bpf_perf_event_data_kern *kctx = bpf_get_kern_ctx(ctx);
     ...

To implement bpf_get_kern_ctx helper, the verifier can find the type
of the context and provide a hidden btf_id as the second parameter of
the actual kernel helper function like
     bpf_get_kern_ctx(ctx) {
        return ctx;
     }
     /* based on ctx_btf_id, find kctx_btf_id and return it to verifier */

     The bpf_get_kern_ctx helper can be inlined as well.

> 
>> +	struct perf_sample_data *sd = ctx->data;
>> +	void *data;
>> +	u32 to_copy = sizeof(u64);
>> +
>> +	/* only allow a single sample flag */
>> +	if (!is_power_of_2(flags))
>> +		return -EINVAL;
>> +
>> +	/* support reading only already populated info */
>> +	if (flags & ~sd->sample_flags)
>> +		return -ENOENT;
>> +
>> +	switch (flags) {
>> +	case PERF_SAMPLE_IP:
>> +		data = &sd->ip;
>> +		break;
>> +	case PERF_SAMPLE_ADDR:
>> +		data = &sd->addr;
>> +		break;
> 
> AFAICS from pe_prog_convert_ctx_access you should be able to read addr
> directly from context right? same as sample_period.. so I think if this
> will be generic way to read sample data, should we add sample_period
> as well?
> 
> 
>> +	default:
>> +		return -ENOSYS;
>> +	}
>> +
>> +	if (!buf)
>> +		return to_copy;
>> +
>> +	if (size < to_copy)
>> +		to_copy = size;
> 
> should we fail in here instead? is there any point in returning
> not complete data?
> 
> jirka
> 
> 
>> +
>> +	memcpy(buf, data, to_copy);
>> +	return to_copy;
>> +}
>> +
>> +static const struct bpf_func_proto bpf_perf_event_read_sample_proto = {
>> +	.func           = bpf_perf_event_read_sample,
>> +	.gpl_only       = true,
>> +	.ret_type       = RET_INTEGER,
>> +	.arg1_type      = ARG_PTR_TO_CTX,
>> +	.arg2_type      = ARG_PTR_TO_MEM_OR_NULL,
>> +	.arg3_type      = ARG_CONST_SIZE_OR_ZERO,
>> +	.arg4_type      = ARG_ANYTHING,
>> +};
>> +
>[...]
  

> On Nov 3, 2022, at 12:45 PM, Yonghong Song <yhs@meta.com> wrote:
> 
> 
> 
> On 11/1/22 3:02 AM, Jiri Olsa wrote:
>> On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
>>> The bpf_perf_event_read_sample() helper is to get the specified sample
>>> data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
>>> decision for filtering on samples.  Currently PERF_SAMPLE_IP and
>>> PERF_SAMPLE_DATA flags are supported only.
>>> 
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  include/uapi/linux/bpf.h       | 23 ++++++++++++++++
>>>  kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
>>>  tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
>>>  3 files changed, 95 insertions(+)
>>> 
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 94659f6b3395..cba501de9373 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -5481,6 +5481,28 @@ union bpf_attr {
>>>   *		0 on success.
>>>   *
>>>   *		**-ENOENT** if the bpf_local_storage cannot be found.
>>> + *
>>> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
>>> + *	Description
>>> + *		For an eBPF program attached to a perf event, retrieve the
>>> + *		sample data associated to *ctx*	and store it in the buffer
>>> + *		pointed by *buf* up to size *size* bytes.
>>> + *
>>> + *		The *sample_flags* should contain a single value in the
>>> + *		**enum perf_event_sample_format**.
>>> + *	Return
>>> + *		On success, number of bytes written to *buf*. On error, a
>>> + *		negative value.
>>> + *
>>> + *		The *buf* can be set to **NULL** to return the number of bytes
>>> + *		required to store the requested sample data.
>>> + *
>>> + *		**-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
>>> + *
>>> + *		**-ENOENT** if the associated perf event doesn't have the data.
>>> + *
>>> + *		**-ENOSYS** if system doesn't support the sample data to be
>>> + *		retrieved.
>>>   */
>>>  #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>>>  	FN(unspec, 0, ##ctx)				\
>>> @@ -5695,6 +5717,7 @@ union bpf_attr {
>>>  	FN(user_ringbuf_drain, 209, ##ctx)		\
>>>  	FN(cgrp_storage_get, 210, ##ctx)		\
>>>  	FN(cgrp_storage_delete, 211, ##ctx)		\
>>> +	FN(perf_event_read_sample, 212, ##ctx)		\
>>>  	/* */
>>>    /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index ce0228c72a93..befd937afa3c 100644
>>> --- a/kernel/trace/bpf_trace.c
>>> +++ b/kernel/trace/bpf_trace.c
>>> @@ -28,6 +28,7 @@
>>>    #include <uapi/linux/bpf.h>
>>>  #include <uapi/linux/btf.h>
>>> +#include <uapi/linux/perf_event.h>
>>>    #include <asm/tlb.h>
>>>  @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
>>>  	.arg4_type      = ARG_ANYTHING,
>>>  };
>>>  +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
>>> +	   void *, buf, u32, size, u64, flags)
>>> +{
>> I wonder we could add perf_btf (like we have tp_btf) program type that
>> could access ctx->data directly without helpers
> 
> Martin and I have discussed an idea to introduce a generic helper like
>    bpf_get_kern_ctx(void *ctx)
> Given a context, the helper will return a PTR_TO_BTF_ID representing the
> corresponding kernel ctx. So in the above example, user could call
> 
>    struct bpf_perf_event_data_kern *kctx = bpf_get_kern_ctx(ctx);
>    ...

This is an interesting idea! 

> To implement bpf_get_kern_ctx helper, the verifier can find the type
> of the context and provide a hidden btf_id as the second parameter of
> the actual kernel helper function like
>    bpf_get_kern_ctx(ctx) {
>       return ctx;
>    }
>    /* based on ctx_btf_id, find kctx_btf_id and return it to verifier */

I think we will need a map of ctx_btf_id => kctx_btf_id. Shall we somehow
expose this to the user? 

Thanks,
Song


>    The bpf_get_kern_ctx helper can be inlined as well.
  

On 11/3/22 1:55 PM, Song Liu wrote:
> 
> 
>> On Nov 3, 2022, at 12:45 PM, Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 11/1/22 3:02 AM, Jiri Olsa wrote:
>>> On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
>>>> The bpf_perf_event_read_sample() helper is to get the specified sample
>>>> data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
>>>> decision for filtering on samples.  Currently PERF_SAMPLE_IP and
>>>> PERF_SAMPLE_DATA flags are supported only.
>>>>
>>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>>> ---
>>>>   include/uapi/linux/bpf.h       | 23 ++++++++++++++++
>>>>   kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
>>>>   tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
>>>>   3 files changed, 95 insertions(+)
>>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index 94659f6b3395..cba501de9373 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -5481,6 +5481,28 @@ union bpf_attr {
>>>>    *		0 on success.
>>>>    *
>>>>    *		**-ENOENT** if the bpf_local_storage cannot be found.
>>>> + *
>>>> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
>>>> + *	Description
>>>> + *		For an eBPF program attached to a perf event, retrieve the
>>>> + *		sample data associated to *ctx*	and store it in the buffer
>>>> + *		pointed by *buf* up to size *size* bytes.
>>>> + *
>>>> + *		The *sample_flags* should contain a single value in the
>>>> + *		**enum perf_event_sample_format**.
>>>> + *	Return
>>>> + *		On success, number of bytes written to *buf*. On error, a
>>>> + *		negative value.
>>>> + *
>>>> + *		The *buf* can be set to **NULL** to return the number of bytes
>>>> + *		required to store the requested sample data.
>>>> + *
>>>> + *		**-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
>>>> + *
>>>> + *		**-ENOENT** if the associated perf event doesn't have the data.
>>>> + *
>>>> + *		**-ENOSYS** if system doesn't support the sample data to be
>>>> + *		retrieved.
>>>>    */
>>>>   #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>>>>   	FN(unspec, 0, ##ctx)				\
>>>> @@ -5695,6 +5717,7 @@ union bpf_attr {
>>>>   	FN(user_ringbuf_drain, 209, ##ctx)		\
>>>>   	FN(cgrp_storage_get, 210, ##ctx)		\
>>>>   	FN(cgrp_storage_delete, 211, ##ctx)		\
>>>> +	FN(perf_event_read_sample, 212, ##ctx)		\
>>>>   	/* */
>>>>     /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>> index ce0228c72a93..befd937afa3c 100644
>>>> --- a/kernel/trace/bpf_trace.c
>>>> +++ b/kernel/trace/bpf_trace.c
>>>> @@ -28,6 +28,7 @@
>>>>     #include <uapi/linux/bpf.h>
>>>>   #include <uapi/linux/btf.h>
>>>> +#include <uapi/linux/perf_event.h>
>>>>     #include <asm/tlb.h>
>>>>   @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
>>>>   	.arg4_type      = ARG_ANYTHING,
>>>>   };
>>>>   +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
>>>> +	   void *, buf, u32, size, u64, flags)
>>>> +{
>>> I wonder we could add perf_btf (like we have tp_btf) program type that
>>> could access ctx->data directly without helpers
>>
>> Martin and I have discussed an idea to introduce a generic helper like
>>     bpf_get_kern_ctx(void *ctx)
>> Given a context, the helper will return a PTR_TO_BTF_ID representing the
>> corresponding kernel ctx. So in the above example, user could call
>>
>>     struct bpf_perf_event_data_kern *kctx = bpf_get_kern_ctx(ctx);
>>     ...
> 
> This is an interesting idea!
> 
>> To implement bpf_get_kern_ctx helper, the verifier can find the type
>> of the context and provide a hidden btf_id as the second parameter of
>> the actual kernel helper function like
>>     bpf_get_kern_ctx(ctx) {
>>        return ctx;
>>     }
>>     /* based on ctx_btf_id, find kctx_btf_id and return it to verifier */
> 
> I think we will need a map of ctx_btf_id => kctx_btf_id. Shall we somehow
> expose this to the user?

Yes, inside the kernel we need ctx_btf_id -> kctx_btf_id mapping.
Good question. We might not want to this mapping as a stable API.
So using kfunc might be more appropriate.

> 
> Thanks,
> Song
> 
> 
>>     The bpf_get_kern_ctx helper can be inlined as well.
> 
>
  

On Thu, Nov 3, 2022 at 2:21 PM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 11/3/22 1:55 PM, Song Liu wrote:
> >
> >
> >> On Nov 3, 2022, at 12:45 PM, Yonghong Song <yhs@meta.com> wrote:
> >>
> >>
> >>
> >> On 11/1/22 3:02 AM, Jiri Olsa wrote:
> >>> On Mon, Oct 31, 2022 at 10:23:39PM -0700, Namhyung Kim wrote:
> >>>> The bpf_perf_event_read_sample() helper is to get the specified sample
> >>>> data (by using PERF_SAMPLE_* flag in the argument) from BPF to make a
> >>>> decision for filtering on samples.  Currently PERF_SAMPLE_IP and
> >>>> PERF_SAMPLE_DATA flags are supported only.
> >>>>
> >>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >>>> ---
> >>>>   include/uapi/linux/bpf.h       | 23 ++++++++++++++++
> >>>>   kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
> >>>>   tools/include/uapi/linux/bpf.h | 23 ++++++++++++++++
> >>>>   3 files changed, 95 insertions(+)
> >>>>
> >>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>>> index 94659f6b3395..cba501de9373 100644
> >>>> --- a/include/uapi/linux/bpf.h
> >>>> +++ b/include/uapi/linux/bpf.h
> >>>> @@ -5481,6 +5481,28 @@ union bpf_attr {
> >>>>    *               0 on success.
> >>>>    *
> >>>>    *               **-ENOENT** if the bpf_local_storage cannot be found.
> >>>> + *
> >>>> + * long bpf_perf_event_read_sample(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 sample_flags)
> >>>> + *        Description
> >>>> + *                For an eBPF program attached to a perf event, retrieve the
> >>>> + *                sample data associated to *ctx* and store it in the buffer
> >>>> + *                pointed by *buf* up to size *size* bytes.
> >>>> + *
> >>>> + *                The *sample_flags* should contain a single value in the
> >>>> + *                **enum perf_event_sample_format**.
> >>>> + *        Return
> >>>> + *                On success, number of bytes written to *buf*. On error, a
> >>>> + *                negative value.
> >>>> + *
> >>>> + *                The *buf* can be set to **NULL** to return the number of bytes
> >>>> + *                required to store the requested sample data.
> >>>> + *
> >>>> + *                **-EINVAL** if *sample_flags* is not a PERF_SAMPLE_* flag.
> >>>> + *
> >>>> + *                **-ENOENT** if the associated perf event doesn't have the data.
> >>>> + *
> >>>> + *                **-ENOSYS** if system doesn't support the sample data to be
> >>>> + *                retrieved.
> >>>>    */
> >>>>   #define ___BPF_FUNC_MAPPER(FN, ctx...)                   \
> >>>>    FN(unspec, 0, ##ctx)                            \
> >>>> @@ -5695,6 +5717,7 @@ union bpf_attr {
> >>>>    FN(user_ringbuf_drain, 209, ##ctx)              \
> >>>>    FN(cgrp_storage_get, 210, ##ctx)                \
> >>>>    FN(cgrp_storage_delete, 211, ##ctx)             \
> >>>> +  FN(perf_event_read_sample, 212, ##ctx)          \
> >>>>    /* */
> >>>>     /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
> >>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >>>> index ce0228c72a93..befd937afa3c 100644
> >>>> --- a/kernel/trace/bpf_trace.c
> >>>> +++ b/kernel/trace/bpf_trace.c
> >>>> @@ -28,6 +28,7 @@
> >>>>     #include <uapi/linux/bpf.h>
> >>>>   #include <uapi/linux/btf.h>
> >>>> +#include <uapi/linux/perf_event.h>
> >>>>     #include <asm/tlb.h>
> >>>>   @@ -1743,6 +1744,52 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
> >>>>    .arg4_type      = ARG_ANYTHING,
> >>>>   };
> >>>>   +BPF_CALL_4(bpf_perf_event_read_sample, struct bpf_perf_event_data_kern *, ctx,
> >>>> +     void *, buf, u32, size, u64, flags)
> >>>> +{
> >>> I wonder we could add perf_btf (like we have tp_btf) program type that
> >>> could access ctx->data directly without helpers
> >>
> >> Martin and I have discussed an idea to introduce a generic helper like
> >>     bpf_get_kern_ctx(void *ctx)
> >> Given a context, the helper will return a PTR_TO_BTF_ID representing the
> >> corresponding kernel ctx. So in the above example, user could call
> >>
> >>     struct bpf_perf_event_data_kern *kctx = bpf_get_kern_ctx(ctx);
> >>     ...
> >
> > This is an interesting idea!
> >
> >> To implement bpf_get_kern_ctx helper, the verifier can find the type
> >> of the context and provide a hidden btf_id as the second parameter of
> >> the actual kernel helper function like
> >>     bpf_get_kern_ctx(ctx) {
> >>        return ctx;
> >>     }
> >>     /* based on ctx_btf_id, find kctx_btf_id and return it to verifier */
> >
> > I think we will need a map of ctx_btf_id => kctx_btf_id. Shall we somehow
> > expose this to the user?
>
> Yes, inside the kernel we need ctx_btf_id -> kctx_btf_id mapping.
> Good question. We might not want to this mapping as a stable API.
> So using kfunc might be more appropriate.

Ok, now I don't think I'm following well.. ;-)

So currently perf event type BPF programs can have perf_event
data context directly as an argument, but we want to disallow it?
I guess the context id mapping can be done implicitly based on
the prog type and/or attach type, but probably I'm missing
something here. :)

Thanks,
Namhyung