Re: Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

Re: Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Gang Yan]

Hi Alexei:
It's my honor to recieve your reply. The response to your concerns is attached below 
for your review.
On Mon, Aug 26, 2024 at 10:57:12AM +0800, Gang Yan wrote:
> On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski wrote:
> >
> > On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
> > > diff --git a/net/socket.c b/net/socket.c
> > > index fcbdd5bc47ac..63ce1caf75eb 100644
> > > --- a/net/socket.c
> > > +++ b/net/socket.c
> > > @@ -1695,6 +1695,7 @@ __weak noinline int update_socket_protocol(int family, int type, int protocol)
> > > {
> > > return protocol;
> > > }
> > > +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
> >
> > IDK if this falls under BPF or directly net, but could you explain
> > what test will use this? I'd prefer not to add test hooks into the
> > kernel unless they are exercised by in-tree tests.

> This looks unnecessary.
> update_socket_protocol is already registered as fmodret.
> There is even selftest that excises this feature:
> tools/testing/selftests/bpf/progs/mptcpify.c
> 
> It doesn't need to be part of the error-inject.

The 'update_socket_protocol' is a BPF interface designed primarily to
fix the socket protocol from TCP protocol to MPTCP protocol without
requiring modifications to user-space application codes. However,
when attempting to achieve this using the BCC tool in user-space,
the BCC tool doesn't support 'fmod_ret'. Therefore, this patch aims to 
further expand capabilities, enabling the 'kprobe' method can overriding 
the update_socket_protocol interface.

As a Python developer, the BCC tool is a commonly utilized instrument for 
interacting with the kernel. If the kernel could permit the use of an 
error-inject method to modify the `update_socket_protocol`, it would significantly 
benefit the subsequent promotion and development of MPTCP applications. 
Thank you for considering this enhancement.

Best wishes!
Gang Yan

Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Yonghong Song]

On 8/25/24 8:29 PM, Gang Yan wrote:
> Hi Alexei:
> It's my honor to recieve your reply. The response to your concerns is attached below
> for your review.
> On Mon, Aug 26, 2024 at 10:57:12AM +0800, Gang Yan wrote:
>> On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski wrote:
>>> On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index fcbdd5bc47ac..63ce1caf75eb 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -1695,6 +1695,7 @@ __weak noinline int update_socket_protocol(int family, int type, int protocol)
>>>> {
>>>> return protocol;
>>>> }
>>>> +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
>>> IDK if this falls under BPF or directly net, but could you explain
>>> what test will use this? I'd prefer not to add test hooks into the
>>> kernel unless they are exercised by in-tree tests.
>> This looks unnecessary.
>> update_socket_protocol is already registered as fmodret.
>> There is even selftest that excises this feature:
>> tools/testing/selftests/bpf/progs/mptcpify.c
>>
>> It doesn't need to be part of the error-inject.
> The 'update_socket_protocol' is a BPF interface designed primarily to
> fix the socket protocol from TCP protocol to MPTCP protocol without
> requiring modifications to user-space application codes. However,
> when attempting to achieve this using the BCC tool in user-space,
> the BCC tool doesn't support 'fmod_ret'. Therefore, this patch aims to
> further expand capabilities, enabling the 'kprobe' method can overriding
> the update_socket_protocol interface.

Gang Yan, could you explore to add fmod_ret support in bcc? It should be
similar to kfunc/kretfunc support. I am happy to review your patches.

Thanks,
Yonghong

>
> As a Python developer, the BCC tool is a commonly utilized instrument for
> interacting with the kernel. If the kernel could permit the use of an
> error-inject method to modify the `update_socket_protocol`, it would significantly
> benefit the subsequent promotion and development of MPTCP applications.
> Thank you for considering this enhancement.
>
> Best wishes!
> Gang Yan
>
>

Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Geliang Tang]

Hi Yonghong,

On Sun, 2024-08-25 at 21:05 -0700, Yonghong Song wrote:
> 
> On 8/25/24 8:29 PM, Gang Yan wrote:
> > Hi Alexei:
> > It's my honor to recieve your reply. The response to your concerns
> > is attached below
> > for your review.
> > On Mon, Aug 26, 2024 at 10:57:12AM +0800, Gang Yan wrote:
> > > On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski wrote:
> > > > On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
> > > > > diff --git a/net/socket.c b/net/socket.c
> > > > > index fcbdd5bc47ac..63ce1caf75eb 100644
> > > > > --- a/net/socket.c
> > > > > +++ b/net/socket.c
> > > > > @@ -1695,6 +1695,7 @@ __weak noinline int
> > > > > update_socket_protocol(int family, int type, int protocol)
> > > > > {
> > > > > return protocol;
> > > > > }
> > > > > +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
> > > > IDK if this falls under BPF or directly net, but could you
> > > > explain
> > > > what test will use this? I'd prefer not to add test hooks into
> > > > the
> > > > kernel unless they are exercised by in-tree tests.
> > > This looks unnecessary.
> > > update_socket_protocol is already registered as fmodret.
> > > There is even selftest that excises this feature:
> > > tools/testing/selftests/bpf/progs/mptcpify.c
> > > 
> > > It doesn't need to be part of the error-inject.
> > The 'update_socket_protocol' is a BPF interface designed primarily
> > to
> > fix the socket protocol from TCP protocol to MPTCP protocol without
> > requiring modifications to user-space application codes. However,
> > when attempting to achieve this using the BCC tool in user-space,
> > the BCC tool doesn't support 'fmod_ret'. Therefore, this patch aims
> > to
> > further expand capabilities, enabling the 'kprobe' method can
> > overriding
> > the update_socket_protocol interface.
> 
> Gang Yan, could you explore to add fmod_ret support in bcc? It should
> be
> similar to kfunc/kretfunc support. I am happy to review your patches.

It took us some time to add this support in bcc, and we have recently
completed it in [1]. We would be grateful if you could help review
these patches.

Thanks,
-Geliang

[1]
https://github.com/iovisor/bcc/pull/5274

> 
> Thanks,
> Yonghong
> 
> > 
> > As a Python developer, the BCC tool is a commonly utilized
> > instrument for
> > interacting with the kernel. If the kernel could permit the use of
> > an
> > error-inject method to modify the `update_socket_protocol`, it
> > would significantly
> > benefit the subsequent promotion and development of MPTCP
> > applications.
> > Thank you for considering this enhancement.
> > 
> > Best wishes!
> > Gang Yan
> > 
> > 
> 

Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Yonghong Song]

On 4/9/25 2:17 AM, Geliang Tang wrote:
> Hi Yonghong,
>
> On Sun, 2024-08-25 at 21:05 -0700, Yonghong Song wrote:
>> On 8/25/24 8:29 PM, Gang Yan wrote:
>>> Hi Alexei:
>>> It's my honor to recieve your reply. The response to your concerns
>>> is attached below
>>> for your review.
>>> On Mon, Aug 26, 2024 at 10:57:12AM +0800, Gang Yan wrote:
>>>> On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski wrote:
>>>>> On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
>>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>>> index fcbdd5bc47ac..63ce1caf75eb 100644
>>>>>> --- a/net/socket.c
>>>>>> +++ b/net/socket.c
>>>>>> @@ -1695,6 +1695,7 @@ __weak noinline int
>>>>>> update_socket_protocol(int family, int type, int protocol)
>>>>>> {
>>>>>> return protocol;
>>>>>> }
>>>>>> +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
>>>>> IDK if this falls under BPF or directly net, but could you
>>>>> explain
>>>>> what test will use this? I'd prefer not to add test hooks into
>>>>> the
>>>>> kernel unless they are exercised by in-tree tests.
>>>> This looks unnecessary.
>>>> update_socket_protocol is already registered as fmodret.
>>>> There is even selftest that excises this feature:
>>>> tools/testing/selftests/bpf/progs/mptcpify.c
>>>>
>>>> It doesn't need to be part of the error-inject.
>>> The 'update_socket_protocol' is a BPF interface designed primarily
>>> to
>>> fix the socket protocol from TCP protocol to MPTCP protocol without
>>> requiring modifications to user-space application codes. However,
>>> when attempting to achieve this using the BCC tool in user-space,
>>> the BCC tool doesn't support 'fmod_ret'. Therefore, this patch aims
>>> to
>>> further expand capabilities, enabling the 'kprobe' method can
>>> overriding
>>> the update_socket_protocol interface.
>> Gang Yan, could you explore to add fmod_ret support in bcc? It should
>> be
>> similar to kfunc/kretfunc support. I am happy to review your patches.
> It took us some time to add this support in bcc, and we have recently
> completed it in [1]. We would be grateful if you could help review
> these patches.

Sounds good. I will take a look.

>
> Thanks,
> -Geliang
>
> [1]
> https://github.com/iovisor/bcc/pull/5274
>
>> Thanks,
>> Yonghong
>>
>>> As a Python developer, the BCC tool is a commonly utilized
>>> instrument for
>>> interacting with the kernel. If the kernel could permit the use of
>>> an
>>> error-inject method to modify the `update_socket_protocol`, it
>>> would significantly
>>> benefit the subsequent promotion and development of MPTCP
>>> applications.
>>> Thank you for considering this enhancement.
>>>
>>> Best wishes!
>>> Gang Yan
>>>
>>>
>

[PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Gang Yan]

From: Gang Yan <yangang@kylinos.cn>

The "update_socket_protocol" interface is designed to empower user space
with the capability to customize and modify socket protocols leveraging
BPF methods. Currently, it has only granted the fmod_ret permission,
allowing for modifications to return values. We are extending the
permissions further by 'ALLOW_ERROR_INJECTION', thereby facilitating
the development of user-space programs with enhanced flexibility and
convenience.

When we attempt to modify the return value of "update_socket_protocol"
to "IPPROTO_MPTCP" using the below code based on the BCC tool:

'''
int kprobe__update_socket_protocol(void* ctx)
{
    ...
    bpf_override_return(ctx, IPPROTO_MPTCP);
    ...
}
'''

But an error occurs:

'''
ioctl(PERF_EVENT_IOC_SET_BPF): Invalid argument
Traceback (most recent call last):
  File "/media/yangang/work/Code/BCC/test.py", line 27, in <module>
    b = BPF(text=prog)
        ^^^^^^^^^^^^^^
  File "/usr/lib/python3/dist-packages/bcc/__init__.py", line 487, \
                  in __init__ self._trace_autoload()
  File "/usr/lib/python3/dist-packages/bcc/__init__.py", line 1466, \
                  in _trace_autoload self.attach_kprobe(
  File "/usr/lib/python3/dist-packages/bcc/__init__.py", line 855,\
                  in attach_kprobe
    raise Exception("Failed to attach BPF program %s to kprobe %s"
Exception: Failed to attach BPF program b'kprobe__update_socket_protocol' \
  to kprobe b'update_socket_protocol', it's not traceable \
  (either non-existing, inlined, or marked as "notrace")
'''

This patch can fix the issue.

Suggested-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..63ce1caf75eb 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1695,6 +1695,7 @@ __weak noinline int update_socket_protocol(int family, int type, int protocol)
 {
 	return protocol;
 }
+ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
 
 __bpf_hook_end();
 
-- 
2.43.0

Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Jakub Kicinski]

On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
> diff --git a/net/socket.c b/net/socket.c
> index fcbdd5bc47ac..63ce1caf75eb 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1695,6 +1695,7 @@ __weak noinline int update_socket_protocol(int family, int type, int protocol)
>  {
>  	return protocol;
>  }
> +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);

IDK if this falls under BPF or directly net, but could you explain
what test will use this? I'd prefer not to add test hooks into the
kernel unless they are exercised by in-tree tests.

Re: [PATCH bpf-next] bpf: Allow error injection for update_socket_protocol

[Alexei Starovoitov]

On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 22 Aug 2024 14:08:57 +0800 Gang Yan wrote:
> > diff --git a/net/socket.c b/net/socket.c
> > index fcbdd5bc47ac..63ce1caf75eb 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1695,6 +1695,7 @@ __weak noinline int update_socket_protocol(int family, int type, int protocol)
> >  {
> >       return protocol;
> >  }
> > +ALLOW_ERROR_INJECTION(update_socket_protocol, ERRNO);
>
> IDK if this falls under BPF or directly net, but could you explain
> what test will use this? I'd prefer not to add test hooks into the
> kernel unless they are exercised by in-tree tests.

This looks unnecessary.
update_socket_protocol is already registered as fmodret.
There is even selftest that excises this feature:
tools/testing/selftests/bpf/progs/mptcpify.c

It doesn't need to be part of the error-inject.

pw-bot: cr