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: <Y1tI1ek80kCrsi2R@sol.localdomain>
Date:   Thu, 27 Oct 2022 20:13:25 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.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 Thu, Oct 27, 2022 at 11:58:03AM -0700, Linus Torvalds wrote:
> 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.
> 

Thanks Linus.  That makes sense in general, but in this case ->s_master_keys
gets used in the middle of the function, in fscrypt_put_master_key_activeref().
I maybe should have made fscrypt_put_master_key_activeref() take the super_block
as an argument, which would have made this a bit clearer.

- Eric