netdev - Re: [RFC bpf-next PATCH] bpf: add comments to BPF ld/ldx sizes

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] [day] [month] [year] [list]

Message-ID: <20180117114102.5e4d142a@redhat.com>
Date:   Wed, 17 Jan 2018 11:41:02 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        netdev@...r.kernel.org, brouer@...hat.com
Subject: Re: [RFC bpf-next PATCH] bpf: add comments to BPF ld/ldx sizes

On Wed, 17 Jan 2018 00:21:27 +0100
Daniel Borkmann <daniel@...earbox.net> wrote:

> On 01/16/2018 12:31 PM, Jesper Dangaard Brouer wrote:
> > Doc BPF ld/ldx size defines, as it help me understand the code in filter.c.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > ---
> >  0 files changed
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 395d261948de..4729d9a002d4 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -17,7 +17,7 @@
> >  #define BPF_ALU64	0x07	/* alu mode in double word width */
> >  
> >  /* ld/ldx fields */
> > -#define BPF_DW		0x18	/* double word */
> > +#define BPF_DW		0x18	/* double word (64-bit) */
> >  #define BPF_XADD	0xc0	/* exclusive add */
> >  
> >  /* alu/jmp fields */
> > diff --git a/include/uapi/linux/bpf_common.h b/include/uapi/linux/bpf_common.h
> > index 18be90725ab0..ee97668bdadb 100644
> > --- a/include/uapi/linux/bpf_common.h
> > +++ b/include/uapi/linux/bpf_common.h
> > @@ -15,9 +15,10 @@
> >  
> >  /* ld/ldx fields */
> >  #define BPF_SIZE(code)  ((code) & 0x18)
> > -#define		BPF_W		0x00
> > -#define		BPF_H		0x08
> > -#define		BPF_B		0x10
> > +#define		BPF_W		0x00 /* 32-bit */
> > +#define		BPF_H		0x08 /* 16-bit */
> > +#define		BPF_B		0x10 /*  8-bit */
> > +/* eBPF		BPF_DW		0x18    64-bit */  
> 
> Hmm, I don't really mind, but we do have it documented in:
> 
>   Documentation/networking/filter.txt +942
>
> Feels like if we put a comment only on BPF_{B,H,W}, then we
> might also want to document all the others such as ALU ops,
> etc.

We can start out small. I made an actual mistake by misunderstanding
these sizes (this was also because BPF_DW is located in another file,
so I didn't deduce BPF_W was 32-bit not 64-bit).  I missed the
documentation you reference.  Documentation is good, but I practice
placing the documentation as close as possible to where you need it. In
a programming setting, I looked up the define BPF_W (with cscope) in a
second, while it will take minutes finding the right documentation.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer