| Message ID | Y1oPDy2mpOd91+Ii@sol.localdomain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [GIT,PULL] fscrypt fix for 6.1-rc3 | expand |
On Wed, Oct 26, 2022 at 9:54 PM Eric Biggers <ebiggers@kernel.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
The pull request you sent on Wed, 26 Oct 2022 21:54:39 -0700:
> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git tags/fscrypt-for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/200204f56f3b5a464c719ddb930a1a2557562dda
Thank you!
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@kernel.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
On Thu, Oct 27, 2022 at 8:13 PM Eric Biggers <ebiggers@kernel.org> wrote: > > 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(). Ouch. I tried to look for things like that, but it's clearly indirect through 'mk' so I missed it. All the callers except for put_crypt_info() do seem to have the 'sb' pointer, and I _think_ sb is inode->i_sb in that case. And this seems to *literally* be the only use of 'mk->mk_sb' in the whole data structure, so I think it's all wrong, and that field just shouldn't exist at all, but be passed into the (only) user as an argument. Oh well. Whatever. I think the code is ugly, but it is what it is. It may not be worth the churn of fixing. Linus
The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780: Linux 6.1-rc1 (2022-10-16 15:36:24 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git tags/fscrypt-for-linus for you to fetch changes up to ccd30a476f8e864732de220bd50e6f372f5ebcab: fscrypt: fix keyring memory leak on mount failure (2022-10-19 20:54:43 -0700) ---------------------------------------------------------------- Fix a memory leak that was introduced by a change that went into -rc1. ---------------------------------------------------------------- Eric Biggers (1): fscrypt: fix keyring memory leak on mount failure fs/crypto/keyring.c | 17 +++++++++++------ fs/super.c | 3 ++- include/linux/fscrypt.h | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-)