netdev - Re: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220125235320.fx775qsdtqon272v@kafai-mbp.dhcp.thefacebook.com>
Date:   Tue, 25 Jan 2022 15:53:20 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Jakub Sitnicki <jakub@...udflare.com>,
        Menglong Dong <menglong8.dong@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        Mengen Sun <mengensun@...cent.com>, <flyingpeng@...cent.com>,
        <mungerjiang@...cent.com>, Menglong Dong <imagedong@...cent.com>
Subject: Re: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct
 bpf_sock'

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@...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).

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?