[RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free

Ally Heev posted 2 patches 2 months, 2 weeks ago
drivers/net/ethernet/intel/ice/ice_flow.c       |  6 ++++--
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 28 +++++++++++++++++--------
2 files changed, 23 insertions(+), 11 deletions(-)
[RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by Ally Heev 2 months, 2 weeks ago
Uninitialized pointers with `__free` attribute can cause undefined
behavior as the memory assigned randomly to the pointer is freed
automatically when the pointer goes out of scope.

We could just fix it by initializing the pointer to NULL, but, as usage of
cleanup attributes is discouraged in net [1], trying to achieve cleanup
using goto

[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Signed-off-by: Ally Heev <allyheev@gmail.com>
---
Ally Heev (2):
      ice: remove __free usage in ice_flow
      idpf: remove __free usage in idpf_virtchnl

 drivers/net/ethernet/intel/ice/ice_flow.c       |  6 ++++--
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 28 +++++++++++++++++--------
 2 files changed, 23 insertions(+), 11 deletions(-)
---
base-commit: 24598358a1b4ca1d596b8e7b34a7bc76f54e630f
change-id: 20251113-aheev-fix-free-uninitialized-ptrs-ethernet-intel-abc0cc9278d8

Best regards,
-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQQBFRpOLrIakF7DYvaWPaLUP9d7HAUCaRn0WAAKCRCWPaLUP9d7
HPCSAP4tu8ld+4Og65tjSYNChRqIR4Gn8C546JFeozyQW6uj3wD/SQEPIidSAYbb
klXrZrKIBOc/avt55S2+krl241aNJA8=
=guHM
-----END PGP SIGNATURE-----
-- 
Ally Heev <allyheev@gmail.com>
Re: [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by Tony Nguyen 2 months, 1 week ago

On 11/23/2025 11:40 PM, Ally Heev wrote:
> Uninitialized pointers with `__free` attribute can cause undefined
> behavior as the memory assigned randomly to the pointer is freed
> automatically when the pointer goes out of scope.
> 
> We could just fix it by initializing the pointer to NULL, but, as usage of
> cleanup attributes is discouraged in net [1], trying to achieve cleanup
> using goto

These two drivers already have multiple other usages of this. All the 
other instances initialize to NULL; I'd prefer to see this do the same 
over changing this single instance.

Thanks,
Tony

> [1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
> 
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> Ally Heev (2):
>        ice: remove __free usage in ice_flow
>        idpf: remove __free usage in idpf_virtchnl
> 
>   drivers/net/ethernet/intel/ice/ice_flow.c       |  6 ++++--
>   drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 28 +++++++++++++++++--------
>   2 files changed, 23 insertions(+), 11 deletions(-)
> ---
> base-commit: 24598358a1b4ca1d596b8e7b34a7bc76f54e630f
> change-id: 20251113-aheev-fix-free-uninitialized-ptrs-ethernet-intel-abc0cc9278d8
> 
> Best regards,
> -----BEGIN PGP SIGNATURE-----
> 
> iHUEABYKAB0WIQQBFRpOLrIakF7DYvaWPaLUP9d7HAUCaRn0WAAKCRCWPaLUP9d7
> HPCSAP4tu8ld+4Og65tjSYNChRqIR4Gn8C546JFeozyQW6uj3wD/SQEPIidSAYbb
> klXrZrKIBOc/avt55S2+krl241aNJA8=
> =guHM
> -----END PGP SIGNATURE-----
Re: [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by ally heev 2 months, 1 week ago
On Mon, 2025-12-01 at 13:40 -0800, Tony Nguyen wrote:
> 
> On 11/23/2025 11:40 PM, Ally Heev wrote:
> > Uninitialized pointers with `__free` attribute can cause undefined
> > behavior as the memory assigned randomly to the pointer is freed
> > automatically when the pointer goes out of scope.
> > 
> > We could just fix it by initializing the pointer to NULL, but, as usage of
> > cleanup attributes is discouraged in net [1], trying to achieve cleanup
> > using goto
> 
> These two drivers already have multiple other usages of this. All the 
> other instances initialize to NULL; I'd prefer to see this do the same 
> over changing this single instance.
> 

Other usages are slightly complicated to be refactored and might need
good testing. Do you want me to do it in a different series?


Regards,
Ally
Re: [Intel-wired-lan] [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by Tony Nguyen 2 months, 1 week ago

On 12/2/2025 11:47 AM, ally heev wrote:
> On Mon, 2025-12-01 at 13:40 -0800, Tony Nguyen wrote:
>>
>> On 11/23/2025 11:40 PM, Ally Heev wrote:
>>> Uninitialized pointers with `__free` attribute can cause undefined
>>> behavior as the memory assigned randomly to the pointer is freed
>>> automatically when the pointer goes out of scope.
>>>
>>> We could just fix it by initializing the pointer to NULL, but, as usage of
>>> cleanup attributes is discouraged in net [1], trying to achieve cleanup
>>> using goto
>>
>> These two drivers already have multiple other usages of this. All the
>> other instances initialize to NULL; I'd prefer to see this do the same
>> over changing this single instance.
>>
> 
> Other usages are slightly complicated to be refactored and might need
> good testing. Do you want me to do it in a different series?

Hi Ally,

Sorry, I think I was unclear. I'd prefer these two initialized to NULL, 
to match the other usages, over removing the __free() from them.

Thanks,
Tony

> Regards,
> Ally
Re: [Intel-wired-lan] [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by ally heev 2 months, 1 week ago
On Tue, 2025-12-02 at 10:17 -0800, Tony Nguyen wrote:
> 
> On 12/2/2025 11:47 AM, ally heev wrote:
> > On Mon, 2025-12-01 at 13:40 -0800, Tony Nguyen wrote:
> > > 
> > > On 11/23/2025 11:40 PM, Ally Heev wrote:
> > > > Uninitialized pointers with `__free` attribute can cause undefined
> > > > behavior as the memory assigned randomly to the pointer is freed
> > > > automatically when the pointer goes out of scope.
> > > > 
> > > > We could just fix it by initializing the pointer to NULL, but, as usage of
> > > > cleanup attributes is discouraged in net [1], trying to achieve cleanup
> > > > using goto
> > > 
> > > These two drivers already have multiple other usages of this. All the
> > > other instances initialize to NULL; I'd prefer to see this do the same
> > > over changing this single instance.
> > > 
> > 
> > Other usages are slightly complicated to be refactored and might need
> > good testing. Do you want me to do it in a different series?
> 
> Hi Ally,
> 
> Sorry, I think I was unclear. I'd prefer these two initialized to NULL, 
> to match the other usages, over removing the __free() from them.

I had a patch for that already, but, isn't using __free discouraged in
networking drivers [1]? Simon was against it [2]

[2] https://lore.kernel.org/all/aQ9xp9pchMwml30P@horms.kernel.org/
[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Regards,
Ally
Re: [Intel-wired-lan] [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by Przemek Kitszel 2 months, 1 week ago
On 12/3/25 09:09, ally heev wrote:
> On Tue, 2025-12-02 at 10:17 -0800, Tony Nguyen wrote:
>>
>> On 12/2/2025 11:47 AM, ally heev wrote:
>>> On Mon, 2025-12-01 at 13:40 -0800, Tony Nguyen wrote:
>>>>
>>>> On 11/23/2025 11:40 PM, Ally Heev wrote:
>>>>> Uninitialized pointers with `__free` attribute can cause undefined
>>>>> behavior as the memory assigned randomly to the pointer is freed
>>>>> automatically when the pointer goes out of scope.
>>>>>
>>>>> We could just fix it by initializing the pointer to NULL, but, as usage of
>>>>> cleanup attributes is discouraged in net [1], trying to achieve cleanup
>>>>> using goto
>>>>
>>>> These two drivers already have multiple other usages of this. All the
>>>> other instances initialize to NULL; I'd prefer to see this do the same
>>>> over changing this single instance.
>>>>
>>>
>>> Other usages are slightly complicated to be refactored and might need
>>> good testing. Do you want me to do it in a different series?
>>
>> Hi Ally,
>>
>> Sorry, I think I was unclear. I'd prefer these two initialized to NULL,
>> to match the other usages, over removing the __free() from them.
> 
> I had a patch for that already, but, isn't using __free discouraged in
> networking drivers [1]? Simon was against it [2]

you see, the construct is discouraged, so we don't use it everywhere,
but cleaning up just a little would not change the state of the matter
(IOW we will still be in "driver has some __free() usage" state).

TBH, I would not spent my time "undoing" all of the __free() that we
have already, especially the testing part sounds not fun.

Turning all usage points to "= NULL" is orthogonal, and would be great.

> 
> [2] https://lore.kernel.org/all/aQ9xp9pchMwml30P@horms.kernel.org/
> [1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
> 
> Regards,
> Ally
>
Re: [Intel-wired-lan] [RFT net-next PATCH RESEND 0/2] ethernet: intel: fix freeing uninitialized pointers with __free
Posted by ally heev 2 months ago
On Wed, 2025-12-03 at 09:45 +0100, Przemek Kitszel wrote:
> On 12/3/25 09:09, ally heev wrote:
> > On Tue, 2025-12-02 at 10:17 -0800, Tony Nguyen wrote:
> > > 
> > > On 12/2/2025 11:47 AM, ally heev wrote:
> > > > On Mon, 2025-12-01 at 13:40 -0800, Tony Nguyen wrote:
> > > > > 
> > > > > On 11/23/2025 11:40 PM, Ally Heev wrote:
> > > > > > Uninitialized pointers with `__free` attribute can cause undefined
> > > > > > behavior as the memory assigned randomly to the pointer is freed
> > > > > > automatically when the pointer goes out of scope.
> > > > > > 
> > > > > > We could just fix it by initializing the pointer to NULL, but, as usage of
> > > > > > cleanup attributes is discouraged in net [1], trying to achieve cleanup
> > > > > > using goto
> > > > > 
> > > > > These two drivers already have multiple other usages of this. All the
> > > > > other instances initialize to NULL; I'd prefer to see this do the same
> > > > > over changing this single instance.
> > > > > 
> > > > 
> > > > Other usages are slightly complicated to be refactored and might need
> > > > good testing. Do you want me to do it in a different series?
> > > 
> > > Hi Ally,
> > > 
> > > Sorry, I think I was unclear. I'd prefer these two initialized to NULL,
> > > to match the other usages, over removing the __free() from them.
> > 
> > I had a patch for that already, but, isn't using __free discouraged in
> > networking drivers [1]? Simon was against it [2]
> 
> you see, the construct is discouraged, so we don't use it everywhere,
> but cleaning up just a little would not change the state of the matter
> (IOW we will still be in "driver has some __free() usage" state).
> 

But still we can just fix the uninitialized ones the right way [1]
right? since we have to fix them anyway. There already a patch [2] for
that

[1]
https://lore.kernel.org/lkml/CAHk-=wiCOTW5UftUrAnvJkr6769D29tF7Of79gUjdQHS_TkF5A@mail.gmail.com/
[2]
https://lore.kernel.org/all/20251106-aheev-uninitialized-free-attr-net-ethernet-v3-1-ef2220f4f476@gmail.com/

> TBH, I would not spent my time "undoing" all of the __free() that we
> have already, especially the testing part sounds not fun.

+1

> 
> Turning all usage points to "= NULL" is orthogonal, and would be great.
> 
> > 
> > [2] https://lore.kernel.org/all/aQ9xp9pchMwml30P@horms.kernel.org/
> > [1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
> > 
> > Regards,
> > Ally
> >