netdev - Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAEf4BzZ4e+pjWqyAj-MEVY2pJi8Eg35OrGUbqACaV_WLdM9C8A@mail.gmail.com>
Date:   Tue, 27 Jul 2021 16:48:31 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size

On Tue, Jul 27, 2021 at 4:25 PM <sdf@...gle.com> wrote:
>
> On 07/27, Andrii Nakryiko wrote:
> > On Tue, Jul 27, 2021 at 3:23 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > Current max cgroup storage value size is 4k (PAGE_SIZE). The other local
> > > storages accept up to 64k (BPF_LOCAL_STORAGE_MAX_VALUE_SIZE). Let's
> > align
> > > max cgroup value size with the other storages.
> > >
> > > For percpu, the max is 32k (PCPU_MIN_UNIT_SIZE) because percpu
> > > allocator is not happy about larger values.
> > >
> > > netcnt test is extended to exercise those maximum values
> > > (non-percpu max size is close to, but not real max).
> > >
> > > v4:
> > > * remove inner union (Andrii Nakryiko)
> > > * keep net_cnt on the stack (Andrii Nakryiko)
> > >
> > > v3:
> > > * refine SIZEOF_BPF_LOCAL_STORAGE_ELEM comment (Yonghong Song)
> > > * anonymous struct in percpu_net_cnt & net_cnt (Yonghong Song)
> > > * reorder free (Yonghong Song)
> > >
> > > v2:
> > > * cap max_value_size instead of BUILD_BUG_ON (Martin KaFai Lau)
> > >
> > > Cc: Martin KaFai Lau <kafai@...com>
> > > Cc: Yonghong Song <yhs@...com>
> > > Cc: Andrii Nakryiko <andrii@...nel.org>
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
>
> > Added Martin's ack and applied to bpf-next. Please carry over received
> > Acks between revisions.
> Ah, sorry, forgot about it :-(
>
> > It's also a good practice to separate selftest from the kernel (or
> > libbpf) changes, unless kernel change doesn't immediately break
> > selftest. Please consider doing that for the future.
> I've actually seen some back and forth on this one. I used to split
> them in the past (assuming it makes it easy to do the
> backports/cherry-picks), but I remember at some point it was
> suggested not to split them for small changes like this.

So we asked to split UAPI header sync for tools/include/ into a
separate patch initially. But then I just improved libbpf's sync
script to handle that regardless and we stopped asking for that. But
the libbpf vs kernel vs selftests split was always (perhaps
implicitly) advised. Personally, I've only had a few cases where
selftest changes had to go in with kernel changes in the same patch to
avoid breaking selftests. In all other cases it's cleaner to have them
split out.

>
> Might be a good idea to document this (when and if to separate
> libbpf/selftests)
> on bpf_devel_QA.rst
>
> > I also just noticed that test_netcnt isn't part of test_progs. It
> > would be great to migrate it under the common test_progs
> > infrastructure. We've been steadily moving towards that, but there are
> > still a bunch of tests that are not run in CI.
> SG, I might do a follow up on this one.

Sounds good, thanks!