lkml.org 
[lkml]   [2022]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct bpf_sock'
    Date
    On Wed, Jan 26, 2022 at 12:53 AM CET, Martin KaFai Lau wrote:
    > On Tue, Jan 25, 2022 at 03:02:37PM -0800, Alexei Starovoitov wrote:
    >> On Tue, Jan 25, 2022 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
    >> >
    >> > On Tue, Jan 25, 2022 at 08:24:27PM +0100, Jakub Sitnicki wrote:
    >> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
    >> > > > index b0383d371b9a..891a182a749a 100644
    >> > > > --- a/include/uapi/linux/bpf.h
    >> > > > +++ b/include/uapi/linux/bpf.h
    >> > > > @@ -5500,7 +5500,11 @@ struct bpf_sock {
    >> > > > __u32 src_ip4;
    >> > > > __u32 src_ip6[4];
    >> > > > __u32 src_port; /* host byte order */
    >> > > > - __u32 dst_port; /* network byte order */
    >> > > > + __u32 dst_port; /* low 16-bits are in network byte order,
    >> > > > + * and high 16-bits are filled by 0.
    >> > > > + * So the real port in host byte order is
    >> > > > + * bpf_ntohs((__u16)dst_port).
    >> > > > + */
    >> > > > __u32 dst_ip4;
    >> > > > __u32 dst_ip6[4];
    >> > > > __u32 state;
    >> > >
    >> > > I'm probably missing something obvious, but is there anything stopping
    >> > > us from splitting the field, so that dst_ports is 16-bit wide?
    >> > >
    >> > > I gave a quick check to the change below and it seems to pass verifier
    >> > > checks and sock_field tests.
    >> > >
    >> > > IDK, just an idea. Didn't give it a deeper thought.
    >> > >
    >> > > --8<--
    >> > >
    >> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
    >> > > index 4a2f7041ebae..344d62ccafba 100644
    >> > > --- a/include/uapi/linux/bpf.h
    >> > > +++ b/include/uapi/linux/bpf.h
    >> > > @@ -5574,7 +5574,8 @@ struct bpf_sock {
    >> > > __u32 src_ip4;
    >> > > __u32 src_ip6[4];
    >> > > __u32 src_port; /* host byte order */
    >> > > - __u32 dst_port; /* network byte order */
    >> > > + __u16 unused;
    >> > > + __u16 dst_port; /* network byte order */
    >> > This will break the existing bpf prog.
    >>
    >> I think Jakub's idea is partially expressed:
    >> + case offsetof(struct bpf_sock, dst_port):
    >> + bpf_ctx_record_field_size(info, sizeof(__u16));
    >> + return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
    >>
    >> Either 'unused' needs to be after dst_port or
    >> bpf_sock_is_valid_access() needs to allow offset at 'unused'
    >> and at 'dst_port'.
    >> And allow u32 access though the size is actually u16.
    >> Then the existing bpf progs (without recompiling) should work?
    > Yes, I think that should work with the existing bpf progs.
    > I suspect putting 'dst_port' first and then followed by 'unused'
    > may be easier. That will also serve as a natural doc for the
    > current behavior (the value is in the lower 16 bits).

    You're right. I can't count. Now fixed in [1].

    >
    > It can be extended to bpf_sk_lookup? bpf_sk_lookup can read at any
    > offset of these 4 bytes, so may need to read 0 during
    > convert_ctx_accesses?

    Let's see what the feedback to [1] will be.

    [1] https://lore.kernel.org/bpf/20220127172448.155686-1-jakub@cloudflare.com/T/#t

    \
     
     \ /
      Last update: 2022-01-27 18:33    [W:4.500 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site