Date:   Wed, 8 Jul 2020 13:34:09 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     Alex Elder <elder@...aro.org>, "# 3.4.x" <stable@...r.kernel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH] bitfield.h: don't compile-time validate _val in
 FIELD_FIT

On Wed, 8 Jul 2020 10:56:43 -0700 Nick Desaulniers wrote:
> On Wed, Jul 8, 2020 at 10:34 AM Alex Elder <elder@...aro.org> wrote:
> >
> > I understand why something needs to be done to handle that case.
> > There's fancy macro gymnastics in "bitfield.h" to add convenient
> > build-time checks for usage problems; I just thought there might
> > be something we could do to preserve the checking--even in this
> > case.  But figuring that out takes more time than I was willing
> > to spend on it yesterday...  
> 
> I also find the use of 0U in FIELD_GET sticks out from the use of 0ULL
> or (0ull) in these macros (hard to notice, but I changed it in my diff
> to 0ULL).  Are there implicit promotion+conversion bugs here?  I don't
> know, but I'd rather not think about it by just using types of the
> same width and signedness.

TBH I just copied the type from other arguments. It doesn't matter
in practice now in this case. I have no preference.

> > >> A second comment about this is that it might be nice to break
> > >> __BF_FIELD_CHECK() into the parts that verify the mask (which
> > >> could be used by FIELD_FIT() here) and the parts that verify
> > >> other things.  
> > >
> > > Like so? Jakub, WDYT? Or do you prefer v1+Alex's suggestion about
> > > using `(typeof(_mask))0` in place of 0ULL?  
> >
> > Yes, very much like that!  But you could do that as a follow-on
> > instead, so as not to delay or confuse things.  
> 
> No rush; let's get it right.
> 
> So I can think of splitting this into maybe 3 patches, based on feedback:
> 1. there's a bug in compile time validating _val in FIELD_FIT, since
> we want to be able to call it at runtime with "bad" values.
> 2. the FIELD_* macros use constants (0ull, 0ULL, 0U) that don't match
> typeof(_mask).
> 3. It might be nice to break up __BF_FIELD_CHECK.
>
> I don't think anyone's raised an objection to 1.
> 
> Assuming Jakub is ok with 3, fixing 3 will actually also address 2.
> So then we don't need 3 patches; only 2.  But if we don't do 3 first,
> then I have to resend a v2 of 1 anyways to address 2 (which was Alex's
> original feedback).
> 
> My above diff was all three in one go, but I don't think it would be
> unreasonable to break it up into 3 then 1.
> 
> If we prefer not to do 3, then I can send a v2 of 1 that addresses the
> inconsistent use of types, as one or two patches.
> 
> Jakub, what is your preference?

I don't see much point in breaking up the checking macro. But even less
in arguing either way :)

> (Also, noting that I'm sending to David, assuming he'll pick up the
> patches once we have everyone's buy in? Or is there someone else more
> appropriate to accept changes to this header? I guess Jakub and David
> are the listed maintainers for
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c)

Seems reasonable, put [PATCH net] in the subject to make that explicit.

Powered by blists - more mailing lists