netdev - Re: [Patch bpf-next 0/3] bpf: introduce timeout map

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: <CAM_iQpX9TDN0RQuhSHsk72fy5akAh-iiB3e2A6PE6KqAfiV=pw@mail.gmail.com>
Date:   Mon, 14 Dec 2020 10:40:34 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Cong Wang <cong.wang@...edance.com>
Subject: Re: [Patch bpf-next 0/3] bpf: introduce timeout map

On Sun, Dec 13, 2020 at 5:33 PM Andrey Ignatov <rdna@...com> wrote:
>
> Cong Wang <xiyou.wangcong@...il.com> [Sat, 2020-12-12 15:18 -0800]:
> > On Sat, Dec 12, 2020 at 2:25 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > >
> > > On Fri, Dec 11, 2020 at 11:55 AM Andrii Nakryiko
> > > <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > On Fri, Dec 11, 2020 at 2:28 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > > > >
> > > > > From: Cong Wang <cong.wang@...edance.com>
> > > > >
> > > > > This patchset introduces a new bpf hash map which has timeout.
> > > > > Patch 1 is a preparation, patch 2 is the implementation of timeout
> > > > > map, patch 3 contains a test case for timeout map. Please check each
> > > > > patch description for more details.
> > > > >
> > > > > ---
> > > >
> > > > This patch set seems to be breaking existing selftests. Please take a
> > > > look ([0]).
> > >
> > > Interesting, looks unrelated to my patches but let me double check.
> >
> > Cc'ing Andrey...
> >
> > Looks like the failure is due to the addition of a new member to struct
> > htab_elem. Any reason why it is hard-coded as 64 in check_hash()?
> > And what's the point of verifying its size? htab_elem should be only
> > visible to the kernel itself.
> >
> > I can certainly change 64 to whatever its new size is, but I do wonder
> > why the test is there.
>
> Cong, the test is there to make sure that access to map pointers from
> BPF program works.
>
> Please see (41c48f3a9823 "bpf: Support access to bpf map fields") for
> more details on what "access to map pointer" means, but it's basically a
> way to access any field (e.g. max_entries) of common `struct bpf_map` or
> any type-specific struct like `struct bpf_htab` from BPF program, i.e.
> these structs are visible to not only kernel but also to BPF programs.

I see, I was not aware of this.

>
> The point of the test is to access a few fields from every map struct
> and make sure it works. Changing `struct htab_elem` indeed breaks the
> `VERIFY(hash->elem_size == 64);` check. But it can be easily updated
> (from 64 to whatever new size is) or replaced by some other field check.
> `htab->elem_size` was chosen semi-randomly since any bpf_htab-specific
> field would work for the test's purposes.

Good to know it is useful, I will have to change 64 to 72, as I tried to use
sizeof but struct htab_elem is not visible to that test.

>
> Hope it clarifies.
>
> Also since you add a new map type it would be great to cover it in
> tools/testing/selftests/bpf/progs/map_ptr_kern.c as well.

Yeah, will do.

Thanks.