On Wed, Oct 26, 2022 at 9:54 PM Eric Biggers <ebiggers@xxxxxxxxxx> 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
