Re: [PATCH net v4] bpf, net: Check cgroup_bpf_enabled() only once in do_sock_getsockopt()
From: Alexei Starovoitov <hidden>
Date: 2024-08-21 21:01:21
Also in:
bpf, linux-mediatek, lkml, netdev
On Wed, Aug 21, 2024 at 2:30 AM Tze-nan Wu [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The return value from `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` can change between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and `BPF_CGROUP_RUN_PROG_GETSOCKOPT`. If `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` changes from "false" to "true" between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and `BPF_CGROUP_RUN_PROG_GETSOCKOPT`, `BPF_CGROUP_RUN_PROG_GETSOCKOPT` will receive an -EFAULT from `__cgroup_bpf_run_filter_getsockopt(max_optlen=0)` due to `get_user()` was not reached in `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN`. Scenario shown as below: `process A` `process B` ----------- ------------ BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN enable CGROUP_GETSOCKOPT BPF_CGROUP_RUN_PROG_GETSOCKOPT (-EFAULT) To prevent this, invoke `cgroup_bpf_enabled()` only once and cache the result in a newly added local variable `enabled`. Both `BPF_CGROUP_*` macros in `do_sock_getsockopt` will then check their condition using the same `enabled` variable as the condition variable, instead of using the return values from `cgroup_bpf_enabled` called by themselves as the condition variable(which could yield different results). This ensures that either both `BPF_CGROUP_*` macros pass the condition or neither does. Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks") Co-developed-by: Yanghui Li <redacted> Signed-off-by: Yanghui Li <redacted> Co-developed-by: Cheng-Jui Wang <redacted> Signed-off-by: Cheng-Jui Wang <redacted> Signed-off-by: Tze-nan Wu <redacted> --- Chagnes from v1 to v2: https://lore.kernel.org/all/20240819082513.27176-1-Tze-nan.Wu@mediatek.com/ (local) Instead of using cgroup_lock in the fastpath, invoke cgroup_bpf_enabled only once and cache the value in the newly added variable `enabled`. `BPF_CGROUP_*` macros in do_sock_getsockopt can then both check their condition with the new variable `enable`, ensuring that either they both passing the condition or both do not. Chagnes from v2 to v3: https://lore.kernel.org/all/20240819155627.1367-1-Tze-nan.Wu@mediatek.com/ (local) Hide cgroup_bpf_enabled in the macro, and some modifications to adapt the coding style. Chagnes from v3 to v4: https://lore.kernel.org/all/20240820092942.16654-1-Tze-nan.Wu@mediatek.com/ (local) Add bpf tag to subject, and Fixes tag in body. --- include/linux/bpf-cgroup.h | 15 ++++++++------- net/socket.c | 5 +++-- 2 files changed, 11 insertions(+), 9 deletions(-)diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index fb3c3e7181e6..5afa2ac76aae 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h@@ -390,20 +390,20 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk, __ret; \ }) -#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) \ +#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled) \ ({ \ int __ret = 0; \ - if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT)) \ + enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT); \ + if (enabled)
I suspect the compiler generates slow code after such a patch. pw-bot: cr What is the problem with double cgroup_bpf_enabled() check? yes it might return two different values, so?