Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <deb77161-3091-a134-4b82-78fef06efe85@linux.dev>
Date:   Tue, 6 Dec 2022 10:36:20 -0800
MIME-Version: 1.0
Subject: Re: [PATCH bpf-next v2] bpf: Upgrade bpf_{g,s}etsockopt return values
Content-Language: en-US
To:     Ji Rongfeng <SikoJobs@outlook.com>
Cc:     daniel@iogearbox.net, john.fastabend@gmail.com, ast@kernel.org,
        andrii@kernel.org, song@kernel.org, yhs@fb.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, bpf@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <DU0P192MB1547FE6F35CC1A3EEA1AFDECD6179@DU0P192MB1547.EURP192.PROD.OUTLOOK.COM>
From:   Martin KaFai Lau <martin.lau@linux.dev>
In-Reply-To: <DU0P192MB1547FE6F35CC1A3EEA1AFDECD6179@DU0P192MB1547.EURP192.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.