netdev - Re: [PATCH bpf-next] bpf: add optional memory accounting for maps

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: <CAH3MdRW1V=BwmLYgo55=-xPuxJy8NKcXZjRPVJnApQ84Gd8CFw@mail.gmail.com>
Date:   Wed, 30 Jan 2019 23:15:06 -0800
From:   Y Song <ys114321@...il.com>
To:     Martynas Pumputis <m@...bda.lt>
Cc:     netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next] bpf: add optional memory accounting for maps

On Wed, Jan 30, 2019 at 6:05 AM Martynas Pumputis <m@...bda.lt> wrote:
>
> Previously, memory allocated for a map was not accounted. Therefore,
> this memory could not be taken into consideration by the cgroups
> memory controller.
>
> This patch introduces the "BPF_F_ACCOUNT_MEM" flag which enables
> the memory accounting for a map, and it can be set during
> the map creation ("BPF_MAP_CREATE") in "map_flags".
>
> When enabled, we account only that amount of memory which is charged
> against the "RLIMIT_MEMLOCK" limit.
>
> To validate the change, first we create the memory cgroup "test-map":
>
>     # mkdir /sys/fs/cgroup/memory/test-map
>
> And then we run the following program against the cgroup:
>
>     $ cat test_map.c
>     <..>
>     int main() {
>         usleep(3 * 1000000);
>         assert(bpf_create_map(BPF_MAP_TYPE_HASH, 8, 16, 65536, 0) > 0);
>         usleep(3 * 1000000);
>     }
>     # cgexec -g memory:test-map ./test_map &
>     # cat /sys/fs/cgroup/memory/test-map/memory{,.kmem}.usage_in_bytes
>     397312
>     258048
>
>     <after 3 sec the map has been created>
>
>     # bpftool map list
>     19: hash  flags 0x0
>         key 8B  value 16B  max_entries 65536  memlock 5771264B
>     # cat /sys/fs/cgroup/memory/test-map/memory{,.kmem}.usage_in_bytes
>     401408
>     262144
>
> As we can see, the memory allocated for map is not accounted, as
> 397312B + 5771264B > 401408B.
>
> Next, we enabled the accounting and re-run the test:
>
>     $ cat test_map.c
>     <..>
>     int main() {
>         usleep(3 * 1000000);
>         assert(bpf_create_map(BPF_MAP_TYPE_HASH, 8, 16, 65536, BPF_F_ACCOUNT_MEM) > 0);
>         usleep(3 * 1000000);
>     }
>     # cgexec -g memory:test-map ./test_map &
>     # cat /sys/fs/cgroup/memory/test-map/memory{,.kmem}.usage_in_bytes
>     450560
>     307200
>
>     <after 3 sec the map has been created>
>
>     # bpftool map list
>     20: hash  flags 0x80
>         key 8B  value 16B  max_entries 65536  memlock 5771264B
>     # cat /sys/fs/cgroup/memory/test-map/memory{,.kmem}.usage_in_bytes
>     6221824
>     6078464
>
> This time, the memory (including kmem) is accounted, as
> 450560B + 5771264B <= 6221824B

The above test result is for cgroup v1.
I also tested the patch for cgroup v2. It works fine too.

>
> Signed-off-by: Martynas Pumputis <m@...bda.lt>
> ---
>  include/linux/bpf.h            |  5 +++--
>  include/uapi/linux/bpf.h       |  2 ++
>  kernel/bpf/arraymap.c          | 14 +++++++++-----
>  kernel/bpf/bpf_lru_list.c      | 11 +++++++++--
>  kernel/bpf/bpf_lru_list.h      |  1 +
>  kernel/bpf/cpumap.c            | 12 +++++++++---
>  kernel/bpf/devmap.c            | 10 ++++++++--
>  kernel/bpf/hashtab.c           | 19 ++++++++++++++-----
>  kernel/bpf/lpm_trie.c          | 19 +++++++++++++------
>  kernel/bpf/queue_stack_maps.c  |  5 +++--
>  kernel/bpf/reuseport_array.c   |  3 ++-
>  kernel/bpf/stackmap.c          | 12 ++++++++----
>  kernel/bpf/syscall.c           | 12 ++++++++----
>  kernel/bpf/xskmap.c            |  9 +++++++--
>  net/core/sock_map.c            | 13 +++++++++----
>  tools/include/uapi/linux/bpf.h |  3 +++
>  16 files changed, 108 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e734f163bd0b..353a3f4304fe 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -79,7 +79,8 @@ struct bpf_map {
>         u32 btf_value_type_id;
>         struct btf *btf;
>         bool unpriv_array;
> -       /* 55 bytes hole */
> +       bool account_mem;
> +       /* 54 bytes hole */
>
>         /* The 3rd and 4th cacheline with misc members to avoid false sharing
>          * particularly with refcounting.
> @@ -506,7 +507,7 @@ void bpf_map_put(struct bpf_map *map);
>  int bpf_map_precharge_memlock(u32 pages);
>  int bpf_map_charge_memlock(struct bpf_map *map, u32 pages);
>  void bpf_map_uncharge_memlock(struct bpf_map *map, u32 pages);
> -void *bpf_map_area_alloc(size_t size, int numa_node);
> +void *bpf_map_area_alloc(size_t size, int numa_node, bool account_mem);
>  void bpf_map_area_free(void *base);
>  void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 91c43884f295..a374ccbaa51b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -278,6 +278,8 @@ enum bpf_attach_type {
>  #define BPF_F_NO_COMMON_LRU    (1U << 1)
>  /* Specify numa node during map creation */
>  #define BPF_F_NUMA_NODE                (1U << 2)
> +/* Enable memory accounting for map */
> +#define BPF_F_ACCOUNT_MEM      (1U << 7)

Could you put the above definition after BPF_F_ZERO_SEED (1U << 6)?
The same for tools.
With this change, you can add my ack:
Acked-by: Yonghong Song <yhs@...com>