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

From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
To: Julia Lawall <julia.lawall@inria.fr>,
	Dan Carpenter <dan.carpenter@linaro.org>
Cc: Jakub Kicinski <kuba@kernel.org>,
	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>,
	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,
	Jonathan Cameron <jic23@kernel.org>
Subject: Re: [PATCH net] ice: Fix freeing uninitialized pointers
Date: Wed, 20 Mar 2024 16:33:02 +0000	[thread overview]
Message-ID: <10F403F7-E8B7-48F0-90CF-3C8A8BEB10F2@jic23.retrosnub.co.uk> (raw)
In-Reply-To: <f1bdbed9-8549-3787-bd17-ecd62851e8a@inria.fr>

On 20 March 2024 07:32:17 GMT, Julia Lawall <julia.lawall@inria.fr> wrote:
>
>
>On Wed, 20 Mar 2024, Dan Carpenter wrote:
>
>> On Tue, Mar 19, 2024 at 12:43:17PM -0700, Jakub Kicinski wrote:
>> > On Sat, 16 Mar 2024 12:44:40 +0300 Dan Carpenter wrote:
>> > > -	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>> > > -	void *mac_buf __free(kfree);
>> > > +	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
>> > > +	void *mac_buf __free(kfree) = NULL;
>> >
>> > This is just trading one kind of bug for another, and the __free()
>> > magic is at a cost of readability.
>> >
>> > I think we should ban the use of __free() in all of networking,
>> > until / unless it cleanly handles the NULL init case.
>>
>> Free handles the NULL init case, it doesn't handle the uninitialized
>> case.  I had previously argued that checkpatch should complain about
>> every __free() pointer if the declaration doesn't have an assignment.
>>
>> The = NULL assignment is unnecessary if the pointer is assigned to
>> something else before the first return, so this might cause "unused
>> assignment" warnings?  I don't know if there are any tools which
>> complain about that in that situation.  I think probably we should just
>> make that an exception and do the checkpatch thing because it's such a
>> simple rule to implement.
>
>My understanding from Jonathan Cameron was that Linus wants a NULL always,
>unless there is an initialization with the declaration.

I don't have thread to hand but Linus strongly preferred moving any declaration using this to
 where it is assigned so that it was obvious that the allocator and freer match.

Not checked if that makes sense here though 
>
>julia