Received: by 2002:ab2:6309:0:b0:1fb:d597:ff75 with SMTP id s9csp502210lqt; Thu, 6 Jun 2024 09:33:56 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCUmMvgkIWUYjiNr+uJr0u4WPUFuRiSuBcPcWhZRx9JovyRu/9PEihjGa8164lhrvIU4Gh6sY+ku3uv4LzYKaq97WCyAKvgoRYVf03ubKg== X-Google-Smtp-Source: AGHT+IFWn7Sv6R6Rp8sHnkzueMORJ8kzGRcuhaa/tCeejGH+4Vxi8HUq6nYtAe7jk3OM8cjRQKwt X-Received: by 2002:a17:90b:342:b0:2c2:4109:94ab with SMTP id 98e67ed59e1d1-2c2b8980f70mr264831a91.4.1717691636308; Thu, 06 Jun 2024 09:33:56 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1717691636; cv=pass; d=google.com; s=arc-20160816; b=WM5TcZCh3ZEW4Jydo0GvAi7cZ+PV67s8xL8Fsd1bMlHS2vAws5RG2hQhBpnGNwwkF4 hgSocfwvaUJg7INW/knW5kqEXYzjX4EztiLRqgtY6G4xuL36E3zsCMm7MCwtaUvPiSnb ZtBOneLbeb5B/ip2/GcFwRxXEPpDWhi1ziO1ZD6m9pWRusijKZ0B7mWO80RR9BAqPUg5 T9A+7587LHnH4tf5bhrLy56dBkQRlXFUMKhCd6lDfY2pLiNg1BKXVjptFvwAogw0vRo3 q4n5/HNN38enEuAJEGc0v22fu4BjmGKG3M5SxIR+nHRVA3j37TQA292Qw9JwIxBOvcTh PYbQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=in-reply-to:content-disposition:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:message-id:subject:cc :to:from:date:dkim-signature; bh=7cwcFz094Cmwd9wBwtswLXjUtJzsFOsgwUQsBpyu92E=; fh=/1JAPC7lVXYtfk9mR2LIhOu8Jhc10giTT5yUf2Lr2wQ=; b=JZxvo79UedY5QZwzv03V4S8dUVxBY7xPdZthS0vNexMW7+CGnvK6j6P8WSdBt7H3f5 8xj+pzxZ13Kpa703XRP+ibrc+8ofNPcvHO+JHti3zyb0PX7YcAGXVdU2k0Fce4b5Nskh laqGa7Vufo29kdAP/9X5zsulHOs3EbN71rOj3nxmMj/1JOHGypWTyGNrLYpp+Cc8dt6f 6hdXPLsT/DVuYbaCwhygii7/gPtf7qcoT+pexyhQoHFwmn41gOSavtRUpGVXLl1RCv9v ntAps9eGPNcqJZmDXYtSF/N422KhXCP4GYABB4aoifnhiTPkqprjwLOmxauuvOknQcw8 HM7A==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=S7IgvyVy; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-204702-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-204702-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id 98e67ed59e1d1-2c29c20451csi1471194a91.38.2024.06.06.09.33.56 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jun 2024 09:33:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-204702-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=S7IgvyVy; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-204702-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-204702-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org 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 sv.mirrors.kernel.org (Postfix) with ESMTPS id BA7CE283D39 for ; Thu, 6 Jun 2024 16:33:55 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id A3CF819885E; Thu, 6 Jun 2024 16:33:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="S7IgvyVy" Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A215210E9; Thu, 6 Jun 2024 16:33:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717691621; cv=none; b=RSBhV/5fq+Eclg45OHpMARDhLih2v/PTWu5abktI6zOgmriSVhlIoeFuxG/pc6HsOgS89ykMalH22uUa+u2riqRA83M1hpQ+EVGtKRgizHvfU/uLDL9nMGlumLjnetK9ng8M1fblamh8KsT1gM/GcywVPHYwQ0glPeXNNZ1dTR4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717691621; c=relaxed/simple; bh=Dd5xuQCXjQbAM4QC/1i17ToQ5z+UxwV8krZqgKleMYw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=uXnKN5bM335cE7uWYGeMDN5AgGF4fBsGHDCjwfitlaE54wj7J68NKMfBpMvBRjvM7gDVDrqoAXlpKYTx50fiUjhJKD+X3BOQ2iGS7P5huADNsQZCKu/BfFaiVoULg/XBFleBu2u7vdtbJpQvxV9Ts665lZLuYIhXzYYBTYCJD7Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=S7IgvyVy; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id 96CD4C32786; Thu, 6 Jun 2024 16:33:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1717691621; bh=Dd5xuQCXjQbAM4QC/1i17ToQ5z+UxwV8krZqgKleMYw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=S7IgvyVyr7ONSY69TsofxX/n2zPzWdGiDcLRgwHNaOQnF4ChSokqEP6+OBPUZ4M2B koC4aXtmLTtI+gRNseSk1bhrMQBo4CfnYkPOEwOATqot2X0YSd0OyAT/mp8sAAP7TH bSV+3rqixQgyaDXW3hvpbA+1bCSRnmhKE7+ZbIJJmp5raQ/6l4XiPaiDh20wOESBHt TYqORfpFQu+hom7AZnFiGKKv4xwR4NY3MM7GQbxCDOXPFtpOhbNadzwYNI85vjaRpp rEf2QOO6Q6CJv80aHUhXQXetttQ+qYZ+oEPg8MoHqwPampnfw2vRyu1JcxfJOoreGw TxA2SMRKurJ5A== Date: Thu, 6 Jun 2024 17:33:34 +0100 From: Simon Horman To: Shradha Gupta 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: <20240606163334.GO791188@kernel.org> References: <1717169861-15825-1-git-send-email-shradhagupta@linux.microsoft.com> <20240604093349.GP491852@kernel.org> <20240605083906.GA15889@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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: <20240605083906.GA15889@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> On Wed, Jun 05, 2024 at 01:39:06AM -0700, Shradha Gupta wrote: > 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. Great! > > 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 I'm not entirely sure, but perhaps that is a candidate for a separate patch. > > > > 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. FWIIW, I think it would be appropriate to put that change in a separate patch. > > > > ... > > > > > @@ -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.