[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d7be9d22-c6aa-da2a-77fc-9638ad1e0f15@linux.dev>
Date: Thu, 1 Jun 2023 08:50:19 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Feng Zhou <zhoufeng.zf@...edance.com>
Cc: bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
yangzhenze@...edance.com, wangdongdong.6@...edance.com,
ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
song@...nel.org, yhs@...com, john.fastabend@...il.com,
kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
jolsa@...nel.org
Subject: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without
checking kernel retval
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@...edance.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@...edance.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);
>>
>
Powered by blists - more mailing lists