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

Message-ID: <X9bA/pSYxW079eYm@rdna-mbp.dhcp.thefacebook.com>
Date:   Sun, 13 Dec 2020 17:33:50 -0800
From:   Andrey Ignatov <rdna@...com>
To:     Cong Wang <xiyou.wangcong@...il.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

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.

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.

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.

-- 
Andrey Ignatov