Re: [PATCH bpf-next v2] bpf: Upgrade bpf_{g,s}etsockopt return values

From: Ji Rongfeng
Date: Wed Dec 07 2022 - 10:29:08 EST


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.