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
|