* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-06 14:07 ` [Intel-wired-lan] " Alexander Lobakin
@ 2025-11-06 16:05 ` ally heev
2025-11-07 5:39 ` Przemek Kitszel
2025-11-08 16:36 ` Simon Horman
2025-11-17 13:11 ` Dan Carpenter
2 siblings, 1 reply; 11+ messages in thread
From: ally heev @ 2025-11-06 16:05 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Aleksandr Loktionov,
intel-wired-lan, netdev, linux-kernel, linux-hyperv,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-06 16:05 ` ally heev
@ 2025-11-07 5:39 ` Przemek Kitszel
2025-11-07 7:27 ` ally heev
0 siblings, 1 reply; 11+ messages in thread
From: Przemek Kitszel @ 2025-11-07 5:39 UTC (permalink / raw)
To: ally heev, Alexander Lobakin
Cc: Tony Nguyen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Aleksandr Loktionov, intel-wired-lan, netdev,
linux-kernel, linux-hyperv, Dan Carpenter
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-07 5:39 ` Przemek Kitszel
@ 2025-11-07 7:27 ` ally heev
0 siblings, 0 replies; 11+ messages in thread
From: ally heev @ 2025-11-07 7:27 UTC (permalink / raw)
To: Przemek Kitszel, Alexander Lobakin
Cc: Tony Nguyen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Aleksandr Loktionov, intel-wired-lan, netdev,
linux-kernel, linux-hyperv, Dan Carpenter
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-06 14:07 ` [Intel-wired-lan] " Alexander Lobakin
2025-11-06 16:05 ` ally heev
@ 2025-11-08 16:36 ` Simon Horman
2025-11-10 6:56 ` ally heev
2025-11-17 13:11 ` Dan Carpenter
2 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2025-11-08 16:36 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Ally Heev, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv, Dan Carpenter
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
...
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-08 16:36 ` Simon Horman
@ 2025-11-10 6:56 ` ally heev
0 siblings, 0 replies; 11+ messages in thread
From: ally heev @ 2025-11-10 6:56 UTC (permalink / raw)
To: Simon Horman, Alexander Lobakin
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Aleksandr Loktionov,
intel-wired-lan, netdev, linux-kernel, linux-hyperv,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-06 14:07 ` [Intel-wired-lan] " Alexander Lobakin
2025-11-06 16:05 ` ally heev
2025-11-08 16:36 ` Simon Horman
@ 2025-11-17 13:11 ` Dan Carpenter
2025-11-17 14:37 ` Alexander Lobakin
2 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2025-11-17 13:11 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Ally Heev, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-17 13:11 ` Dan Carpenter
@ 2025-11-17 14:37 ` Alexander Lobakin
2025-11-17 18:11 ` Dan Carpenter
0 siblings, 1 reply; 11+ messages in thread
From: Alexander Lobakin @ 2025-11-17 14:37 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ally Heev, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-17 14:37 ` Alexander Lobakin
@ 2025-11-17 18:11 ` Dan Carpenter
2025-11-19 11:25 ` Alexander Lobakin
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2025-11-17 18:11 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Ally Heev, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
2025-11-17 18:11 ` Dan Carpenter
@ 2025-11-19 11:25 ` Alexander Lobakin
0 siblings, 0 replies; 11+ messages in thread
From: Alexander Lobakin @ 2025-11-19 11:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ally Heev, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv
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
^ permalink raw reply [flat|nested] 11+ messages in thread