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

[Eric Biggers]

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@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.
> 

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