From: Alexei Starovoitov <ast@fb.com>
To: Yonghong Song <yhs@fb.com>, <bpf@vger.kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>, <kernel-team@fb.com>,
Jiri Olsa <jolsa@kernel.org>, Roman Gushchin <guro@fb.com>
Subject: Re: [PATCH bpf-next v4] bpf: fix NULL pointer dereference in bpf_get_local_storage() helper
Date: Thu, 25 Mar 2021 18:38:54 -0700 [thread overview]
Message-ID: <3a2052e8-eb4b-fefc-4a0c-ad051b5609d0@fb.com> (raw)
In-Reply-To: <20210323055146.3334476-1-yhs@fb.com>
On 3/22/21 10:51 PM, Yonghong Song wrote:
> Jiri Olsa reported a bug ([1]) in kernel where cgroup local
> storage pointer may be NULL in bpf_get_local_storage() helper.
> There are two issues uncovered by this bug:
> (1). kprobe or tracepoint prog incorrectly sets cgroup local storage
> before prog run,
> (2). due to change from preempt_disable to migrate_disable,
> preemption is possible and percpu storage might be overwritten
> by other tasks.
>
> This issue (1) is fixed in [2]. This patch tried to address issue (2).
> The following shows how things can go wrong:
> task 1: bpf_cgroup_storage_set() for percpu local storage
> preemption happens
> task 2: bpf_cgroup_storage_set() for percpu local storage
> preemption happens
> task 1: run bpf program
>
> task 1 will effectively use the percpu local storage setting by task 2
> which will be either NULL or incorrect ones.
>
> Instead of just one common local storage per cpu, this patch fixed
> the issue by permitting 8 local storages per cpu and each local
> storage is identified by a task_struct pointer. This way, we
> allow at most 8 nested preemption between bpf_cgroup_storage_set()
> and bpf_cgroup_storage_unset(). The percpu local storage slot
> is released (calling bpf_cgroup_storage_unset()) by the same task
> after bpf program finished running.
> bpf_test_run() is also fixed to use the new bpf_cgroup_storage_set()
> interface.
>
> The patch is tested on top of [2] with reproducer in [1].
> Without this patch, kernel will emit error in 2-3 minutes.
> With this patch, after one hour, still no error.
>
> [1] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T
> [2] https://lore.kernel.org/bpf/20210309185028.3763817-1-yhs@fb.com
>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Roman Gushchin <guro@fb.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
Applied to bpf-next. Thanks
next prev parent reply other threads:[~2021-03-26 1:40 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-23 5:51 [PATCH bpf-next v4] bpf: fix NULL pointer dereference in bpf_get_local_storage() helper Yonghong Song
2021-03-26 1:38 ` Alexei Starovoitov [this message]
2021-03-26 11:50 ` Jiri Olsa
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=3a2052e8-eb4b-fefc-4a0c-ad051b5609d0@fb.com \
--to=ast@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=guro@fb.com \
--cc=jolsa@kernel.org \
--cc=kernel-team@fb.com \
--cc=yhs@fb.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.