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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D47DAC433FE for ; Mon, 7 Nov 2022 17:45:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232308AbiKGRpt (ORCPT ); Mon, 7 Nov 2022 12:45:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53840 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232132AbiKGRpY (ORCPT ); Mon, 7 Nov 2022 12:45:24 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 430AA22297 for ; Mon, 7 Nov 2022 09:45:23 -0800 (PST) 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 ams.source.kernel.org (Postfix) with ESMTPS id DA37FB81608 for ; Mon, 7 Nov 2022 17:45:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 00C71C433C1; Mon, 7 Nov 2022 17:45:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1667843120; bh=cL0Cmn1wMSgyZwA7z18q8ng4pj7xfO231EleFGDrTNs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cKLweVQxAximlaYYUdVyVnlFYThWpbJvwt/OhHpsbx0ZhBjl+pqGe2N05tE5v4gzH FEIC13zcYsdlm/CSLM/RCPBYrbq+CEj7lePrRvbyt1bXIDq93WRKQ49ZEYsgsN2eKI pZfRawMG4B3K3I41fnUDcXRApzmmojsuoX/V/n8Io0K+3aYyMXQ1JwGgN6RKIDl2T1 H3oIaOpCLds4DQbsa8aGZ8TTm65XPqyb1cBTgIF/GSRuP/uKm5hDQmtN5vDvLb1Rg3 HaWmEdKsh2IJCixttrZZ4yUtiyAfFFYy1P/CkxTVoaHOAfBSgH59G+4P0QH/ipCX8t Xh8taIO0hAnng== Date: Mon, 7 Nov 2022 19:45:15 +0200 From: Leon Romanovsky To: "Ruhl, Michael J" Cc: "Nguyen, Anthony L" , "davem@davemloft.net" , "kuba@kernel.org" , "pabeni@redhat.com" , "edumazet@google.com" , Kees Cook , "netdev@vger.kernel.org" , "Brandeburg, Jesse" , "intel-wired-lan@lists.osuosl.org" , "Keller, Jacob E" , "G, GurucharanX" Subject: Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one was allocated Message-ID: References: <20221104205414.2354973-1-anthony.l.nguyen@intel.com> <20221104205414.2354973-6-anthony.l.nguyen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Nov 07, 2022 at 01:55:58PM +0000, Ruhl, Michael J wrote: > >-----Original Message----- > >From: Leon Romanovsky > >Sent: Monday, November 7, 2022 2:03 AM > >To: Nguyen, Anthony L > >Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > >edumazet@google.com; Kees Cook ; > >netdev@vger.kernel.org; Brandeburg, Jesse ; > >intel-wired-lan@lists.osuosl.org; Ruhl, Michael J ; > >Keller, Jacob E ; G, GurucharanX > > > >Subject: Re: [PATCH net-next 5/6] igb: Do not free q_vector unless new one > >was allocated > > > >On Fri, Nov 04, 2022 at 01:54:13PM -0700, Tony Nguyen wrote: > >> From: Kees Cook > >> > >> Avoid potential use-after-free condition under memory pressure. If the > >> kzalloc() fails, q_vector will be freed but left in the original > >> adapter->q_vector[v_idx] array position. > >> > >> Cc: Jesse Brandeburg > >> Cc: Tony Nguyen > >> Cc: "David S. Miller" > >> Cc: Eric Dumazet > >> Cc: Jakub Kicinski > >> Cc: Paolo Abeni > >> Cc: intel-wired-lan@lists.osuosl.org > >> Cc: netdev@vger.kernel.org > >> Signed-off-by: Kees Cook > >> Reviewed-by: Michael J. Ruhl > >> Reviewed-by: Jacob Keller > >> Tested-by: Gurucharan (A Contingent worker > >at Intel) > > > >You should use first and last names here. > > > >> Signed-off-by: Tony Nguyen > >> --- > >> drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c > >b/drivers/net/ethernet/intel/igb/igb_main.c > >> index d6c1c2e66f26..c2bb658198bf 100644 > >> --- a/drivers/net/ethernet/intel/igb/igb_main.c > >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c > >> @@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter > >*adapter, > >> if (!q_vector) { > >> q_vector = kzalloc(size, GFP_KERNEL); > >> } else if (size > ksize(q_vector)) { > >> - kfree_rcu(q_vector, rcu); > >> - q_vector = kzalloc(size, GFP_KERNEL); > >> + struct igb_q_vector *new_q_vector; > >> + > >> + new_q_vector = kzalloc(size, GFP_KERNEL); > >> + if (new_q_vector) > >> + kfree_rcu(q_vector, rcu); > >> + q_vector = new_q_vector; > > > >I wonder if this is correct. > >1. if new_q_vector is NULL, you will overwrite q_vector without releasing it. > >2. kfree_rcu() doesn't immediately release memory, but after grace > >period, but here you are overwriting the pointer which is not release > >yet. > > The actual pointer is: adapter->q_vector[v_idx] > > q_vector is just a convenience pointer. > > If the allocation succeeds, the q_vector[v_idx] will be replaced (later in the code). > > If the allocation fails, this is not being freed. The original code freed the adapter > pointer but didn't not remove the pointer. > > If q_vector is NULL, (i.e. the allocation failed), the function exits, but the original > pointer is left in place. > > I think this logic is correct. > > The error path leaves the original allocation in place. If this is incorrect behavior, > a different change would be: > > q_vector = adapter->q_vector[v_idx]; > adapter->q_vector[v_idx] = NULL; > ... the original code... > > But I am not sure if that is what is desired? I understand the issue what you are trying to solve, I just don't understand your RCU code. I would expect calls to rcu_dereference() in order to get q_vector and rcu_assign_pointer() to clear adapter->q_vector[v_idx], but igb has none. Thanks