linux-kernel - Re: [PATCH bpf-next v8 3/7] bpf: Generalize bpf_sk

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200819171215.lcgoon3fbm4kvkpc@kafai-mbp.dhcp.thefacebook.com>
Date:   Wed, 19 Aug 2020 10:12:15 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     KP Singh <kpsingh@...omium.org>
CC:     <linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
        <linux-security-module@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Paul Turner <pjt@...gle.com>, Jann Horn <jannh@...gle.com>,
        Florent Revest <revest@...omium.org>
Subject: Re: [PATCH bpf-next v8 3/7] bpf: Generalize bpf_sk_storage

On Wed, Aug 19, 2020 at 02:41:50PM +0200, KP Singh wrote:
> On 8/18/20 3:05 AM, Martin KaFai Lau wrote:
> > On Mon, Aug 03, 2020 at 06:46:51PM +0200, KP Singh wrote:
> >> From: KP Singh <kpsingh@...gle.com>
> >>
> >> Refactor the functionality in bpf_sk_storage.c so that concept of
> >> storage linked to kernel objects can be extended to other objects like
> >> inode, task_struct etc.
> >>
> >> Each new local storage will still be a separate map and provide its own
> >> set of helpers. This allows for future object specific extensions and
> >> still share a lot of the underlying implementation.
> >>
> >> This includes the changes suggested by Martin in:
> >>
> >>   https://lore.kernel.org/bpf/20200725013047.4006241-1-kafai@fb.com/
> >>
> >> which adds map_local_storage_charge, map_local_storage_uncharge,
> >> and map_owner_storage_ptr.
> > A description will still be useful in the commit message to talk
> > about the new map_ops, e.g.
> > they allow kernel object to optionally have different mem-charge strategy.
> > 
> >>
> >> Co-developed-by: Martin KaFai Lau <kafai@...com>
> >> Signed-off-by: KP Singh <kpsingh@...gle.com>
> >> ---
> >>  include/linux/bpf.h            |   9 ++
> >>  include/net/bpf_sk_storage.h   |  51 +++++++
> >>  include/uapi/linux/bpf.h       |   8 +-
> >>  net/core/bpf_sk_storage.c      | 246 +++++++++++++++++++++------------
> >>  tools/include/uapi/linux/bpf.h |   8 +-
> >>  5 files changed, 233 insertions(+), 89 deletions(-)
> >>
> 
> >> +			struct bpf_local_storage_map *smap,
> >> +			struct bpf_local_storage_elem *first_selem);
> >> +
> >> +struct bpf_local_storage_data *
> >> +bpf_local_storage_update(void *owner, struct bpf_map *map, void *value,
> > Nit.  It may be more consistent to take "struct bpf_local_storage_map *smap"
> > instead of "struct bpf_map *map" here.
> > 
> > bpf_local_storage_map_check_btf() will be the only one taking
> > "struct bpf_map *map".
> 
> That's because it is used in map operations as map_check_btf which expects
> a bpf_map *map pointer. We can wrap it in another function but is that
> worth doing?
Agree.  bpf_local_storage_map_check_btf() should stay as is.

I meant to only change the "bpf_local_storage_update()" to take
"struct bpf_local_storage_map *smap".

> > 
> >>  	 *
> >>  	 * The elem of this map can be cleaned up here
> >>  	 * or
> >> -	 * by bpf_sk_storage_free() during __sk_destruct().
> >> +	 * by bpf_local_storage_free() during the destruction of the
> >> +	 * owner object. eg. __sk_destruct.
> > This belongs to patch 1 also.
> 
> 
> In patch, 1, changed it to:
> 
> 	 * The elem of this map can be cleaned up here
> 	 * or when the storage is freed e.g.
> 	 * by bpf_sk_storage_free() during __sk_destruct().
>
+1