[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <deb77161-3091-a134-4b82-78fef06efe85@linux.dev>
Date: Tue, 6 Dec 2022 10:36:20 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Ji Rongfeng <SikoJobs@...look.com>
Cc: daniel@...earbox.net, john.fastabend@...il.com, ast@...nel.org,
andrii@...nel.org, song@...nel.org, yhs@...com, kpsingh@...nel.org,
sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] bpf: Upgrade bpf_{g,s}etsockopt return values
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().
>
> * 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.
>
> * 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.
Powered by blists - more mailing lists