linux-kernel - Re: [PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list

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: <CABVgOSkegmhmeRa=7Qcx3MnX88wLy9qZx97CMhk4NvWb-pgpYQ@mail.gmail.com>
Date:   Tue, 22 Oct 2019 15:13:28 -0700
From:   David Gow <davidgow@...gle.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Shuah Khan <shuah@...nel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH linux-kselftest/test v4] lib/list-test: add a test for the
 'list' doubly linked list

On Sat, Oct 19, 2019 at 1:27 AM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> On Fri, Oct 18, 2019 at 02:55:49PM -0700, David Gow wrote:
> > +     list4 = kzalloc(sizeof(*list4), GFP_KERNEL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4);
>
> Why not just use GFP_KERNEL | GFP_NOFAIL and remove the check?

I've sent a new version of the patch out (v5) which uses __GFP_NOFAIL instead.

The idea had been to exercise KUnit's assertion functionality, in the
hope that it'd allow the test to fail (but potentially allow other
tests to still run) in the case of allocation failure. Given that
we're only allocating enough to store ~4 pointers in total, though,
that's probably of little use.

> kzalloc() can't return error pointers.  If this were an IS_ERR_OR_NULL()
> check then it would generate a static checker warning, but static
> checkers don't know about KUNIT_ASSERT_NOT_ERR_OR_NULL() yet so you're
> safe.

Alas, KUnit doesn't have a KUNIT_ASSERT_NOT_NULL() macro, and I'd
assumed it was not dangerous (even if not ideal) to check for error
pointers, even if kzalloc() can't return them.

Perhaps it'd make sense to add a convenient way of checking the
NULL-ness of pointers to KUnit (it's possible with the
KUNIT_ASSERT_PTR_EQ(), but requires a bit of casting to make the type
checker happy) in the future. Once KUnit is properly upstream, it may
be worth teaching the static analysis tools about these functions to
avoid having warnings in these sorts of tests.

For now, though, (and for this test in particular), I agree with the
suggestion of just using __GFP_NOFAIL.

Thanks a lot for the comments,
-- David