Re: Typo in the man7 bpf-helpers page

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Quentin,
After reading https://github.com/torvalds/linux/commit/d692f1138a4b,
now I figure out that only BPF_FUNC_get_socket_cookie is exposed.
The same name (function id BPF_FUNC_get_socket_cookie) will be
resolved to a different function (bpf_get_socket_cookie_sock_ops and
the others) according to the scope.

Thanks for your explanation!

Zexuan Luo <spacewanderlzx@xxxxxxxxx> 于2023年1月31日周二 22:36写道:
>
> My bad!
>
> > No, I don't think there is anything wrong with that. I suppose you mean
> bpf_get_socket_cookie_sock_(ad
> dr|ops) (the functions you mentioned don't
> exist), but the four variants of the helper just have the same name, and
> take different objects for their context.
>
> Yes! I made a mistake in the function names in the first email. Thank
> you for pointing that out.
>
> I am an eBPF newbie and I am learning it currently. AFAIK, language C
> doesn't support function overriding via different parameters.
> So how do these four functions co-exist?
>
> Some naive search in the kernel code leads me to:
> https://elixir.bootlin.com/linux/v6.2-rc6/source/net/core/filter.c#L4919
> ```
> static const struct bpf_func_proto bpf_get_socket_cookie_sock_addr_proto = {
>     .func        = bpf_get_socket_cookie_sock_addr,
>     .gpl_only    = false,
>     .ret_type    = RET_INTEGER,
>     .arg1_type    = ARG_PTR_TO_CTX,
> };
> ```
>
> https://elixir.bootlin.com/linux/v6.2-rc6/source/net/core/filter.c#L4955
> ```
> static const struct bpf_func_proto bpf_get_socket_cookie_sock_ops_proto = {
>         .func           = bpf_get_socket_cookie_sock_ops,
>         .gpl_only       = false,
>         .ret_type       = RET_INTEGER,
>         .arg1_type      = ARG_PTR_TO_CTX,
> };
> ```
>
> It seems that the function definitions are quite real...
>
> Quentin Monnet <quentin@xxxxxxxxxxxxx> 于2023年1月31日周二 20:02写道:
>
> >
> > 2023-01-31 12:40 UTC+0100 ~ Alejandro Colomar <alx.manpages@xxxxxxxxx>
> > > [Resend with Quentin's right address, I hope]
> > >
> > > Hi Zexuan, Quentin,
> > >
> > > On 1/31/23 11:03, Zexuan Luo wrote:
> > >> Hello Colomar,
> > >>
> > >> I just found a potential bug in the bpf-helpers page.
> > >
> > > Thanks for reporting bugs :)
> > >
> > >>
> > >> Under the https://www.man7.org/linux/man-pages/man7/bpf-helpers.7.html:
> > >
> > > This page is generated from the Linux kernel sources.  I've CCed Quentin
> > > and the BPF list so they can check it there.
> > >
> >
> > Hi Alejandro, Zexuan,
> > Thanks for the report! Happy to take fixes, however, see below...
> >
> > > BTW, I'm refreshing the page now.
> > >
> >
> > Great, thank you!
> >
> > > Quentin, I realized in the diff that there is some inconsistency in the
> > > number of spaces after a sentence-ending period.  Could you please use
> > > two spaces for that?  It's especially important for groff(1), which will
> > > render it differently.   However, it's not a big issue, so don't feel
> > > urged to do that.
> >
> > Yes, you mentioned that in the past and this is on my list. As you can
> > see, I haven't felt urged so far indeed :). But it's still on my mind
> > for the next time I take a look at this doc for typos etc.
> >
> > >
> > > Cheers,
> > >
> > > Alex
> > >
> > >>
> > >> ```
> > >>         u64 bpf_get_socket_cookie(struct sk_buff *skb)
> > >>
> > >>                Description
> > >>                       If the struct sk_buff pointed by skb has a known
> > >>                       socket, retrieve the cookie (generated by the
> > >>                       kernel) of this socket.  If no cookie has been set
> > >>                       yet, generate a new cookie. Once generated, the
> > >>                       socket cookie remains stable for the life of the
> > >>                       socket. This helper can be useful for monitoring
> > >>                       per socket networking traffic statistics as it
> > >>                       provides a global socket identifier that can be
> > >>                       assumed unique.
> > >>
> > >>                Return A 8-byte long non-decreasing number on success, or
> > >>                       0 if the socket field is missing inside skb.
> > >>
> > >>         u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
> > >>
> > >>                Description
> > >>                       Equivalent to bpf_get_socket_cookie() helper that
> > >>                       accepts skb, but gets socket from struct
> > >>                       bpf_sock_addr context.
> > >>
> > >>                Return A 8-byte long non-decreasing number.
> > >>
> > >>         u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
> > >>
> > >>                Description
> > >>                       Equivalent to bpf_get_socket_cookie() helper that
> > >>                       accepts skb, but gets socket from struct
> > >>                       bpf_sock_ops context.
> > >>
> > >>                Return A 8-byte long non-decreasing number.
> > >> ```
> > >>
> > >> The function bpf_get_socket_cookie repeats three times. The second one
> > >> should be bpf_get_socket_cookie_addr and the third one should be
> > >> bpf_get_socket_cookie_ops.
> > >
> >
> > No, I don't think there is anything wrong with that. I suppose you mean
> > bpf_get_socket_cookie_sock_(addr|ops) (the functions you mentioned don't
> > exist), but the four variants of the helper just have the same name, and
> > take different objects for their context. There is no risk of collision
> > because they are each associated to distinct eBPF program types.
> >
> > Please see also commit d692f1138a4b ("bpf: Support bpf_get_socket_cookie
> > in more prog types"): "It doesn't introduce new helpers. Instead it
> > reuses same helper name bpf_get_socket_cookie() but adds support to this
> > helper to accept `struct bpf_sock_addr` and `struct bpf_sock_ops`.".
> >
> > Thanks,
> > Quentin




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux