linux-kernel - Re: [GIT PULL] SCSI fixes for 4.16-rc5

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: <CA+55aFzRss7BfRbLW8fsO5mRhj5obX9sg2ebkNSp5G68VmpWvg@mail.gmail.com>
Date:   Wed, 14 Mar 2018 17:09:14 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     James Bottomley <James.Bottomley@...senpartnership.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] SCSI fixes for 4.16-rc5

On Wed, Mar 14, 2018 at 4:28 PM, James Bottomley
<James.Bottomley@...senpartnership.com> wrote:
> -       vfree(ha->optrom_buffer);
> -       kfree(ha->nvram);
> -       kfree(ha->npiv_info);
> -       kfree(ha->swl);
> -       kfree(ha->loop_id_map);
> +
> +       if (ha->optrom_buffer)
> +               vfree(ha->optrom_buffer);
> +       if (ha->nvram)
> +               kfree(ha->nvram);
> +       if (ha->npiv_info)
> +               kfree(ha->npiv_info);
> +       if (ha->swl)
> +               kfree(ha->swl);
> +       if (ha->loop_id_map)
> +               kfree(ha->loop_id_map);

Why? This part is just garbage.

I've pulled it, but I don't see why (and how) this kind of garbage got
reviewed and acked by multiple people.

Both vfree and kfree are perfectly happy with NULL pointers, and there
doesn't seem to be any structural reason to have the test.

There *can* be valid reasons to do those kinds of things, if NULL is
the common fast-path case, and you have profiles that show that the
cost of just the call is noticeable. Then you probably also should
have an "unlikely()" there to document that fact.

But this is not one of those cases. This is just garbage and generates
less legible code.

                          Linus