Messages in this thread | | | Date | Wed, 7 Dec 2022 23:28:49 +0800 | Subject | Re: [PATCH bpf-next v2] bpf: Upgrade bpf_{g,s}etsockopt return values | From | Ji Rongfeng <> |
| |
On 2022/12/7 19:19, Ji Rongfeng wrote: > On 2022/12/7 2:36, Martin KaFai Lau wrote: >> On 12/2/22 9:39 AM, Ji Rongfeng wrote: >>> Returning -EINVAL almost all the time when error occurs is not very >>> helpful for the bpf prog to figure out what is wrong. This patch >>> upgrades some return values so that they will be much more helpful. >>> >>> * return -ENOPROTOOPT when optname is unsupported >>> >>> The same as {g,s}etsockopt() syscall does. Before this patch, >>> bpf_setsockopt(TCP_SAVED_SYN) already returns -ENOPROTOOPT, which >>> may confuse the user, as -EINVAL is returned on other unsupported >>> optnames. This patch also rejects TCP_SAVED_SYN right in >>> sol_tcp_sockopt() when getopt is false, since do_tcp_setsockopt() >>> is just the executor and it's not its duty to discover such error >>> in bpf. We should maintain a precise allowlist to control whether >>> an optname is supported and allowed to enter the executor or not. >>> Functions like do_tcp_setsockopt(), their behaviour are not fully >>> controllable by bpf. Imagine we let an optname pass, expecting >>> -ENOPROTOOPT will be returned, but someday that optname is >>> actually processed and unfortunately causes deadlock when calling >>> from bpf. Thus, precise access control is essential. >> >> Please leave the current -EINVAL to distinguish between optnames >> rejected by bpf and optnames rejected by the do_*_{get,set}sockopt(). > > To reach that goal, it would be better for us to pick a value other than > -ENOPROTOOPT or -EINVAL. This patch actually makes sk-related errors, > level-reletad errors, optname-related errors and opt{val,len}-related > errors distinguishable, as they should be, by leaving -EINVAL to > opt{val,len}-related errors only. man setsockopt: > > > EINVAL optlen invalid in setsockopt(). In some cases this error > > can also occur for an invalid value in optval (e.g., for > > the IP_ADD_MEMBERSHIP option described in ip(7)). > > With an unique return value, the bpf prog developer will be able to know > that the error is "unsupported or unknown optname" for sure, saving time > on figuring the actual cause of the error. In production environment, > the bpf prog will be able to test whether an optname is available in > current bpf env and decide what to do next also, which is very useful. > >> >>> >>> * return -EOPNOTSUPP on level-related errors >>> >>> In do_ip_getsockopt(), -EOPNOTSUPP will be returned if level != >>> SOL_IP. In ipv6_getsockopt(), -ENOPROTOOPT will be returned if >>> level != SOL_IPV6. To be distinguishable, the former is chosen. >> >> I would leave this one as is also. Are you sure the do_ip_*sockopt >> cannot handle sk_family == AF_INET6? afaict, bpf is rejecting those >> optnames instead. > > -EOPNOTSUPP is just picked here as an unique return value representing > "unknown level or unsupported sk_family or mismatched protocol in > bpf_{g,s}etsockopt()". I'm ok if you want to pick another unique value > for them or pick three unique values for each type of error : )
Sorry, I meant "three unique values for three types of error", which is growing more and more sensible in my mind as I'm thinking about it.
> >> >>> >>> * return -EBADFD when sk is not a full socket >>> >>> -EPERM or -EBUSY was an option, but in many cases one of them >>> will be returned, especially under level SOL_TCP. -EBADFD is the >>> better choice, since it is hardly returned in all cases. The bpf >>> prog will be able to recognize it and decide what to do next. >> >> This one makes sense and is useful. >> >
| |