linux-kernel - Re: [GIT PULL] fscrypt fix for 6.1-rc3

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: <CAHk-=wjDQiJn6YUJ18Nb=L82qsgx3LBLtQu0xANeVoc6OAzFtQ@mail.gmail.com>
Date:   Thu, 27 Oct 2022 11:58:03 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, "Theodore Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>
Subject: Re: [GIT PULL] fscrypt fix for 6.1-rc3

On Wed, Oct 26, 2022 at 9:54 PM Eric Biggers <ebiggers@...nel.org> wrote:
>
> Fix a memory leak that was introduced by a change that went into -rc1.

Unrelated to the patch in question, but since it made me look, I wish
code like that fscrypt_destroy_keyring() function would be much more
obvious about the whole "yes, I can validly be called multiple times"
(not exactly idempotent, but you get the idea).

Yes, it does that

        struct fscrypt_keyring *keyring = sb->s_master_keys;
        ...
        if (!keyring)
                return;
        ...
        sb->s_master_keys = NULL;

but it's all spread out so that you have to actually look for it (and
check that there's not some other early return).

Now, this would need an atomic xchg(NULL) to be actually thread-safe,
and that's not what I'm looking for - I'm just putting out the idea
that for functions that are intentionally meant to be cleanup
functions that can be called multiple times serially, we should strive
to make that more clear.

Just putting that sequence together at the very top of the function
would have helped, being one simple visually obvious pattern:

        keyring = sb->s_master_keys;
        if (!keyring)
                return;
        sb->s_master_keys = NULL;

makes it easier to see that yes, it's fine to call this sequentially.

It also, incidentally, tends to generate better code, because that
means that we're just done with 'sb' entirely after that initial
sequence and that it has better register pressure and cache patterns.

No, that code generation is not really important here, but just a sign
that this is just a good coding pattern in general - not just good for
people looking at the code, but for the compiler and hardware too.

                   Linus