drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++-- drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)
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.
It is better to initialize and assign pointers with `__free`
attribute in one statement to ensure proper scope-based cleanup.
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
Changes in v3:
- fixed style issues
- Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
Changes in v2:
- fixed non-pointer initialization to NULL
- NOTE: drop v1
- Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
---
drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++--
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
--- a/drivers/net/ethernet/intel/ice/ice_flow.c
+++ b/drivers/net/ethernet/intel/ice/ice_flow.c
@@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
struct ice_parser_profile *prof, enum ice_block blk)
{
u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
- struct ice_flow_prof_params *params __free(kfree);
u8 fv_words = hw->blk[blk].es.fvw;
int status;
int i, idx;
- params = kzalloc(sizeof(*params), GFP_KERNEL);
+ struct ice_flow_prof_params *params __free(kfree) =
+ kzalloc(sizeof(*params), GFP_KERNEL);
+
if (!params)
return -ENOMEM;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
*/
static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
{
- struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
struct idpf_vc_xn_params xn_params = {
.vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
@@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
ssize_t reply_sz;
int err = 0;
- rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+ struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
+ kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+
if (!rcvd_regions)
return -ENOMEM;
---
base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
Best regards,
--
Ally Heev <allyheev@gmail.com>From: Ally Heev <allyheev@gmail.com>
Date: Thu, 06 Nov 2025 17:25:48 +0530
> 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.
>
> It is better to initialize and assign pointers with `__free`
> attribute in one statement to ensure proper scope-based cleanup.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> Changes in v3:
> - fixed style issues
> - Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
>
> Changes in v2:
> - fixed non-pointer initialization to NULL
> - NOTE: drop v1
> - Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
> ---
> drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++--
> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> struct ice_parser_profile *prof, enum ice_block blk)
> {
> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> - struct ice_flow_prof_params *params __free(kfree);
> u8 fv_words = hw->blk[blk].es.fvw;
> int status;
> int i, idx;
>
> - params = kzalloc(sizeof(*params), GFP_KERNEL);
> + struct ice_flow_prof_params *params __free(kfree) =
> + kzalloc(sizeof(*params), GFP_KERNEL);
Please don't do it that way. It's not C++ with RAII and
declare-where-you-use.
Just leave the variable declarations where they are, but initialize them
with `= NULL`.
Variable declarations must be in one block and sorted from the longest
to the shortest.
But most important, I'm not even sure how you could trigger an
"undefined behaviour" here. Both here and below the variable tagged with
`__free` is initialized right after the declaration block, before any
return. So how to trigger an UB here?
> +
> if (!params)
> return -ENOMEM;
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
> */
> static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> {
> - struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
> struct idpf_vc_xn_params xn_params = {
> .vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
> .recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> ssize_t reply_sz;
> int err = 0;
>
> - rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> + struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> + kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> +
> if (!rcvd_regions)
> return -ENOMEM;
Same here, @rcvd_regions is initialized before the very first return, no
idea how one can provoke an UB here.
>
>
> ---
> base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
>
> Best regards,
Thanks,
Olek
On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
> > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > struct ice_parser_profile *prof, enum ice_block blk)
> > {
> > u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > - struct ice_flow_prof_params *params __free(kfree);
> > u8 fv_words = hw->blk[blk].es.fvw;
> > int status;
> > int i, idx;
> >
> > - params = kzalloc(sizeof(*params), GFP_KERNEL);
> > + struct ice_flow_prof_params *params __free(kfree) =
> > + kzalloc(sizeof(*params), GFP_KERNEL);
>
> Please don't do it that way. It's not C++ with RAII and
> declare-where-you-use.
> Just leave the variable declarations where they are, but initialize them
> with `= NULL`.
>
> Variable declarations must be in one block and sorted from the longest
> to the shortest.
>
These days, with __free the trend is to say yes this is RAII and we
should declare it where you use it. I personally don't have a strong
opinion on this either way, but other maintainers do and will NAK the
`= NULL` approach.
The documentation says you should do it that way and avoid the `= NULL`
as well. The issue is with lock ordering. It's a FILO ordering, so if
we require a specific unlock order then declaring variables at the top
could mess things up.
The counter argument is that if you declare a variable after a goto
then that's undefined behavior as well. Clang will detect that bug so
it be detected before it hits actual users.
regards,
dan carpenter
From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Mon, 17 Nov 2025 16:11:32 +0300
> On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>>> struct ice_parser_profile *prof, enum ice_block blk)
>>> {
>>> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
>>> - struct ice_flow_prof_params *params __free(kfree);
>>> u8 fv_words = hw->blk[blk].es.fvw;
>>> int status;
>>> int i, idx;
>>>
>>> - params = kzalloc(sizeof(*params), GFP_KERNEL);
>>> + struct ice_flow_prof_params *params __free(kfree) =
>>> + kzalloc(sizeof(*params), GFP_KERNEL);
>>
>> Please don't do it that way. It's not C++ with RAII and
>> declare-where-you-use.
>> Just leave the variable declarations where they are, but initialize them
>> with `= NULL`.
>>
>> Variable declarations must be in one block and sorted from the longest
>> to the shortest.
>>
>
> These days, with __free the trend is to say yes this is RAII and we
> should declare it where you use it. I personally don't have a strong
Sorta, but we can't "declare it where you use it" since we don't allow
declaration-after-statement in the kernel.
> opinion on this either way, but other maintainers do and will NAK the
> `= NULL` approach.
>
> The documentation says you should do it that way and avoid the `= NULL`
> as well. The issue is with lock ordering. It's a FILO ordering, so if
> we require a specific unlock order then declaring variables at the top
> could mess things up.
>
> The counter argument is that if you declare a variable after a goto
> then that's undefined behavior as well. Clang will detect that bug so
> it be detected before it hits actual users.
>
> regards,
> dan carpenter
Thanks,
Olek
On Mon, Nov 17, 2025 at 03:37:30PM +0100, Alexander Lobakin wrote:
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Date: Mon, 17 Nov 2025 16:11:32 +0300
>
> > On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> >>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> >>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> >>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> >>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> >>> struct ice_parser_profile *prof, enum ice_block blk)
> >>> {
> >>> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> >>> - struct ice_flow_prof_params *params __free(kfree);
> >>> u8 fv_words = hw->blk[blk].es.fvw;
> >>> int status;
> >>> int i, idx;
> >>>
> >>> - params = kzalloc(sizeof(*params), GFP_KERNEL);
> >>> + struct ice_flow_prof_params *params __free(kfree) =
> >>> + kzalloc(sizeof(*params), GFP_KERNEL);
> >>
> >> Please don't do it that way. It's not C++ with RAII and
> >> declare-where-you-use.
> >> Just leave the variable declarations where they are, but initialize them
> >> with `= NULL`.
> >>
> >> Variable declarations must be in one block and sorted from the longest
> >> to the shortest.
> >>
> >
> > These days, with __free the trend is to say yes this is RAII and we
> > should declare it where you use it. I personally don't have a strong
>
> Sorta, but we can't "declare it where you use it" since we don't allow
> declaration-after-statement in the kernel.
That changed when we merged cleanup.h. It is allowed now. I still don't
like to declare variables anywhere unless it's a __free() variable and I
think almost everyone else agrees. The only subsystem which I know that
completely moved to declaring variables willy-nilly was bcachefs.
regards,
dan carpenter
From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Mon, 17 Nov 2025 21:11:44 +0300
> On Mon, Nov 17, 2025 at 03:37:30PM +0100, Alexander Lobakin wrote:
>> From: Dan Carpenter <dan.carpenter@linaro.org>
>> Date: Mon, 17 Nov 2025 16:11:32 +0300
>>
>>> On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
>>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>>>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
>>>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>>>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>>>>> struct ice_parser_profile *prof, enum ice_block blk)
>>>>> {
>>>>> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
>>>>> - struct ice_flow_prof_params *params __free(kfree);
>>>>> u8 fv_words = hw->blk[blk].es.fvw;
>>>>> int status;
>>>>> int i, idx;
>>>>>
>>>>> - params = kzalloc(sizeof(*params), GFP_KERNEL);
>>>>> + struct ice_flow_prof_params *params __free(kfree) =
>>>>> + kzalloc(sizeof(*params), GFP_KERNEL);
>>>>
>>>> Please don't do it that way. It's not C++ with RAII and
>>>> declare-where-you-use.
>>>> Just leave the variable declarations where they are, but initialize them
>>>> with `= NULL`.
>>>>
>>>> Variable declarations must be in one block and sorted from the longest
>>>> to the shortest.
>>>>
>>>
>>> These days, with __free the trend is to say yes this is RAII and we
>>> should declare it where you use it. I personally don't have a strong
>>
>> Sorta, but we can't "declare it where you use it" since we don't allow
>> declaration-after-statement in the kernel.
>
> That changed when we merged cleanup.h. It is allowed now. I still don't
> like to declare variables anywhere unless it's a __free() variable and I
> think almost everyone else agrees. The only subsystem which I know that
> completely moved to declaring variables willy-nilly was bcachefs.
Oops, seems like I completely missed that.
So does it mean we're now allowed to write code in C++ style:
int a = func_a();
func_b();
int c = func_c();
?
Anyway, I have the same preferences as you: to declare everything at the
top of the function (or the scope if it's a loop/if/whatever). And
didn't plan to change this :D
>
> regards,
> dan carpenter
Thanks,
Olek
On Thu, Nov 06, 2025 at 03:07:26PM +0100, Alexander Lobakin wrote:
> From: Ally Heev <allyheev@gmail.com>
> Date: Thu, 06 Nov 2025 17:25:48 +0530
>
> > 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.
> >
> > It is better to initialize and assign pointers with `__free`
> > attribute in one statement to ensure proper scope-based cleanup.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> > Signed-off-by: Ally Heev <allyheev@gmail.com>
> > ---
> > Changes in v3:
> > - fixed style issues
> > - Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
> >
> > Changes in v2:
> > - fixed non-pointer initialization to NULL
> > - NOTE: drop v1
> > - Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
> > ---
> > drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++--
> > drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > struct ice_parser_profile *prof, enum ice_block blk)
> > {
> > u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > - struct ice_flow_prof_params *params __free(kfree);
> > u8 fv_words = hw->blk[blk].es.fvw;
> > int status;
> > int i, idx;
> >
> > - params = kzalloc(sizeof(*params), GFP_KERNEL);
> > + struct ice_flow_prof_params *params __free(kfree) =
> > + kzalloc(sizeof(*params), GFP_KERNEL);
>
> Please don't do it that way. It's not C++ with RAII and
> declare-where-you-use.
> Just leave the variable declarations where they are, but initialize them
> with `= NULL`.
>
> Variable declarations must be in one block and sorted from the longest
> to the shortest.
>
> But most important, I'm not even sure how you could trigger an
> "undefined behaviour" here. Both here and below the variable tagged with
> `__free` is initialized right after the declaration block, before any
> return. So how to trigger an UB here?
FWIIW, I'd prefer if we sidestepped this discussion entirely
by not using __free [1] in this driver.
It seems to me that for both functions updated by this
patch that can easily be achieved using an idiomatic
goto label to free on error.
[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
...
On Sat, 2025-11-08 at 16:36 +0000, Simon Horman wrote: [..] > > Please don't do it that way. It's not C++ with RAII and > > declare-where-you-use. > > Just leave the variable declarations where they are, but initialize them > > with `= NULL`. > > > > Variable declarations must be in one block and sorted from the longest > > to the shortest. > > > > But most important, I'm not even sure how you could trigger an > > "undefined behaviour" here. Both here and below the variable tagged with > > `__free` is initialized right after the declaration block, before any > > return. So how to trigger an UB here? > > FWIIW, I'd prefer if we sidestepped this discussion entirely > by not using __free [1] in this driver. > > It seems to me that for both functions updated by this > patch that can easily be achieved using an idiomatic > goto label to free on error. > > [1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs > > ... Understood. I will come-up with a new patch series for removing these two instances Regards, Ally
On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
[..]
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > struct ice_parser_profile *prof, enum ice_block blk)
> > {
> > u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > - struct ice_flow_prof_params *params __free(kfree);
> > u8 fv_words = hw->blk[blk].es.fvw;
> > int status;
> > int i, idx;
> >
> > - params = kzalloc(sizeof(*params), GFP_KERNEL);
> > + struct ice_flow_prof_params *params __free(kfree) =
> > + kzalloc(sizeof(*params), GFP_KERNEL);
>
> Please don't do it that way. It's not C++ with RAII and
> declare-where-you-use.
> Just leave the variable declarations where they are, but initialize them
> with `= NULL`.
>
> Variable declarations must be in one block and sorted from the longest
> to the shortest.
>
> But most important, I'm not even sure how you could trigger an
> "undefined behaviour" here. Both here and below the variable tagged with
> `__free` is initialized right after the declaration block, before any
> return. So how to trigger an UB here?
It doesn't occur here. But, many maintainers/developers consider it a
bad practice because if the function returns before initialization or
use of `goto` can cause such behaviors.
Here though, the definitions are still at the top right? Maybe I could
just sort them
>
> > +
> > if (!params)
> > return -ENOMEM;
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
> > */
> > static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> > {
> > - struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
> > struct idpf_vc_xn_params xn_params = {
> > .vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
> > .recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> > @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> > ssize_t reply_sz;
> > int err = 0;
> >
> > - rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > + struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> > + kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > +
> > if (!rcvd_regions)
> > return -ENOMEM;
>
> Same here, @rcvd_regions is initialized before the very first return, no
> idea how one can provoke an UB here.
>
> >
> >
> > ---
> > base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> > change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
> >
> > Best regards,
>
> Thanks,
> Olek
Regards,
Ally
On 11/6/25 17:05, ally heev wrote:
> On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> [..]
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>>> struct ice_parser_profile *prof, enum ice_block blk)
>>> {
>>> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
>>> - struct ice_flow_prof_params *params __free(kfree);
>>> u8 fv_words = hw->blk[blk].es.fvw;
>>> int status;
>>> int i, idx;
>>>
>>> - params = kzalloc(sizeof(*params), GFP_KERNEL);
>>> + struct ice_flow_prof_params *params __free(kfree) =
>>> + kzalloc(sizeof(*params), GFP_KERNEL);
>>
>> Please don't do it that way. It's not C++ with RAII and
>> declare-where-you-use.
>> Just leave the variable declarations where they are, but initialize them
>> with `= NULL`.
+1
>>
>> Variable declarations must be in one block and sorted from the longest
>> to the shortest.
>>
>> But most important, I'm not even sure how you could trigger an
>> "undefined behaviour" here. Both here and below the variable tagged with
>> `__free` is initialized right after the declaration block, before any
>> return. So how to trigger an UB here?
>
> It doesn't occur here. But, many maintainers/developers consider it a
> bad practice because if the function returns before initialization or
> use of `goto` can cause such behaviors.
we were bitten by that already, scenario is as follow:
0. have a good code w/o UB and w/o redundant = NULL
1. add some early return, say:
if (dest_vsi == fdir_vsi)
return -EINVAL;
2. almost granted that person adding 1. will forget to add = NULL to all
declarations marked __free
>
> Here though, the definitions are still at the top right? Maybe I could
> just sort them
we discourage putting any operations, including allocations, that may
fail into the declarations block
On Fri, 2025-11-07 at 06:39 +0100, Przemek Kitszel wrote:
> On 11/6/25 17:05, ally heev wrote:
> > On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> > [..]
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > > > struct ice_parser_profile *prof, enum ice_block blk)
> > > > {
> > > > u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > > > - struct ice_flow_prof_params *params __free(kfree);
> > > > u8 fv_words = hw->blk[blk].es.fvw;
> > > > int status;
> > > > int i, idx;
> > > >
> > > > - params = kzalloc(sizeof(*params), GFP_KERNEL);
> > > > + struct ice_flow_prof_params *params __free(kfree) =
> > > > + kzalloc(sizeof(*params), GFP_KERNEL);
> > >
> > > Please don't do it that way. It's not C++ with RAII and
> > > declare-where-you-use.
> > > Just leave the variable declarations where they are, but initialize them
> > > with `= NULL`.
>
> +1
>
> > >
> > > Variable declarations must be in one block and sorted from the longest
> > > to the shortest.
> > >
> > > But most important, I'm not even sure how you could trigger an
> > > "undefined behaviour" here. Both here and below the variable tagged with
> > > `__free` is initialized right after the declaration block, before any
> > > return. So how to trigger an UB here?
> >
> > It doesn't occur here. But, many maintainers/developers consider it a
> > bad practice because if the function returns before initialization or
> > use of `goto` can cause such behaviors.
>
> we were bitten by that already, scenario is as follow:
> 0. have a good code w/o UB and w/o redundant = NULL
> 1. add some early return, say:
> if (dest_vsi == fdir_vsi)
> return -EINVAL;
> 2. almost granted that person adding 1. will forget to add = NULL to all
> declarations marked __free
>
> >
> > Here though, the definitions are still at the top right? Maybe I could
> > just sort them
>
> we discourage putting any operations, including allocations, that may
> fail into the declarations block
>
Makes sense. I will just initialize them with NULL then
© 2016 - 2025 Red Hat, Inc.