From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7EE26C4345F for ; Fri, 19 Apr 2024 08:51:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Message-ID:Date:Subject:CC:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Drd4NR2IgVUtVWtpvgjFaxZvXBxY6PVf2F85FP9p1zg=; b=4oEfq5IT0Xo/ege2hjNxtqFgv9 54x/JxqKp32ohsTyJ5bWuMHAdd9gBI1KeNYlyDszNOsJJirGEbcjmFYQc6CkeRsPW/9jyNYyd6daS zoYhDobhSgtTi0svdrvFgm66PtGGwNqgGAHzuRV+n0UgK+38Zvwbq9KnyLoSe8E4seJdwFDpWVLQr jQBFMVaM+s7d9Y2W4Otjxq/2PzLmDYZegqIfuopQgqaNiirpKsjbV68hKBCIEuR21JINIJtsQfzUi f0KcaT90Pl0oX03xeJ5JJlOec5J9DS/3ukXigk/C8h3upltX1gcXHglYK9LIGUzgGLA+Lqbdwu94Q A/I1zM2g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rxjxb-00000004zVh-2uKt; Fri, 19 Apr 2024 08:51:07 +0000 Received: from mailgw01.mediatek.com ([216.200.240.184]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rxjxY-00000004zUK-49F5; Fri, 19 Apr 2024 08:51:06 +0000 X-UUID: f1a0aa64fe2911eeac1957ae9f99f617-20240419 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:CC:To:From; bh=Drd4NR2IgVUtVWtpvgjFaxZvXBxY6PVf2F85FP9p1zg=; b=NWSKgK6dp+5WB/2jyj0KilVaP93YJ0etiXLHjtxvco/9EBe0h8+QV0LhiJz6saTBMio+D6SQLoaBCzKhdgVLC+FBxWVcVoYjSEbUfS5p1zYNha8AdesPiCEKe7dxvnSyLnWeIS/REVPx12HMNTKcbbNb9SXTYoDwz2kakLD6/3E=; X-CID-P-RULE: Release_Ham X-CID-O-INFO: VERSION:1.1.38,REQID:e6defc14-08bf-4632-8851-f73754e29a68,IP:0,U RL:0,TC:0,Content:0,EDM:0,RT:0,SF:0,FILE:0,BULK:0,RULE:Release_Ham,ACTION: release,TS:0 X-CID-META: VersionHash:82c5f88,CLOUDID:69121cfb-ed05-4274-9204-014369d201e8,B ulkID:nil,BulkQuantity:0,Recheck:0,SF:102,TC:nil,Content:0,EDM:-3,IP:nil,U RL:11|1,File:nil,RT:nil,Bulk:nil,QS:nil,BEC:nil,COL:0,OSI:0,OSA:0,AV:0,LES :1,SPR:NO,DKR:0,DKP:0,BRR:0,BRE:0 X-CID-BVR: 0,NGT X-CID-BAS: 0,NGT,0,_ X-CID-FACTOR: TF_CID_SPAM_SNR,TF_CID_SPAM_ULN X-UUID: f1a0aa64fe2911eeac1957ae9f99f617-20240419 Received: from mtkmbs11n1.mediatek.inc [(172.21.101.185)] by mailgw01.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384 256/256) with ESMTP id 333620738; Fri, 19 Apr 2024 01:50:59 -0700 Received: from mtkmbs13n1.mediatek.inc (172.21.101.193) by mtkmbs10n2.mediatek.inc (172.21.101.183) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.26; Fri, 19 Apr 2024 16:40:55 +0800 Received: from mbjsdccf07.gcn.mediatek.inc (10.15.20.246) by mtkmbs13n1.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.2.1118.26 via Frontend Transport; Fri, 19 Apr 2024 16:40:54 +0800 From: Guoyong Wang To: "Jason A . Donenfeld" , Theodore Ts'o , Tejun Heo , Lai Jiangshan , "Matthias Brugger" , AngeloGioacchino Del Regno CC: , , , , "Guoyong Wang" Subject: Re: [PATCH] random: Fix the issue of '_might_sleep' function running in an atomic contex Date: Fri, 19 Apr 2024 16:41:12 +0800 Message-ID: <20240419084112.4089-1-guoyong.wang@mediatek.com> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20240417120217.3814215-2-Jason@zx2c4.com> References: <20240417120217.3814215-2-Jason@zx2c4.com> MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-Product-Ver: SMEX-14.0.0.3152-9.1.1006-23728.005 X-TM-AS-Result: No-10--18.737100-8.000000 X-TMASE-MatchedRID: EMyCvCfVN1Fq0U6EhO9EEya1MaKuob8PC/ExpXrHizwKogmGusPLb1wJ qhTPmfKkCe3jA2E8gwcQ3Uikp7+aivQYZJBBoF8RaK+MsTwM+1mBHKTJ+sfXGRxUkJPe1WBq3J8 6v+pUS6ApTBCqN/Cg+/hQCrII02E2g9rU75/tZ02sMW2Z7ncq+6ns03+XRsVeRaXolgMyx3HGXV TaHB60j/wnrdJjIWkajIO1ikJs9mfDBNgbKIiiTEhEDfw/93BugGa+oYp5i6rwNtyWWbgV+usnO fzCil8nHhgMFd/39H00L7hY/wAQOnXo24sP1JtCsyNb+yeIRApdymZBcuGGRIfAYSb4KlgZkB8g HfQ6Qd3AOq0lI6oDZxYchAzpNtsT29aHfVG01jxbuDP8ZuCmXsMdI0UcXEHzTUobVis5Bb/Rl4E KYR0JsWbURhko7VE1Goq5PC0a+a2lTHNxJDwdpD3stoORN7tbh8Ytn75ClDOo+b+yOP0oGNYyy3 /zxX0m4kYcXv9HvHLTMj+5r/4BdrxXyJr7SWpsZlRzaO1xpJ19LQinZ4QefIWH8POvJNOW4Hwn1 pZzW/9XKaQsz6vtVJBlLa6MK1y4 X-TM-AS-User-Approved-Sender: No X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--18.737100-8.000000 X-TMASE-Version: SMEX-14.0.0.3152-9.1.1006-23728.005 X-TM-SNTS-SMTP: F76AA4E8210E43E4BA69E16CC2B5BD02BA2B50B76B5ABE62256E4807296A4EBD2000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240419_015105_054658_B199751F X-CRM114-Status: GOOD ( 33.45 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Wed, 17 Apr 2024 14:01:11 +0200, Jason A. Donenfeld wrote: > The entropy accounting changes a static key when the RNG has > initialized, since it only ever initializes once. Static key changes, > however, cannot be made from atomic context, so depending on where the > last creditable entropy comes from, the static key change might need to > be deferred to a worker. > > Previously the code used the execute_in_process_context() helper > function, which accounts for whether or not the caller is > in_interrupt(). However, that doesn't account for the case where the > caller is actually in process context but is holding a spinlock. > > This turned out to be the case with input_handle_event() in > drivers/input/input.c contributing entropy: > > [] die+0xa8/0x2fc > [] bug_handler+0x44/0xec > [] brk_handler+0x90/0x144 > [] do_debug_exception+0xa0/0x148 > [] el1_dbg+0x60/0x7c > [] el1h_64_sync_handler+0x38/0x90 > [] el1h_64_sync+0x64/0x6c > [] __might_resched+0x1fc/0x2e8 > [] __might_sleep+0x44/0x7c > [] cpus_read_lock+0x1c/0xec > [] static_key_enable+0x14/0x38 > [] crng_set_ready+0x14/0x28 > [] execute_in_process_context+0xb8/0xf8 > [] _credit_init_bits+0x118/0x1dc > [] add_timer_randomness+0x264/0x270 > [] add_input_randomness+0x38/0x48 > [] input_handle_event+0x2b8/0x490 > [] input_event+0x6c/0x98 > > According to Guoyong, it's not really possible to refactor the various > drivers to never hold a spinlock there. And in_atomic() isn't reliable. > > So, rather than trying to be too fancy, just punt the change in the > static key to a workqueue always. There's basically no drawback of doing > this, as the code already needed to account for the static key not > changing immediately, and given that it's just an optimization, there's > not exactly a hurry to change the static key right away, so deferal is > fine. > > Reported-by: Guoyong Wang > Cc: stable@vger.kernel.org > Fixes: f5bda35fba61 ("random: use static branch for crng_ready()") > Signed-off-by: Jason A. Donenfeld > --- > Guoyong- can you test this and tell me whether it fixes the problem you > were seeing? If so, I'll try to get this sent up for 6.9. -Jason > > drivers/char/random.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 456be28ba67c..2597cb43f438 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -702,7 +702,7 @@ static void extract_entropy(void *buf, size_t len) > > static void __cold _credit_init_bits(size_t bits) > { > - static struct execute_work set_ready; > + static DECLARE_WORK(set_ready, crng_set_ready); > unsigned int new, orig, add; > unsigned long flags; > > @@ -718,8 +718,8 @@ static void __cold _credit_init_bits(size_t bits) > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > crng_reseed(NULL); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > - if (static_key_initialized) > - execute_in_process_context(crng_set_ready, &set_ready); > + if (static_key_initialized && system_unbound_wq) > + queue_work(system_unbound_wq, &set_ready); > atomic_notifier_call_chain(&random_ready_notifier, 0, NULL); > wake_up_interruptible(&crng_init_wait); > kill_fasync(&fasync, SIGIO, POLL_IN); > @@ -890,8 +890,8 @@ void __init random_init(void) > > /* > * If we were initialized by the cpu or bootloader before jump labels > - * are initialized, then we should enable the static branch here, where > - * it's guaranteed that jump labels have been initialized. > + * or workqueues are initialized, then we should enable the static > + * branch here, where it's guaranteed that these have been initialized. > */ > if (!static_branch_likely(&crng_is_ready) && crng_init >= CRNG_READY) > crng_set_ready(NULL); > -- > 2.44.0 Hi Jason, Thanks for your feedback. We concur with the proposed change and have verified that it works well in our tests. Next, I will provide a patch v2 for the changes discussed.