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: <c902419b-e265-75ff-1275-338dbfd69cda@bytedance.com>
Date:   Thu, 8 Jun 2023 11:39:21 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.0
Subject: Re: Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval
 without checking kernel retval
To:     Stanislav Fomichev <sdf@google.com>
Cc:     Martin KaFai Lau <martin.lau@linux.dev>, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, yangzhenze@bytedance.com,
        wangdongdong.6@bytedance.com, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, haoluo@google.com,
        jolsa@kernel.org
References: <20230601024900.22902-1-zhoufeng.zf@bytedance.com>
 <5bc1ac0d-cea8-19e5-785a-cd72140d8cdb@linux.dev>
 <20881602-9afc-96b7-3d58-51c31e3f50b7@bytedance.com>
 <d7be9d22-c6aa-da2a-77fc-9638ad1e0f15@linux.dev>
 <2d138e12-9273-46e6-c219-96c665f38f0f@bytedance.com>
 <CAKH8qBtxNuwvawZ3v1-eK0RovPHu5AtYpays29TjxE2s-2RHpQ@mail.gmail.com>
From:   Feng Zhou <zhoufeng.zf@bytedance.com>
In-Reply-To: <CAKH8qBtxNuwvawZ3v1-eK0RovPHu5AtYpays29TjxE2s-2RHpQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

在 2023/6/7 01:04, Stanislav Fomichev 写道:
> On Mon, Jun 5, 2023 at 8:20 PM Feng Zhou <zhoufeng.zf@bytedance.com> wrote:
>>
>> 在 2023/6/1 23:50, Martin KaFai Lau 写道:
>>> On 5/31/23 11:05 PM, Feng Zhou wrote:
>>>> 在 2023/6/1 13:37, Martin KaFai Lau 写道:
>>>>> On 5/31/23 7:49 PM, Feng zhou wrote:
>>>>>> From: Feng Zhou <zhoufeng.zf@bytedance.com>
>>>>>>
>>>>>> Remove the judgment on retval and pass bpf ctx by default. The
>>>>>> advantage of this is that it is more flexible. Bpf getsockopt can
>>>>>> support the new optname without using the module to call the
>>>>>> nf_register_sockopt to register.
>>>>>>
>>>>>> Signed-off-by: Feng Zhou <zhoufeng.zf@bytedance.com>
>>>>>> ---
>>>>>>    kernel/bpf/cgroup.c | 35 +++++++++++++----------------------
>>>>>>    1 file changed, 13 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>>>>> index 5b2741aa0d9b..ebad5442d8bb 100644
>>>>>> --- a/kernel/bpf/cgroup.c
>>>>>> +++ b/kernel/bpf/cgroup.c
>>>>>> @@ -1896,30 +1896,21 @@ int
>>>>>> __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
>>>>>>        if (max_optlen < 0)
>>>>>>            return max_optlen;
>>>>>> -    if (!retval) {
>>>>>> -        /* If kernel getsockopt finished successfully,
>>>>>> -         * copy whatever was returned to the user back
>>>>>> -         * into our temporary buffer. Set optlen to the
>>>>>> -         * one that kernel returned as well to let
>>>>>> -         * BPF programs inspect the value.
>>>>>> -         */
>>>>>> -
>>>>>> -        if (get_user(ctx.optlen, optlen)) {
>>>>>> -            ret = -EFAULT;
>>>>>> -            goto out;
>>>>>> -        }
>>>>>> +    if (get_user(ctx.optlen, optlen)) {
>>>>>> +        ret = -EFAULT;
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> -        if (ctx.optlen < 0) {
>>>>>> -            ret = -EFAULT;
>>>>>> -            goto out;
>>>>>> -        }
>>>>>> -        orig_optlen = ctx.optlen;
>>>>>> +    if (ctx.optlen < 0) {
>>>>>> +        ret = -EFAULT;
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +    orig_optlen = ctx.optlen;
>>>>>> -        if (copy_from_user(ctx.optval, optval,
>>>>>> -                   min(ctx.optlen, max_optlen)) != 0) {
>>>>>> -            ret = -EFAULT;
>>>>>> -            goto out;
>>>>>> -        }
>>>>>> +    if (copy_from_user(ctx.optval, optval,
>>>>>> +                min(ctx.optlen, max_optlen)) != 0) {
>>>>> What is in optval that is useful to copy from if the kernel didn't
>>>>> handle the optname?
>>>>
>>>> For example, if the user customizes a new optname, it will not be
>>>> processed if the kernel does not support it. Then the data stored in
>>>> optval is the data put
>>>
>>>
>>>
>>>> by the user. If this part can be seen by bpf prog, the user can
>>>> implement processing logic of the custom optname through bpf prog.
>>>
>>> This part does not make sense. It is a (get)sockopt. Why the bpf prog
>>> should expect anything useful in the original __user optval? Other than
>>> unnecessary copy for other common cases, it looks like a bad api, so
>>> consider it a NAK.
>>>
>>>>
>>>>>
>>>>> and there is no selftest also.
>>>>>
>>>>
>>>> Yes, if remove this restriction, everyone thinks it's ok, I'll add it
>>>> in the next version.
>>>>
>>>>>> +        ret = -EFAULT;
>>>>>> +        goto out;
>>>>>>        }
>>>>>>        lock_sock(sk);
>>>>>
>>>>
>>>
>>
>> According to my understanding, users will have such requirements,
>> customize an optname, which is not available in the kernel. All logic is
>> completed in bpf prog, and bpf prog needs to obtain the user data passed
>> in by the system call, and then return the data required by the user
>> according to this data.
>>
>> For optname not in the kernel, the error code is
>> #define ENOPROTOOPT 92/* Protocol not available */
>> Whether to consider the way of judging with error codes,
>> If (! retval | | retval == -ENOPROTOOPT)
> 
> I'm also failing to see what you're trying to do here. You can already
> implement custom optnames via getsockopt, so what's missing?
> If you need to pass some data from the userspace to the hook, then
> setsockopt hook will serve you better.
> getsockopt is about reading something from the kernel/bpf; ignoring
> initial user buffer value is somewhat implied here.

Thanks, you reminded me that can pass data to bpf prog by setsockopt.