Re: BPF map freezing is unreliable; can we instead just inline constants?

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jann Horn <jannh@google.com>
Cc: bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Matthew Garrett <mjg59@google.com>, KP Singh <kpsingh@google.com>
Subject: Re: BPF map freezing is unreliable; can we instead just inline constants?
Date: Wed, 15 Apr 2020 22:42:28 -0700	[thread overview]
Message-ID: <CAEf4BzYGWYhXdp6BJ7_=9OQPJxQpgug080MMjdSB72i9R+5c6g@mail.gmail.com> (raw)
In-Reply-To: <CAG48ez1Bs8_3_+uUB69Qe3RN7tDgD8PcBrzv1H0fqbvd0f4jPw@mail.gmail.com>

On Wed, Apr 15, 2020 at 1:04 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Apr 15, 2020 at 9:07 PM Jann Horn <jannh@google.com> wrote:
> > On Wed, Apr 15, 2020 at 6:59 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > On Tue, Apr 14, 2020 at 3:50 PM Jann Horn <jannh@google.com> wrote:
> > > > On Tue, Apr 14, 2020 at 9:46 PM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > On Tue, Apr 14, 2020 at 9:07 AM Jann Horn <jannh@google.com> wrote:
> > > > > > On Fri, Apr 10, 2020 at 10:48 PM Andrii Nakryiko
> > > > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > > > On Fri, Apr 10, 2020 at 1:47 AM Jann Horn <jannh@google.com> wrote:
> > > > > > > > On Fri, Apr 10, 2020 at 1:33 AM Andrii Nakryiko
> > > > > > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > > > > > On Wed, Apr 8, 2020 at 12:42 PM Jann Horn <jannh@google.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi!
> > > > > > > > > >
> > > > > > > > > > I saw that BPF allows root to create frozen maps, for which the
> > > > > > > > > > verifier then assumes that they contain constant values. However, map
> > > > > > > > > > freezing is pretty wobbly:
> [...]
> > > > > I'd say
> > > > > the better way would be to implement immutable BPF maps from the time
> > > > > they are created. E.g., at the time of creating map, you specify extra
> > > > > flag BPF_F_IMMUTABLE and specify pointer to a blob of memory with
> > > > > key/value pairs in it.
> > > >
> > > > It seems a bit hacky to me to add a new special interface for
> > > > populating an immutable map. Wouldn't it make more sense to add a flag
> > > > for "you can't use mmap on this map", or "I plan to freeze this map",
> > > > or something like that, and keep the freezing API?
> > >
> > > "you can't use mmap on this map" is default behavior, unless you
> > > specify BPF_F_MMAPABLE.
> >
> > Ah, right.
> >
> > > "I plan to freeze this map" could be added,
> > > but how that would help existing users that freeze and mmap()?
> > > Disallowing those now would be a breaking change.
> > > Currently, libbpf is using freezing for .rodata variables, but it
> > > doesn't mmap() before freezing.
> >
> > Okay, so it sounds like there are probably no actual users that use
> > both BPF_F_MMAPABLE and freezing, and so we can just forbid that
> > combination? That sounds great.
>
> kpsingh pointed out to me that bpf_object__load_skeleton() has code
> specifically for mmap()ing BPF_F_RDONLY_PROG maps, so this might not
> work... but perhaps we could make `BPF_F_RDONLY_PROG|BPF_F_MMAPABLE`
> imply "you can only map with PROT_READ, not with PROT_WRITE"?
> bpf_object__load_skeleton() only maps BPF_F_RDONLY_PROG maps with
> PROT_READ.

I think that's reasonable and given the intent of frozen read-only
map, shouldn't break any reasonable use case. I'll send a patch soon.