netdev - Re: [PATCH] bitfield.h: don't compile-time validate _val in FIELD

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]

Date:   Wed, 8 Jul 2020 10:56:43 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Alex Elder <elder@...aro.org>, Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>
Cc:     "# 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, 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.

> >> 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?

(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)
-- 
Thanks,
~Nick Desaulniers