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]

Date:   Fri, 01 Feb 2019 13:03:45 +0200
From:   Martynas <m@...bda.lt>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        Y Song <ys114321@...il.com>
Subject: Re: [PATCH bpf-next] bpf: add optional memory accounting for maps



On Thu, Jan 31, 2019, at 8:35 PM, Alexei Starovoitov wrote:
> On Wed, Jan 30, 2019 at 03:02:51PM +0100, Martynas Pumputis 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
> > 
> > Signed-off-by: Martynas Pumputis <m@...bda.lt>
> ...
> > @@ -49,7 +51,9 @@ static struct bpf_map *xsk_map_alloc(union bpf_attr *attr)
> >  
> >  	err = -ENOMEM;
> >  
> > -	m->flush_list = alloc_percpu(struct list_head);
> > +	if (account_mem)
> > +		gfp |= __GFP_ACCOUNT;
> > +	m->flush_list = alloc_percpu_gfp(struct list_head, gfp);
> 
> I think it's better to account this memory by default.
> Extra flag during map creation is not needed.

The main reason I made the accounting optional is that otherwise it could break existing BPF map users who have set cgroup memory limits.

> There are nokmem and nosocket memcg boot options.
> We can add one more to turn off accounting of bpf map memory.
> 

Considering the suggested boot option, I'm OK with enabling it by default. Do you want me to add the option as part of this PR?