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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 3/21/24 04:29, Jakub Kicinski wrote:
> On Wed, 20 Mar 2024 08:01:49 +0300 Dan Carpenter wrote:
>>> This is just trading one kind of bug for another, and the __free()
>>> magic is at a cost of readability.

Apologies for not catching it during review.
It's good that we have started small, with just a few functions.

>>>
>>> I think we should ban the use of __free() in all of networking,
>>> until / unless it cleanly handles the NULL init case.

Current API is indeed asking for bugs, especially when combined with RCT
and early error checking rules. Perhaps that's why there is double
underscore prefix ;)

Simplest solution would be to add a macro wrapper, especially that there
are only a few deallocation methods.

in cleanup.h:
+#define auto_kfree __free(kfree) = NULL

and similar macros for auto vfree(), etc.

then in the drivers:
-struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL,
				  *othercaps __free(kfree) = NULL;
+struct ice_aqc_get_phy_caps_data *pcaps auto_kfree,
				  *othercaps auto_kfree;

With that only developers introducing new allocators/wrappers would be
using bare __free(), the rest of us will be free to focus on other
things.
One could argue (+CC David Laight) that additional zero-init would not
be wiped out by compiler, but that is a price I would happily pay in
almost all cases.

I have no idea if someone already proposed exactly that, as this is
almost obvious solution.

>>
>> 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.
> 
> What I was trying to say is that the __free() thing is supposed to
> prevent bugs, and it's not. Even if it was easy to write the matcher
> rule, if __free() needs a rule to double check its use - it's failing
> at making it easier to write correct code.
> 
> In any case. This is a patch for Intel wired, I'll let Intel folks
> decide.