linux-kernel - Re: [PATCH v6 bpf-next 1/2] bpf: Add bpf_read_branch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <C06T0N0N8TD7.1MBJSGUZDFW0A@dlxu-fedora-R90QNFJV>
Date:   Mon, 27 Jan 2020 11:01:08 -0800
From:   "Daniel Xu" <dxu@...uu.xyz>
To:     "Daniel Borkmann" <daniel@...earbox.net>, <bpf@...r.kernel.org>,
        <ast@...nel.org>, <songliubraving@...com>, <yhs@...com>,
        <andriin@...com>
Cc:     <linux-kernel@...r.kernel.org>, <kernel-team@...com>,
        <peterz@...radead.org>, <mingo@...hat.com>, <acme@...nel.org>
Subject: Re: [PATCH v6 bpf-next 1/2] bpf: Add bpf_read_branch_records()
 helper

On Mon Jan 27, 2020 at 1:26 PM, Daniel Borkmann wrote:
[...]
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index f1d74a2bd234..332aa433d045 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2892,6 +2892,25 @@ union bpf_attr {
> >    *		Obtain the 64bit jiffies
> >    *	Return
> >    *		The 64 bit jiffies
> > + *
> > + * int bpf_read_branch_records(struct bpf_perf_event_data *ctx, void *buf, u32 buf_size, u64 flags)
>
> 
> Small nit: s/buf_size/size/, so that it matches with your BPF_CALL
> below.

Ok

> 
> +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *,
> ctx,
> + void *, buf, u32, size, u64, flags)
>
> 
> > + *	Description
> > + *		For an eBPF program attached to a perf event, retrieve the
> > + *		branch records (struct perf_branch_entry) associated to *ctx*
> > + *		and store it in	the buffer pointed by *buf* up to size
> > + *		*buf_size* bytes.
> > + *
> > + *		The *flags* can be set to **BPF_F_GET_BRANCH_RECORDS_SIZE** to
> > + *		instead	return the number of bytes required to store all the
> > + *		branch entries. If this flag is set, *buf* may be NULL.
> > + *	Return
> > + *		On success, number of bytes written to *buf*. On error, a
> > + *		negative value.
>
> 
> Maybe pull the 2nd paragraph from above in here so that it reflects the
> description
> of the return value when flag is used also for this case in the 'Return'
> description.

Ok.

[...]
> >   
> > +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx,
> > +	   void *, buf, u32, size, u64, flags)
> > +{
> > +#ifndef CONFIG_X86
> > +	return -ENOENT;
> > +#else
> > +	struct perf_branch_stack *br_stack = ctx->data->br_stack;
> > +	u32 br_entry_size = sizeof(struct perf_branch_entry);
>
> 
> 'static const u32 br_entry_size' if we use it as such below.

Ok

>
> 
> > +	u32 to_copy;
> > +
> > +	if (unlikely(flags & ~BPF_F_GET_BRANCH_RECORDS_SIZE))
> > +		return -EINVAL;
> > +
> > +	if (unlikely(!br_stack))
> > +		return -EINVAL;
>
> 
> Why the ifdef X86? In previous thread I meant to change it into since
> it's
> implicit:
>
> 
> if (unlikely(!br_stack))
> return -ENOENT;
>
> 
> Or is there any other additional rationale?

Yeah, so br_stack can be null if the perf_event is misconfigured (branch
record not enabled). So we need to differentiate that from arch not
supporting it.

[...]


Thanks,
Daniel