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

Commit Message

Gang Yan Aug. 22, 2024, 6:08 a.m. UTC

From: Gang Yan <[e-mail]>

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 <[e-mail]>
Signed-off-by: Gang Yan <[e-mail]>
---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jakub Kicinski Aug. 22, 2024, 3:29 p.m. UTC | #1

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.

Alexei Starovoitov Aug. 22, 2024, 6:43 p.m. UTC | #2

On Thu, Aug 22, 2024 at 8:33 AM Jakub Kicinski <[e-mail]> 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

Patch

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