Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 10 Jul 2018 10:18:47 -0700
From:   Martin KaFai Lau <kafai@fb.com>
To:     David Laight <David.Laight@ACULAB.COM>
CC:     Okash Khawaja <osk@fb.com>, Daniel Borkmann <daniel@iogearbox.net>,
        "Alexei Starovoitov" <ast@kernel.org>, Yonghong Song <yhs@fb.com>,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        "David S. Miller" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "kernel-team@fb.com" <kernel-team@fb.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
Message-ID: <20180710171836.xcegzammqym4sqri@kafai-mbp.dhcp.thefacebook.com>
References: <20180709002202.019053555@fb.com>
 <20180709004002.440153594@fb.com>
 <20180709183236.r4b7gzmev5h4lcbw@kafai-mbp.dhcp.thefacebook.com>
 <457ec72d8d7c4cdd8c112f9c8c91b0e7@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <457ec72d8d7c4cdd8c112f9c8c91b0e7@AcuMS.aculab.com>
User-Agent: NeoMutt/20180512
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 10, 2018 at 04:35:04PM +0000, David Laight wrote:
> From: Martin KaFai Lau
> > Sent: 09 July 2018 19:33
> > On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> > > When extracting bitfield from a number, btf_int_bits_seq_show() builds
> > > a mask and accesses least significant byte of the number in a way
> > > specific to little-endian. This patch fixes that by checking endianness
> > > of the machine and then shifting left and right the unneeded bits.
> > >
> > > Thanks to Martin Lau for the help in navigating potential pitfalls when
> > > dealing with endianess and for the final solution.
> > >
> > > Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> > > Signed-off-by: Okash Khawaja <osk@fb.com>
> > >
> > > ---
> > >  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
> > >  1 file changed, 15 insertions(+), 17 deletions(-)
> > >
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -162,6 +162,8 @@
> > >  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> > >  #define BITS_ROUNDUP_BYTES(bits) \
> > >  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> > > +const int one = 1;
> > > +#define is_big_endian() ((*(char *)&one) == 0)
> > >
> > >  #define BTF_INFO_MASK 0x0f00ffff
> > >  #define BTF_INT_MASK 0x0fffffff
> > > @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
> > >  				  void *data, u8 bits_offset,
> > >  				  struct seq_file *m)
> > >  {
> > > +	u8 left_shift_bits, right_shift_bits;
> > Nit.
> > Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> > it is better to use u16 such that it will be consistent to BTF_INT_BITS.
> 
> Double-nit.
> 
> Use 'int' or 'unsigned int'.
> Sub-word arithmetic will require extra instructions on almost everything
> except x86.
I would prefer to keep it as u16 which is the max width that is allowed for
this field in the wire format.  Keeping the usage consistent can avoid
accidentally incorrect offsetting or writing wrong data out in other
cases.