Messages in this thread | | | Date | Thu, 1 Jun 2023 08:50:19 -0700 | Subject | Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval | From | Martin KaFai Lau <> |
| |
On 5/31/23 11:05 PM, Feng Zhou wrote: > 在 2023/6/1 13:37, Martin KaFai Lau 写道: >> On 5/31/23 7:49 PM, Feng zhou wrote: >>> From: Feng Zhou <zhoufeng.zf@bytedance.com> >>> >>> Remove the judgment on retval and pass bpf ctx by default. The >>> advantage of this is that it is more flexible. Bpf getsockopt can >>> support the new optname without using the module to call the >>> nf_register_sockopt to register. >>> >>> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com> >>> --- >>> kernel/bpf/cgroup.c | 35 +++++++++++++---------------------- >>> 1 file changed, 13 insertions(+), 22 deletions(-) >>> >>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >>> index 5b2741aa0d9b..ebad5442d8bb 100644 >>> --- a/kernel/bpf/cgroup.c >>> +++ b/kernel/bpf/cgroup.c >>> @@ -1896,30 +1896,21 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock >>> *sk, int level, >>> if (max_optlen < 0) >>> return max_optlen; >>> - if (!retval) { >>> - /* If kernel getsockopt finished successfully, >>> - * copy whatever was returned to the user back >>> - * into our temporary buffer. Set optlen to the >>> - * one that kernel returned as well to let >>> - * BPF programs inspect the value. >>> - */ >>> - >>> - if (get_user(ctx.optlen, optlen)) { >>> - ret = -EFAULT; >>> - goto out; >>> - } >>> + if (get_user(ctx.optlen, optlen)) { >>> + ret = -EFAULT; >>> + goto out; >>> + } >>> - if (ctx.optlen < 0) { >>> - ret = -EFAULT; >>> - goto out; >>> - } >>> - orig_optlen = ctx.optlen; >>> + if (ctx.optlen < 0) { >>> + ret = -EFAULT; >>> + goto out; >>> + } >>> + orig_optlen = ctx.optlen; >>> - if (copy_from_user(ctx.optval, optval, >>> - min(ctx.optlen, max_optlen)) != 0) { >>> - ret = -EFAULT; >>> - goto out; >>> - } >>> + if (copy_from_user(ctx.optval, optval, >>> + min(ctx.optlen, max_optlen)) != 0) { >> What is in optval that is useful to copy from if the kernel didn't handle the >> optname? > > For example, if the user customizes a new optname, it will not be processed if > the kernel does not support it. Then the data stored in optval is the data put
> by the user. If this part can be seen by bpf prog, the user can implement > processing logic of the custom optname through bpf prog.
This part does not make sense. It is a (get)sockopt. Why the bpf prog should expect anything useful in the original __user optval? Other than unnecessary copy for other common cases, it looks like a bad api, so consider it a NAK.
> >> >> and there is no selftest also. >> > > Yes, if remove this restriction, everyone thinks it's ok, I'll add it in the > next version. > >>> + ret = -EFAULT; >>> + goto out; >>> } >>> lock_sock(sk); >> >
| |