linux-kernel - Re: [PATCH v2 RESEND] kunit: debugfs: Handle errors from alloc_string

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: <a863d7dd-d982-462c-af5d-0ef0b48ea953@suswa.mountain>
Date:   Fri, 1 Dec 2023 10:50:50 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Rae Moar <rmoar@...gle.com>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>,
        brendan.higgins@...ux.dev, davidgow@...gle.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v2 RESEND] kunit: debugfs: Handle errors from
 alloc_string_stream()

On Thu, Nov 30, 2023 at 04:11:18PM -0500, Rae Moar wrote:
> > +       stream = alloc_string_stream(GFP_KERNEL);
> > +       if (IS_ERR_OR_NULL(stream))
> 
> In response to Dan Carpenter's comment from the last version, I see
> the benefits of changing IS_ERR_OR_NULL() to IS_ERR() instead because
> "stream" will not be NULL. This would then also be the same as the
> check in kunit_alloc_string_stream.
> 
> However, I also see the benefit of checking for NULL just in case anyways.
> 

Returning NULL in alloc_string_stream() is a bug.  Checking for NULL is
a work around for bugs.  There are basically two times where it can
be valid to work around bugs like this instead of fixing them.  1)  When
the function is implemented by over 10 different driver authors.  In
that case you can guarantee that at least one of them is going to do the
wrong thing.  There are between 2-5 places which do this in the kernel.
2)  If it's a API that used to return NULL and it's changed to returning
error pointers.  I've never seen anyone do this, but I've proposed it as
a solution to make backporting easier.

regards,
dan carpenter