mbox series

[RFC,bpf-next,0/5] Add bpf_link support for sk_msg prog

Message ID 20240305202155.3890667-1-yonghong.song@linux.dev (mailing list archive)
Headers show
Series Add bpf_link support for sk_msg prog | expand

Message

Yonghong Song March 5, 2024, 8:21 p.m. UTC
One of our internal services started to use sk_msg program and currently
it used existing prog attach/detach2 as demonstrated in selftests.
But attach/detach of all other bpf programs are based on bpf_link.
Consistent attach/detach APIs for all programs will make things easy to
undersand and less error prone. So this patch added bpf_link
support for BPF_PROG_TYPE_SK_MSG.

I marked the patch as RFC as not all functionality are covered
by tests yet, e.g. update_prog(). Maybe somebody can suggest
an existing test which I can look into.
Or maybe some other tests need to be added as well.

Yonghong Song (5):
  bpf: Add link support for sk_msg prog
  libbpf: Refactor bpf_program_attach_fd()
  libbpf: Add link support for BPF_PROG_TYPE_SK_MSG
  bpftool: Add link dump support for BPF_LINK_TYPE_SK_MSG
  selftests/bpf: Add some tests with new bpf_program__attach_sk_msg()
    API

 include/linux/bpf.h                           |  13 ++
 include/uapi/linux/bpf.h                      |   5 +
 kernel/bpf/syscall.c                          |   3 +
 net/core/skmsg.c                              | 153 ++++++++++++++++++
 net/core/sock_map.c                           |   6 +-
 tools/bpf/bpftool/link.c                      |   8 +
 tools/include/uapi/linux/bpf.h                |   5 +
 tools/lib/bpf/libbpf.c                        |  26 ++-
 tools/lib/bpf/libbpf.h                        |   3 +
 tools/lib/bpf/libbpf.map                      |   1 +
 .../selftests/bpf/prog_tests/sockmap_basic.c  |  27 ++++
 .../selftests/bpf/prog_tests/sockmap_listen.c |  38 +++++
 12 files changed, 279 insertions(+), 9 deletions(-)

Comments

John Fastabend March 6, 2024, 7:19 p.m. UTC | #1
Yonghong Song wrote:
> One of our internal services started to use sk_msg program and currently
> it used existing prog attach/detach2 as demonstrated in selftests.
> But attach/detach of all other bpf programs are based on bpf_link.
> Consistent attach/detach APIs for all programs will make things easy to
> undersand and less error prone. So this patch added bpf_link
> support for BPF_PROG_TYPE_SK_MSG.
> 
> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

Agree this was missing it will be nice to get this merged. The links
are much nicer to deal with.
Jakub Sitnicki March 7, 2024, 1:01 p.m. UTC | #2
On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:
> One of our internal services started to use sk_msg program and currently
> it used existing prog attach/detach2 as demonstrated in selftests.
> But attach/detach of all other bpf programs are based on bpf_link.
> Consistent attach/detach APIs for all programs will make things easy to
> undersand and less error prone. So this patch added bpf_link
> support for BPF_PROG_TYPE_SK_MSG.
>
> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

I have a general remark, not specific to this work.

We can't attach with links from CLI, apart from when auto-attach is
supported.  `bpftool prog attach` doesn't use bpf links. For instance:

bpftool prog attach \
        pinned /sys/fs/bpf/test/sk_msg_prog \
        sk_msg_verdict \
        pinned /sys/fs/bpf/test/sock_map

Is there a plan for the CLI tooling to support it?
Yonghong Song March 7, 2024, 10:47 p.m. UTC | #3
On 3/6/24 11:19 AM, John Fastabend wrote:
> Yonghong Song wrote:
>> One of our internal services started to use sk_msg program and currently
>> it used existing prog attach/detach2 as demonstrated in selftests.
>> But attach/detach of all other bpf programs are based on bpf_link.
>> Consistent attach/detach APIs for all programs will make things easy to
>> undersand and less error prone. So this patch added bpf_link
>> support for BPF_PROG_TYPE_SK_MSG.
>>
>> I marked the patch as RFC as not all functionality are covered
>> by tests yet, e.g. update_prog(). Maybe somebody can suggest
>> an existing test which I can look into.
>> Or maybe some other tests need to be added as well.
> Agree this was missing it will be nice to get this merged. The links
> are much nicer to deal with.

Agree that bpf_link is better as proved by other cases. It would be great
if you can take a look at the patch set and let me any issues, missing cases,
missing tests, etc. Thanks!
Jakub Sitnicki March 10, 2024, 7:52 p.m. UTC | #4
On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:

[...]

> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

Looks like we don't have prog update covered today.
That would be something to add to sockmap_basic suite.
Yonghong Song March 11, 2024, 9:53 p.m. UTC | #5
On 3/7/24 5:01 AM, Jakub Sitnicki wrote:
> On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:
>> One of our internal services started to use sk_msg program and currently
>> it used existing prog attach/detach2 as demonstrated in selftests.
>> But attach/detach of all other bpf programs are based on bpf_link.
>> Consistent attach/detach APIs for all programs will make things easy to
>> undersand and less error prone. So this patch added bpf_link
>> support for BPF_PROG_TYPE_SK_MSG.
>>
>> I marked the patch as RFC as not all functionality are covered
>> by tests yet, e.g. update_prog(). Maybe somebody can suggest
>> an existing test which I can look into.
>> Or maybe some other tests need to be added as well.
> I have a general remark, not specific to this work.
>
> We can't attach with links from CLI, apart from when auto-attach is
> supported.  `bpftool prog attach` doesn't use bpf links. For instance:
>
> bpftool prog attach \
>          pinned /sys/fs/bpf/test/sk_msg_prog \
>          sk_msg_verdict \
>          pinned /sys/fs/bpf/test/sock_map
>
> Is there a plan for the CLI tooling to support it?

As far as I know, there is no plan to support this. Of course, somebody
could try to implement this ...