Received: by 2002:ab2:6d45:0:b0:1fb:d597:ff75 with SMTP id d5csp151112lqr; Wed, 5 Jun 2024 01:39:31 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCWetEI3g+QIQ78oaJ6AdiLv4MBzrPMrzbcxld6qFZNtU2V2HQPI0jdW65xLNrUBYP8mttPa7u11bR4mT0KTyozCLdDAaILUSAJWUHx28w== X-Google-Smtp-Source: AGHT+IHsxHO5sXoDr/HJ/CRZe6+nIUvLCSG1OeAXogd3yKZdmL4SkR897HWEDM8EzkiCYIgyS7lk X-Received: by 2002:a05:622a:51:b0:43b:9f:5ac8 with SMTP id d75a77b69052e-44029ef09demr34574701cf.31.1717576771479; Wed, 05 Jun 2024 01:39:31 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1717576771; cv=pass; d=google.com; s=arc-20160816; b=pgaPnREAzNkZIjVoEt8rDvUfbQqAE1GPhnhp53A8dXJLuDWMkqv3z4UoQj7AsmAxGZ YvsahTNNh5hEL3C5BSQVM11Pxo/l4HLxjj/EVqkjoCzw0TASgShHAtOhDxhj5p/OMd03 pvVHP3x8K7aBCvPubSrCeAHb0loILkTaDg6MHoVgWmovb916p620kKBxud+77lUoXDrz cb8QoJLqR3OukIUggpwKUx1FRMP3A2D4AiZTbRE9DKrqDQiPkZDvW1+F0SjAjNQyWtPa K5ksnwpqu6Eka+1qYqhFfd/kcC5MfT103AC2gRtcVnv7Hb1R8SZuL9BVZO24t0Z7+8Ml Ge2g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:references :message-id:subject:cc:to:from:date:dkim-signature:dkim-filter; bh=TrGB2Ttu+2GQP4P667beS3OkIPRmk8NFPGqeTHHLFWQ=; fh=AkKHHLzkju8WhH4DNbxAHhVSDLSvVOTPoPFAhUKv2W4=; b=lW7oTJBqNYtn2JVL6XftRvAXMzGQqAraTu82mCgRvkp+eevmt1NXkVfCsuEd+xF6Kq HO5cXzHm6zPHcAPRTK6xwXYv4tmZjuI4hF0MuKOy4ISftjXuAVjwarHcC/sk7/H+RO8q ZQ9qHHYTlSwMs5KlV7+8z0L84y2HEWBcxFgGExKOaGeG6CG1l47XHq3fwyUkGxHr/+cW cbACsWs1TMBA2TSNslUJ45HG2KpUVxqh/9ZYxjxsDG9OmHUh1qMVxcge77aRrPmb40ct fX7tDjLECnm/Eh6fOjk8nTs4wPAPiFEV3kJtxbEyKKmzQxfv8kvqfmwYME9Sw2u5F2xL EesQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=HanVOSjg; arc=pass (i=1 spf=pass spfdomain=linux.microsoft.com dkim=pass dkdomain=linux.microsoft.com dmarc=pass fromdomain=linux.microsoft.com); spf=pass (google.com: domain of linux-kernel+bounces-202023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-202023-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id d75a77b69052e-44025bc98acsi30016401cf.784.2024.06.05.01.39.31 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Jun 2024 01:39:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-202023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=HanVOSjg; arc=pass (i=1 spf=pass spfdomain=linux.microsoft.com dkim=pass dkdomain=linux.microsoft.com dmarc=pass fromdomain=linux.microsoft.com); spf=pass (google.com: domain of linux-kernel+bounces-202023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-202023-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 161E81C2111A for ; Wed, 5 Jun 2024 08:39:31 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 4E1C21946CE; Wed, 5 Jun 2024 08:39:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b="HanVOSjg" Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by smtp.subspace.kernel.org (Postfix) with ESMTP id D5C324963E; Wed, 5 Jun 2024 08:39:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=13.77.154.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717576748; cv=none; b=bBqgBXiHTKtnlmPGPGLsThf5q+nlPblj+RoNSQnDcxFvGogo1gSSdTBVAyfURlt9SXu5Juq+wODpwbT3vDhJfnXg7Ef6x3wpAdSbNXIwSSDO/+z/QuHIBnG7We/UhcbUka0TqXDNitp3+Tv0CCvLUVSbOrNH2sDNZdOGRuBnSIk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717576748; c=relaxed/simple; bh=CNAuilfiRwwKm0fGNUOB0FH43WBrxg4Srl1Cepe++Do=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ashag912Xmsgkj1v3d1VQakwdi438KNAGYLQtacsom2XSKIaWvXcsJ9j5jxCUbKxTUb6pLBomch2ei33E8hN+qtkz8xV+BybGqx78jagWqsUUBGNzw0LJqI9AVFj8QJl1uTLv4LW7Q3LCrm0+upQLMWVY8Lbcfagm7tTCYhfyiw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com; spf=pass smtp.mailfrom=linux.microsoft.com; dkim=pass (1024-bit key) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b=HanVOSjg; arc=none smtp.client-ip=13.77.154.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.microsoft.com Received: by linux.microsoft.com (Postfix, from userid 1134) id 678D120B9260; Wed, 5 Jun 2024 01:39:06 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 678D120B9260 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1717576746; bh=TrGB2Ttu+2GQP4P667beS3OkIPRmk8NFPGqeTHHLFWQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HanVOSjgakHW3wIPeXeqrGKQ1S3YE4V/rsqZcbU8ENuI6HzP2JbQ/gZM2NzgEAvV9 yutTk2fmzDfEd/kXALFdNom5jAXN17IeP5kKUuv8Xwe4sB7CPBfj4TsdS90ZzkH58c fDqpCeXRUSUtle0vYZxpyFrajGZLw4X+sjyHRrpQ= Date: Wed, 5 Jun 2024 01:39:06 -0700 From: Shradha Gupta To: Simon Horman Cc: linux-hardening@vger.kernel.org, netdev@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org, Colin Ian King , Ahmed Zaki , Pavan Chebbi , Souradeep Chakrabarti , Konstantin Taranov , Kees Cook , Paolo Abeni , Jakub Kicinski , Eric Dumazet , "David S. Miller" , Dexuan Cui , Wei Liu , Haiyang Zhang , "K. Y. Srinivasan" , Leon Romanovsky , Jason Gunthorpe , Long Li , Shradha Gupta Subject: Re: [PATCH net-next v3] net: mana: Allow variable size indirection table Message-ID: <20240605083906.GA15889@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1717169861-15825-1-git-send-email-shradhagupta@linux.microsoft.com> <20240604093349.GP491852@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240604093349.GP491852@kernel.org> User-Agent: Mutt/1.5.21 (2010-09-15) On Tue, Jun 04, 2024 at 10:33:49AM +0100, Simon Horman wrote: > On Fri, May 31, 2024 at 08:37:41AM -0700, Shradha Gupta wrote: > > Allow variable size indirection table allocation in MANA instead > > of using a constant value MANA_INDIRECT_TABLE_SIZE. > > The size is now derived from the MANA_QUERY_VPORT_CONFIG and the > > indirection table is allocated dynamically. > > > > Signed-off-by: Shradha Gupta > > Reviewed-by: Dexuan Cui > > Reviewed-by: Haiyang Zhang > > ... > > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c > > ... > > > @@ -2344,11 +2352,33 @@ static int mana_create_vport(struct mana_port_context *apc, > > return mana_create_txq(apc, net); > > } > > > > +static int mana_rss_table_alloc(struct mana_port_context *apc) > > +{ > > + if (!apc->indir_table_sz) { > > + netdev_err(apc->ndev, > > + "Indirection table size not set for vPort %d\n", > > + apc->port_idx); > > + return -EINVAL; > > + } > > + > > + apc->indir_table = kcalloc(apc->indir_table_sz, sizeof(u32), GFP_KERNEL); > > + if (!apc->indir_table) > > + return -ENOMEM; > > + > > + apc->rxobj_table = kcalloc(apc->indir_table_sz, sizeof(mana_handle_t), GFP_KERNEL); > > + if (!apc->rxobj_table) { > > + kfree(apc->indir_table); > > Hi, Shradha > > Perhaps I am on the wrong track here, but I have some concerns > about clean-up paths. > > Firstly. I think that apc->indir_table should be to NULL here for > consistency with other clean-up paths. Or alternatively, fields of apc > should not set to NULL elsewhere after being freed. Hi Simon, Thanks for the comments. This makes sense, I am planning of consistently removing the NULLify from other places too as per Leon's comments. > > In looking into this I noticed that mana_probe() does not call > mana_remove() or return an error in the cases where mana_probe_port() or > mana_attach() fail unless add_adev also fails. If so, is that intentional? Right, so most calls like mana_probe_port(), mana_attach() cleanup after themselves in the code if there is any error. So, not having to call mana_remove() in these cases in mana_probe() is intentional. But I do agree that an error is returned in mana_probe() only if add_adev also fails. I'll fix that too in the next version > > In any case, I would suggest as a follow-up, arranging things so that when > an error occurs in a function, anything that was allocated is unwound > before returning an error. > > I think this would make allocation/deallocation easier to reason with. > And I suspect it would avoid both the need for fields of structures to be > zeroed after being freed, and the need to call mana_remove() from > mana_probe(). Agreed > > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > static void mana_rss_table_init(struct mana_port_context *apc) > > { > > int i; > > > > - for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) > > + for (i = 0; i < apc->indir_table_sz; i++) > > apc->indir_table[i] = > > ethtool_rxfh_indir_default(i, apc->num_queues); > > } > > ... > > > @@ -2739,11 +2772,17 @@ static int mana_probe_port(struct mana_context *ac, int port_idx, > > err = register_netdev(ndev); > > if (err) { > > netdev_err(ndev, "Unable to register netdev.\n"); > > - goto reset_apc; > > + goto free_indir; > > } > > > > return 0; > > > > +free_indir: > > + apc->indir_table_sz = 0; > > + kfree(apc->indir_table); > > + apc->indir_table = NULL; > > + kfree(apc->rxobj_table); > > + apc->rxobj_table = NULL; > > reset_apc: > > kfree(apc->rxqs); > > apc->rxqs = NULL; > > nit: Not strictly related to this patch, but the reset_apc code should > probably be a call to mana_cleanup_port_context() as it is the dual of > mana_init_port_context() which is called earlier in mana_probe_port() Sure, let me do that too. > > ... > > > @@ -2931,6 +2972,11 @@ void mana_remove(struct gdma_dev *gd, bool suspending) > > } > > > > unregister_netdevice(ndev); > > + apc->indir_table_sz = 0; > > + kfree(apc->indir_table); > > + apc->indir_table = NULL; > > + kfree(apc->rxobj_table); > > + apc->rxobj_table = NULL; > > The code to free and zero indir_table_sz and indir_table appears twice > in this patch. Perhaps a helper to do this, which would be the dual > of mana_rss_table_alloc is in order. Makes sense, will change this too. Thanks, Shradha. > > > > > rtnl_unlock(); > > > > ...