Message ID | Y1oPDy2mpOd91+Ii@sol.localdomain |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp31266wru; Wed, 26 Oct 2022 21:59:30 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5nUfQEWkbyTrQC4FVCmtwnmp/25JrB+1tOinlR1p21kl2N8VbgjyoE0lFXttOtywIp8Idz X-Received: by 2002:a63:4283:0:b0:457:dced:8ba3 with SMTP id p125-20020a634283000000b00457dced8ba3mr40154549pga.220.1666846770147; Wed, 26 Oct 2022 21:59:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666846770; cv=none; d=google.com; s=arc-20160816; b=XF76cXWPfnvRT/OTVu4LR0mthVsZbp8BgjfjOTs80WSBc7kM8P0olPxXbqZ+6Qhcxw Urnb3LS34i0Dk5X+ssSg2P2J/YlSzX5Jne9wGsJTRIZttBwZ/aJvBUkCZbvRfi9vzk46 XeV8iGJ2rwTp0bM/eEPU9G8B4CTLRqbqqiV73ysjbwYRML7nLF42gb5YHHW+s67bzng1 V7el02ER57ZjY+pdITojvDKtzoqqLktn3jgYEdiceHAbYT6ybz+gDFDh8+5ZAGs87HjB QEZEAIP1WqEIQwvmYkotsFO+jb07R6yzQlDgdIWWLW+3za7CimEOJBFE6nkPjo9Ukqf9 sMVQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=ypnS8AQzvNZ0EGSFDtyY2Z9aw18GUWOR5SxdZRfwx2I=; b=HAbL4FR+NCI4IBqGtBLmDBWbvWWj634eWFGzYnR2+KY/rLZ6CycL47yQc+EAR2eMN8 IfKWFclXECUuoqGSlArKoIWvw0K76NpOxjstHULXGmIBuRXNEFxc/JdgoG9GCDiRrocx HyR3Zb27VddEZw7w1GYiJ57G5Kz7NCtQmrV7QQpWZgG9TZ7hteytoYBTUxjkVe6vv2NH BNR5nE/N6REZ/xIRUzLQuVOGkXKh1VYx/MTPCbrqb76+dRFXEhZ7n7wYdnFmqCjTR6J+ IqVzwzI9fRqGx9lIdy8cBwuwf76IfVXQ2GIsElpGw8jyselX1IaJh7jPNYEze/Un6jwN Cy6A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=QRYIrPM3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t11-20020a63dd0b000000b00434f41f6981si398461pgg.360.2022.10.26.21.59.14; Wed, 26 Oct 2022 21:59:30 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=QRYIrPM3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234230AbiJ0Eys (ORCPT <rfc822;hiifong.im@gmail.com> + 99 others); Thu, 27 Oct 2022 00:54:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50578 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234220AbiJ0Eyp (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 27 Oct 2022 00:54:45 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B242963D7; Wed, 26 Oct 2022 21:54:42 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 5B07E6216E; Thu, 27 Oct 2022 04:54:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 84BDBC433D6; Thu, 27 Oct 2022 04:54:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1666846481; bh=un9lzumgYGFX4N31OPIAznIJ14qEZHgfKlS9DrUzqA4=; h=Date:From:To:Cc:Subject:From; b=QRYIrPM313GyosWvE+FOcKoxg3goWTSbV+S6s5InwdsO24p5oonKkiNmEFxxjBBcf nh/Hi2Gl3ELVetHY8KmGb3qjLpj5wyAGM0p/QgXbctc7Q3auFJz0p9nZ4GprUlOUrz +tYD3eRqdahnusnU7kBuKLU5zDrDNnEg6D152ctsCHRpibOY1paUBdk62g7k49mZmO Ez2hYckWfdA0lS0hYqSVanmSGjDbCEqu/9qMQvKL2+YZktspSJpudeEbCIz7REZTHq Eb4k44OwwkRafpFGlzSNNz5MhOmzxoAa5xGDJSW+wDpsPnSAH3pjmrq1V32M/Wrh3Q K462hLShUXVBw== Date: Wed, 26 Oct 2022 21:54:39 -0700 From: Eric Biggers <ebiggers@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>, Jaegeuk Kim <jaegeuk@kernel.org> Subject: [GIT PULL] fscrypt fix for 6.1-rc3 Message-ID: <Y1oPDy2mpOd91+Ii@sol.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747815518441314948?= X-GMAIL-MSGID: =?utf-8?q?1747815518441314948?= |
Series |
[GIT,PULL] fscrypt fix for 6.1-rc3
|
|
Pull-request
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git tags/fscrypt-for-linusMessage
Eric Biggers
Oct. 27, 2022, 4:54 a.m. UTC
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(-)
Comments
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