| 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
| ||
|
Message-ID: <ea6acda9-417f-af4e-22df-7fcb0f50598b@fb.com>
Date: Mon, 26 Jul 2021 15:35:50 -0700
From: Yonghong Song <yhs@...com>
To: <sdf@...gle.com>
CC: <netdev@...r.kernel.org>, <bpf@...r.kernel.org>, <ast@...nel.org>,
<daniel@...earbox.net>, <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v2] bpf: increase supported cgroup storage value
size
On 7/26/21 8:39 AM, sdf@...gle.com wrote:
> On 07/25, Yonghong Song wrote:
>
>
>> On 7/23/21 4:54 PM, Stanislav Fomichev 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).
>> >
>> > v2:
>> > * cap max_value_size instead of BUILD_BUG_ON (Martin KaFai Lau)
>> >
>> > Cc: Martin KaFai Lau <kafai@...com>
>> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
>> > ---
>> > kernel/bpf/local_storage.c | 11 +++++-
>> > tools/testing/selftests/bpf/netcnt_common.h | 38
>> +++++++++++++++----
>> > .../testing/selftests/bpf/progs/netcnt_prog.c | 29 +++++++-------
>> > tools/testing/selftests/bpf/test_netcnt.c | 25 +++++++-----
>> > 4 files changed, 72 insertions(+), 31 deletions(-)
>> >
>> > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
>> > index 7ed2a14dc0de..035e9e3a7132 100644
>> > --- a/kernel/bpf/local_storage.c
>> > +++ b/kernel/bpf/local_storage.c
>> > @@ -1,6 +1,7 @@
>> > //SPDX-License-Identifier: GPL-2.0
>> > #include <linux/bpf-cgroup.h>
>> > #include <linux/bpf.h>
>> > +#include <linux/bpf_local_storage.h>
>> > #include <linux/btf.h>
>> > #include <linux/bug.h>
>> > #include <linux/filter.h>
>> > @@ -283,9 +284,17 @@ static int cgroup_storage_get_next_key(struct
>> bpf_map *_map, void *key,
>> > static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr)
>> > {
>> > + __u32 max_value_size = BPF_LOCAL_STORAGE_MAX_VALUE_SIZE;
>> > int numa_node = bpf_map_attr_numa_node(attr);
>> > struct bpf_cgroup_storage_map *map;
>> > + /* percpu is bound by PCPU_MIN_UNIT_SIZE, non-percu
>> > + * is the same as other local storages.
>> > + */
>> > + if (attr->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
>> > + max_value_size = min_t(__u32, max_value_size,
>> > + PCPU_MIN_UNIT_SIZE);
>> > +
>> > if (attr->key_size != sizeof(struct bpf_cgroup_storage_key) &&
>> > attr->key_size != sizeof(__u64))
>> > return ERR_PTR(-EINVAL);
>> > @@ -293,7 +302,7 @@ static struct bpf_map
>> *cgroup_storage_map_alloc(union bpf_attr *attr)
>> > if (attr->value_size == 0)
>> > return ERR_PTR(-EINVAL);
>> > - if (attr->value_size > PAGE_SIZE)
>> > + if (attr->value_size > max_value_size)
>> > return ERR_PTR(-E2BIG);
>> > if (attr->map_flags & ~LOCAL_STORAGE_CREATE_FLAG_MASK ||
>> > diff --git a/tools/testing/selftests/bpf/netcnt_common.h
>> b/tools/testing/selftests/bpf/netcnt_common.h
>> > index 81084c1c2c23..dfcf184ff713 100644
>> > --- a/tools/testing/selftests/bpf/netcnt_common.h
>> > +++ b/tools/testing/selftests/bpf/netcnt_common.h
>> > @@ -6,19 +6,43 @@
>> > #define MAX_PERCPU_PACKETS 32
>> > +/* sizeof(struct bpf_local_storage_elem):
>> > + *
>> > + * It really is about 128 bytes, but allocate more to account for
>> possible
>
>> Maybe more specific to be 128 bytes on x86_64? As suggested below, 32bit
>> architecture may have smaller size.
>
>> Looks like that the size of struct bpf_local_storage_elem won't change
>> anytime soon, so it is probably okay to mention 128 bytes here.
> SG, I'll clarify that it's x86_64 only. Or do you want me to use 128
> as SIZEOF_BPF_LOCAL_STORAGE_ELEM ?
Just clarify it is x86_64 only.
>
>> > + * layout changes, different architectures, etc.
>> > + * It will wrap up to PAGE_SIZE internally anyway.
>
>> What will be wrap up to PAGE_SIZE? More explanations?
>
> I'll reword to the following:
>
> It really is about 128 bytes on x86_64, but allocate more to account for
> possible layout changes, different architectures, etc.
> The kernel will wrap up to PAGE_SIZE internally anyway.
This is better.
>
> (clarify that the kernel will wrap it up internally)
>
>> > + */
>> > +#define SIZEOF_BPF_LOCAL_STORAGE_ELEM 256
>> > +
>> > +/* Try to estimate kernel's BPF_LOCAL_STORAGE_MAX_VALUE_SIZE: */
>> > +#define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE (0xFFFF - \
>> > + SIZEOF_BPF_LOCAL_STORAGE_ELEM)
>> > +
>> > +#define PCPU_MIN_UNIT_SIZE 32768
>> > +
>> > struct percpu_net_cnt {
>> > - __u64 packets;
>> > - __u64 bytes;
>> > + union {
>> > + struct {
>> > + __u64 packets;
>> > + __u64 bytes;
>> > - __u64 prev_ts;
>> > + __u64 prev_ts;
>> > - __u64 prev_packets;
>> > - __u64 prev_bytes;
>> > + __u64 prev_packets;
>> > + __u64 prev_bytes;
>> > + } val;
>
>> You don't need 'val' here. This way the code churn can be reduced.
> Good idea, dropping for both.
>
>> > + __u8 data[PCPU_MIN_UNIT_SIZE];
>> > + };
>> > };
>> > struct net_cnt {
>> > - __u64 packets;
>> > - __u64 bytes;
>> > + union {
>> > + struct {
>> > + __u64 packets;
>> > + __u64 bytes;
>> > + } val;
>
>> The same here. 'val' is not needed.
>
>> > + __u8 data[BPF_LOCAL_STORAGE_MAX_VALUE_SIZE];
>> > + };
>> > };
>> > #endif
>> > diff --git a/tools/testing/selftests/bpf/progs/netcnt_prog.c
>> b/tools/testing/selftests/bpf/progs/netcnt_prog.c
>> > index d071adf178bd..4b0884239892 100644
>> > --- a/tools/testing/selftests/bpf/progs/netcnt_prog.c
>> > +++ b/tools/testing/selftests/bpf/progs/netcnt_prog.c
>> > @@ -34,34 +34,35 @@ int bpf_nextcnt(struct __sk_buff *skb)
>> > cnt = bpf_get_local_storage(&netcnt, 0);
>> > percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
>> > - percpu_cnt->packets++;
>> > - percpu_cnt->bytes += skb->len;
>> > + percpu_cnt->val.packets++;
>> > + percpu_cnt->val.bytes += skb->len;
>> > - if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
>> > - __sync_fetch_and_add(&cnt->packets,
>> > - percpu_cnt->packets);
>> > - percpu_cnt->packets = 0;
>> > + if (percpu_cnt->val.packets > MAX_PERCPU_PACKETS) {
>> > + __sync_fetch_and_add(&cnt->val.packets,
>> > + percpu_cnt->val.packets);
>> > + percpu_cnt->val.packets = 0;
>> > - __sync_fetch_and_add(&cnt->bytes,
>> > - percpu_cnt->bytes);
>> > - percpu_cnt->bytes = 0;
>> > + __sync_fetch_and_add(&cnt->val.bytes,
>> > + percpu_cnt->val.bytes);
>> > + percpu_cnt->val.bytes = 0;
>> > }
>> > ts = bpf_ktime_get_ns();
>> > - dt = ts - percpu_cnt->prev_ts;
>> > + dt = ts - percpu_cnt->val.prev_ts;
>> > dt *= MAX_BPS;
>> > dt /= NS_PER_SEC;
>> > - if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
>> > + if (cnt->val.bytes + percpu_cnt->val.bytes -
>> > + percpu_cnt->val.prev_bytes < dt)
>> > ret = 1;
>> > else
>> > ret = 0;
>> > if (dt > REFRESH_TIME_NS) {
>> > - percpu_cnt->prev_ts = ts;
>> > - percpu_cnt->prev_packets = cnt->packets;
>> > - percpu_cnt->prev_bytes = cnt->bytes;
>> > + percpu_cnt->val.prev_ts = ts;
>> > + percpu_cnt->val.prev_packets = cnt->val.packets;
>> > + percpu_cnt->val.prev_bytes = cnt->val.bytes;
>> > }
>> > return !!ret;
>> > diff --git a/tools/testing/selftests/bpf/test_netcnt.c
>> b/tools/testing/selftests/bpf/test_netcnt.c
>> > index a7b9a69f4fd5..1138765406a5 100644
>> > --- a/tools/testing/selftests/bpf/test_netcnt.c
>> > +++ b/tools/testing/selftests/bpf/test_netcnt.c
>> > @@ -33,11 +33,11 @@ static int bpf_find_map(const char *test, struct
>> bpf_object *obj,
>> > int main(int argc, char **argv)
>> > {
>> > - struct percpu_net_cnt *percpu_netcnt;
>> > + struct percpu_net_cnt *percpu_netcnt = NULL;
>
>> Assigning NULL is not needed, right?
> Yeah, it's not needed, but I'd prefer to have it for consistency
> (so both free() work regardless of the ordering and future changes).
> Let me know if you strongly disagree, I can drop it.
That is fine. The compiler will probably remove the initialization.
>
>> > struct bpf_cgroup_storage_key key;
>> > + struct net_cnt *netcnt = NULL;
>> > int map_fd, percpu_map_fd;
>> > int error = EXIT_FAILURE;
>> > - struct net_cnt netcnt;
>> > struct bpf_object *obj;
>> > int prog_fd, cgroup_fd;
>> > unsigned long packets;
>> > @@ -52,6 +52,12 @@ int main(int argc, char **argv)
[...]
Powered by blists - more mailing lists