Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
From: Martin KaFai Lau
Date: Thu Jun 01 2023 - 11:51:53 EST
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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);