bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Roman Gushchin <guro@fb.com>, bpf@vger.kernel.org
Cc: netdev@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	kernel-team@fb.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v3 00/29] bpf: switch to memcg-based memory accounting
Date: Mon, 3 Aug 2020 14:05:29 +0200	[thread overview]
Message-ID: <6b1777ac-cae1-fa1f-db53-f6061d9ae675@iogearbox.net> (raw)
In-Reply-To: <20200730212310.2609108-1-guro@fb.com>

On 7/30/20 11:22 PM, Roman Gushchin wrote:
> Currently bpf is using the memlock rlimit for the memory accounting.
> This approach has its downsides and over time has created a significant
> amount of problems:
> 
> 1) The limit is per-user, but because most bpf operations are performed
>     as root, the limit has a little value.
> 
> 2) It's hard to come up with a specific maximum value. Especially because
>     the counter is shared with non-bpf users (e.g. memlock() users).
>     Any specific value is either too low and creates false failures
>     or too high and useless.
> 
> 3) Charging is not connected to the actual memory allocation. Bpf code
>     should manually calculate the estimated cost and precharge the counter,
>     and then take care of uncharging, including all fail paths.
>     It adds to the code complexity and makes it easy to leak a charge.
> 
> 4) There is no simple way of getting the current value of the counter.
>     We've used drgn for it, but it's far from being convenient.
> 
> 5) Cryptic -EPERM is returned on exceeding the limit. Libbpf even had
>     a function to "explain" this case for users.
> 
> In order to overcome these problems let's switch to the memcg-based
> memory accounting of bpf objects. With the recent addition of the percpu
> memory accounting, now it's possible to provide a comprehensive accounting
> of memory used by bpf programs and maps.
> 
> This approach has the following advantages:
> 1) The limit is per-cgroup and hierarchical. It's way more flexible and allows
>     a better control over memory usage by different workloads.
> 
> 2) The actual memory consumption is taken into account. It happens automatically
>     on the allocation time if __GFP_ACCOUNT flags is passed. Uncharging is also
>     performed automatically on releasing the memory. So the code on the bpf side
>     becomes simpler and safer.
> 
> 3) There is a simple way to get the current value and statistics.
> 
> The patchset consists of the following parts:
> 1) memcg-based accounting for various bpf objects: progs and maps
> 2) removal of the rlimit-based accounting
> 3) removal of rlimit adjustments in userspace samples

The diff stat looks nice & agree that rlimit sucks, but I'm missing how this is set
is supposed to work reliably, at least I currently fail to see it. Elaborating on this
in more depth especially for the case of unprivileged users should be a /fundamental/
part of the commit message.

Lets take an example: unprivileged user adds a max sized hashtable to one of its
programs, and configures the map that it will perform runtime allocation. The load
succeeds as it doesn't surpass the limits set for the current memcg. Kernel then
processes packets from softirq. Given the runtime allocations, we end up mischarging
to whoever ended up triggering __do_softirq(). If, for example, ksoftirq thread, then
it's probably reasonable to assume that this might not be accounted e.g. limits are
not imposed on the root cgroup. If so we would probably need to drag the context of
/where/ this must be charged to __memcg_kmem_charge_page() to do it reliably. Otherwise
how do you protect unprivileged users to OOM the machine?

Similarly, what happens to unprivileged users if kmemcg was not configured into the
kernel or has been disabled?

Thanks,
Daniel

  parent reply	other threads:[~2020-08-03 12:07 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30 21:22 [PATCH bpf-next v3 00/29] bpf: switch to memcg-based memory accounting Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 01/29] bpf: memcg-based memory accounting for bpf progs Roman Gushchin
2020-07-31 22:48   ` Song Liu
2020-07-30 21:22 ` [PATCH bpf-next v3 02/29] bpf: memcg-based memory accounting for bpf maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 03/29] bpf: refine memcg-based memory accounting for arraymap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 04/29] bpf: refine memcg-based memory accounting for cpumap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 05/29] bpf: memcg-based memory accounting for cgroup storage maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 06/29] bpf: refine memcg-based memory accounting for devmap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 07/29] bpf: refine memcg-based memory accounting for hashtab maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 08/29] bpf: memcg-based memory accounting for lpm_trie maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 09/29] bpf: memcg-based memory accounting for bpf ringbuffer Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 10/29] bpf: memcg-based memory accounting for socket storage maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 11/29] bpf: refine memcg-based memory accounting for sockmap and sockhash maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 12/29] bpf: refine memcg-based memory accounting for xskmap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 13/29] bpf: eliminate rlimit-based memory accounting for arraymap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 14/29] bpf: eliminate rlimit-based memory accounting for bpf_struct_ops maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 15/29] bpf: eliminate rlimit-based memory accounting for cpumap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 16/29] bpf: eliminate rlimit-based memory accounting for cgroup storage maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 17/29] bpf: eliminate rlimit-based memory accounting for devmap maps Roman Gushchin
2020-07-30 21:22 ` [PATCH bpf-next v3 18/29] bpf: eliminate rlimit-based memory accounting for hashtab maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 19/29] bpf: eliminate rlimit-based memory accounting for lpm_trie maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 20/29] bpf: eliminate rlimit-based memory accounting for queue_stack_maps maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 21/29] bpf: eliminate rlimit-based memory accounting for reuseport_array maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 22/29] bpf: eliminate rlimit-based memory accounting for bpf ringbuffer Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 23/29] bpf: eliminate rlimit-based memory accounting for sockmap and sockhash maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 24/29] bpf: eliminate rlimit-based memory accounting for stackmap maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 25/29] bpf: eliminate rlimit-based memory accounting for socket storage maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 26/29] bpf: eliminate rlimit-based memory accounting for xskmap maps Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 27/29] bpf: eliminate rlimit-based memory accounting infra for bpf maps Roman Gushchin
2020-07-31 22:47   ` Song Liu
2020-07-30 21:23 ` [PATCH bpf-next v3 28/29] bpf: eliminate rlimit-based memory accounting for bpf progs Roman Gushchin
2020-07-30 21:23 ` [PATCH bpf-next v3 29/29] bpf: samples: do not touch RLIMIT_MEMLOCK Roman Gushchin
2020-08-03 12:05 ` Daniel Borkmann [this message]
2020-08-03 15:34   ` [PATCH bpf-next v3 00/29] bpf: switch to memcg-based memory accounting Roman Gushchin
2020-08-03 16:39     ` Daniel Borkmann
2020-08-03 17:05       ` Roman Gushchin
2020-08-03 18:37         ` Daniel Borkmann
2020-08-03 19:06           ` Roman Gushchin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6b1777ac-cae1-fa1f-db53-f6061d9ae675@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).