Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers

From: Dan Carpenter <dan.carpenter@linaro.org>
To: Jiri Pirko <jiri@resnulli.us>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Przemek Kitszel <przemyslaw.kitszel@intel.com>,
	intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
Date: Thu, 21 Mar 2024 18:49:29 +0300	[thread overview]
Message-ID: <dd392ed9-9f68-4364-bc9a-ab95d6b54cda@moroto.mountain> (raw)
In-Reply-To: <ZfxTjYUPAFz_LRlk@nanopsycho>

On Thu, Mar 21, 2024 at 04:34:37PM +0100, Jiri Pirko wrote:
> >The change to ice_update_link_info() isn't required because it's
> >assigned on the very next line...  But I did that because it's harmless
> >and makes __free() stuff easier to verify.  I felt like moving the
> >declarations into the code would be controversial and it also ends up
> >making the lines really long.
> >
> >		goto goto err_unroll_sched;
> >
> >	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> >		kzalloc(sizeof(*pcaps), GFP_KERNEL);
> 
> Yeah, that is why I'm proposing KZALLOC_FREE helper:
> https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
> 

I like the idea, but I'm not keen on the format.  What about something
like?

#define __ALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL)

	struct ice_aqc_get_phy_caps_data *__ALLOC(pcaps);

I'm not a huge fan of putting functions which can fail into the
declaration block but I feel like we're going to officially say that
small allocations can't fail.

https://lwn.net/Articles/964793/
https://lore.kernel.org/all/170925937840.24797.2167230750547152404@noble.neil.brown.name/

Normally we would try to delay the allocations until after all the
sanity checks have run but that's optimizing for the failure case.  In
the normal case we're going to want these allocations.

regards,
damn carpenter