Re: [PATCH bpf] bpf: Return -EINVAL on calling bpf_setsockopt(TCP_SAVED_SYN)

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/25/22 4:22 AM, Rongfeng Ji wrote:
> TCP_SAVED_SYN is not supported by do_tcp_setsockopt(), but it is not
> rejected by sol_tcp_sockopt() during calling bpf_setsockopt(), which
> results in returning -ENOPROTOOPT instead of common -EINVAL.
> 
> This patch fixes the issue.
> 
> Signed-off-by: Rongfeng Ji <SikoJobs@outlook.com>
> ---
>   net/core/filter.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index bb0136e7a8e4..42cd7ec8cc4c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5206,6 +5206,9 @@ static int sol_tcp_sockopt(struct sock *sk, int optname,
>   		return do_tcp_getsockopt(sk, SOL_TCP, optname,
>   					 KERNEL_SOCKPTR(optval),
>   					 KERNEL_SOCKPTR(optlen));
> +	} else {
> +		if (optname == TCP_SAVED_SYN)
> +			return -EINVAL;

ENOPROTOOPT is fine and is better imo.  man 7 setsockopt:

        ENOPROTOOPT
                  The option is unknown at the level indicated.

It is why I did not single out the TCP_SAVED_SYN again to return -EINVAL.
I don't see how the bpf_prog would handle them differently (bpf prog does not 
allow it -EINVAL or the underlying kernel's setsockopt does not know it 
-ENOPROTOOPT).  In general, the bpf_{get,set}sockopt caller has to be ready to 
handle any errno from the kernel underlying {get,set}sockopt.

Also, some of the -EINVAL in bpf_{get,set}sockopt() is not the best one to 
return.  It is not very helpful for the bpf prog to figure out what is wrong. 
They should be fixed in the future also.