bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
@ 2023-06-01  2:49 Feng zhou
  2023-06-01  5:37 ` Martin KaFai Lau
  0 siblings, 1 reply; 7+ messages in thread
From: Feng zhou @ 2023-06-01  2:49 UTC (permalink / raw)
  To: martin.lau, ast, daniel, andrii, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa
  Cc: bpf, linux-kernel, yangzhenze, wangdongdong.6, zhoufeng.zf

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) {
+		ret = -EFAULT;
+		goto out;
 	}
 
 	lock_sock(sk);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-01  2:49 [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval Feng zhou
@ 2023-06-01  5:37 ` Martin KaFai Lau
  2023-06-01  6:05   ` Feng Zhou
  0 siblings, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2023-06-01  5:37 UTC (permalink / raw)
  To: Feng zhou
  Cc: bpf, linux-kernel, yangzhenze, wangdongdong.6, ast, daniel,
	andrii, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa

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?

and there is no selftest also.

> +		ret = -EFAULT;
> +		goto out;
>   	}
>   
>   	lock_sock(sk);


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-01  5:37 ` Martin KaFai Lau
@ 2023-06-01  6:05   ` Feng Zhou
  2023-06-01 15:50     ` Martin KaFai Lau
  0 siblings, 1 reply; 7+ messages in thread
From: Feng Zhou @ 2023-06-01  6:05 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, linux-kernel, yangzhenze, wangdongdong.6, ast, daniel,
	andrii, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa

在 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.

> 
> 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);
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-01  6:05   ` Feng Zhou
@ 2023-06-01 15:50     ` Martin KaFai Lau
  2023-06-06  3:20       ` Feng Zhou
  0 siblings, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2023-06-01 15:50 UTC (permalink / raw)
  To: Feng Zhou
  Cc: bpf, linux-kernel, yangzhenze, wangdongdong.6, ast, daniel,
	andrii, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa

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);
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-01 15:50     ` Martin KaFai Lau
@ 2023-06-06  3:20       ` Feng Zhou
  2023-06-06 17:04         ` Stanislav Fomichev
  0 siblings, 1 reply; 7+ messages in thread
From: Feng Zhou @ 2023-06-06  3:20 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, linux-kernel, yangzhenze, wangdongdong.6, ast, daniel,
	andrii, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa

在 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)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-06  3:20       ` Feng Zhou
@ 2023-06-06 17:04         ` Stanislav Fomichev
  2023-06-08  3:39           ` Feng Zhou
  0 siblings, 1 reply; 7+ messages in thread
From: Stanislav Fomichev @ 2023-06-06 17:04 UTC (permalink / raw)
  To: Feng Zhou
  Cc: Martin KaFai Lau, bpf, linux-kernel, yangzhenze, wangdongdong.6,
	ast, daniel, andrii, song, yhs, john.fastabend, kpsingh, haoluo,
	jolsa

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: Re: [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval
  2023-06-06 17:04         ` Stanislav Fomichev
@ 2023-06-08  3:39           ` Feng Zhou
  0 siblings, 0 replies; 7+ messages in thread
From: Feng Zhou @ 2023-06-08  3:39 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Martin KaFai Lau, bpf, linux-kernel, yangzhenze, wangdongdong.6,
	ast, daniel, andrii, song, yhs, john.fastabend, kpsingh, haoluo,
	jolsa

在 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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-06-08  3:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-01  2:49 [PATCH bpf-next] bpf: getsockopt hook to get optval without checking kernel retval Feng zhou
2023-06-01  5:37 ` Martin KaFai Lau
2023-06-01  6:05   ` Feng Zhou
2023-06-01 15:50     ` Martin KaFai Lau
2023-06-06  3:20       ` Feng Zhou
2023-06-06 17:04         ` Stanislav Fomichev
2023-06-08  3:39           ` Feng Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).