lkml.org 
[lkml]   [2022]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH bpf-next v2] bpf: Upgrade bpf_{g,s}etsockopt return values
From
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.
>>
>

\
 
 \ /
  Last update: 2022-12-07 16:30    [W:0.078 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site