diff mbox series

[net] ice: Fix freeing uninitialized pointers

Message ID 77145930-e3df-4e77-a22d-04851cf3a426@moroto.mountain (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] ice: Fix freeing uninitialized pointers | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 939 this patch: 939
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 956 this patch: 956
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 956 this patch: 956
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-03-19--18-00 (tests: 907)

Commit Message

Dan Carpenter March 16, 2024, 9:44 a.m. UTC
Automatically cleaned up pointers need to be initialized before exiting
their scope.  In this case, they need to be initialized to NULL before
any return statement.

Fixes: 90f821d72e11 ("ice: avoid unnecessary devm_ usage")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/net/ethernet/intel/ice/ice_common.c  | 4 ++--
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Jiri Pirko March 18, 2024, 7:58 a.m. UTC | #1
Sat, Mar 16, 2024 at 10:44:40AM CET, dan.carpenter@linaro.org wrote:
>Automatically cleaned up pointers need to be initialized before exiting
>their scope.  In this case, they need to be initialized to NULL before
>any return statement.
>
>Fixes: 90f821d72e11 ("ice: avoid unnecessary devm_ usage")
>Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>---
> drivers/net/ethernet/intel/ice/ice_common.c  | 4 ++--
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
>index 4d8111aeb0ff..4b27d2bc2912 100644
>--- a/drivers/net/ethernet/intel/ice/ice_common.c
>+++ b/drivers/net/ethernet/intel/ice/ice_common.c
>@@ -1002,8 +1002,8 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw)
>  */
> int ice_init_hw(struct ice_hw *hw)
> {
>-	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;
> 	u16 mac_buf_len;
> 	int status;
> 

How about similar issues in:
ice_set_fc()
ice_cfg_phy_fec()
?


>diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>index 255a9c8151b4..78b833b3e1d7 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>@@ -941,11 +941,11 @@ static u64 ice_loopback_test(struct net_device *netdev)
> 	struct ice_netdev_priv *np = netdev_priv(netdev);
> 	struct ice_vsi *orig_vsi = np->vsi, *test_vsi;
> 	struct ice_pf *pf = orig_vsi->back;
>+	u8 *tx_frame __free(kfree) = NULL;
> 	u8 broadcast[ETH_ALEN], ret = 0;
> 	int num_frames, valid_frames;
> 	struct ice_tx_ring *tx_ring;
> 	struct ice_rx_ring *rx_ring;
>-	u8 *tx_frame __free(kfree);
> 	int i;
> 
> 	netdev_info(netdev, "loopback test\n");
>-- 
>2.43.0
>
>
Dan Carpenter March 18, 2024, 8:10 a.m. UTC | #2
On Mon, Mar 18, 2024 at 08:58:24AM +0100, Jiri Pirko wrote:
> Sat, Mar 16, 2024 at 10:44:40AM CET, dan.carpenter@linaro.org wrote:
> >Automatically cleaned up pointers need to be initialized before exiting
> >their scope.  In this case, they need to be initialized to NULL before
> >any return statement.
> >
> >Fixes: 90f821d72e11 ("ice: avoid unnecessary devm_ usage")
> >Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >---
> > drivers/net/ethernet/intel/ice/ice_common.c  | 4 ++--
> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> >index 4d8111aeb0ff..4b27d2bc2912 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_common.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_common.c
> >@@ -1002,8 +1002,8 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw)
> >  */
> > int ice_init_hw(struct ice_hw *hw)
> > {
> >-	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;
> > 	u16 mac_buf_len;
> > 	int status;
> > 
> 
> How about similar issues in:
> ice_set_fc()
> ice_cfg_phy_fec()
> ?

Yeah.  Sorry, I'll resend.  Smatch didn't warn about those bugs because
the sanity checks are the begining of the functions:

	if (!pi || !aq_failures)
		return -EINVAL;

are never true...  It's the first time I've run into this as an issue.

regards,
dan carpenter
Jakub Kicinski March 19, 2024, 7:43 p.m. UTC | #3
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.
Dan Carpenter March 20, 2024, 5:01 a.m. UTC | #4
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.

regards,
dan carpenter
Julia Lawall March 20, 2024, 7:32 a.m. UTC | #5
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.

julia
Markus Elfring March 20, 2024, 12:18 p.m. UTC | #6
> Automatically cleaned up pointers need to be initialized before exiting
> their scope.  In this case, they need to be initialized to NULL before
> any return statement.

I suggest to reconsider such information a bit more.


…
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1002,8 +1002,8 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw)
>   */
>  int ice_init_hw(struct ice_hw *hw)
>  {
> -	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;
>  	u16 mac_buf_len;
>  	int status;

How do you think about to reduce the scope for affected local variables instead
with the help of a small script (like the following) for the semantic patch language?


@movement1@
attribute name __free;
@@
-struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
 ... when any
+struct ice_aqc_get_phy_caps_data *
 pcaps
+__free(kfree)
 = kzalloc(sizeof(*pcaps), ...);

@movement2@
attribute name __free;
@@
-void *mac_buf __free(kfree);
 ... when any
+void *
 mac_buf
+__free(kfree)
 = kcalloc(2, sizeof(struct ice_aqc_manage_mac_read_resp), ...);


Regards,
Markus
Jonathan Cameron March 20, 2024, 4:33 p.m. UTC | #7
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
Jakub Kicinski March 21, 2024, 3:29 a.m. UTC | #8
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.
> > 
> > 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.

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.
Przemek Kitszel March 21, 2024, 9:59 a.m. UTC | #9
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.
Dan Carpenter March 21, 2024, 10:34 a.m. UTC | #10
On Thu, Mar 21, 2024 at 10:59:42AM +0100, Przemek Kitszel wrote:
> 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;

The auto_kfree looks like a variable to my eyes.  I'd prefer something
like:

#define __FREE(p) p __free(kfree) = NULL

	struct ice_aqc_get_phy_caps_data *__FREE(pcaps);

regards,
dan carpenter
Markus Elfring March 21, 2024, 5:59 p.m. UTC | #11
> Automatically cleaned up pointers need to be initialized before exiting
> their scope.  In this case, they need to be initialized to NULL before
> any return statement.

How will development interests evolve further for such design aspects?


…
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -941,11 +941,11 @@ static u64 ice_loopback_test(struct net_device *netdev)
>  	struct ice_netdev_priv *np = netdev_priv(netdev);
>  	struct ice_vsi *orig_vsi = np->vsi, *test_vsi;
>  	struct ice_pf *pf = orig_vsi->back;
> +	u8 *tx_frame __free(kfree) = NULL;
>  	u8 broadcast[ETH_ALEN], ret = 0;
>  	int num_frames, valid_frames;
>  	struct ice_tx_ring *tx_ring;
>  	struct ice_rx_ring *rx_ring;
> -	u8 *tx_frame __free(kfree);
>  	int i;
>
>  	netdev_info(netdev, "loopback test\n");

How do you think about to reduce the scope for the affected local variable instead
with the help of a small script (like the following) for the semantic patch language?

@movement@
attribute name __free;
@@
-u8 *tx_frame __free(kfree);
 int i;
 ... when any
 if (ice_fltr_add_mac(test_vsi, ...))
 { ... }
+
+{
+u8 *tx_frame __free(kfree) = NULL;
 if (ice_lbtest_create_frame(pf, &tx_frame, ...))
 { ... }
 ... when any
+}
+
 valid_frames = ice_lbtest_receive_frames(...);


Regards,
Markus
Andy Shevchenko March 21, 2024, 6:03 p.m. UTC | #12
On Thu, Mar 21, 2024 at 06:59:00PM +0100, Markus Elfring wrote:

…

> > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > @@ -941,11 +941,11 @@ static u64 ice_loopback_test(struct net_device *netdev)
> >  	struct ice_netdev_priv *np = netdev_priv(netdev);
> >  	struct ice_vsi *orig_vsi = np->vsi, *test_vsi;
> >  	struct ice_pf *pf = orig_vsi->back;
> > +	u8 *tx_frame __free(kfree) = NULL;
> >  	u8 broadcast[ETH_ALEN], ret = 0;
> >  	int num_frames, valid_frames;
> >  	struct ice_tx_ring *tx_ring;
> >  	struct ice_rx_ring *rx_ring;
> > -	u8 *tx_frame __free(kfree);
> >  	int i;
> >
> >  	netdev_info(netdev, "loopback test\n");
> 
> How do you think about to reduce the scope for the affected local variable instead
> with the help of a small script (like the following) for the semantic patch language?
> 
> @movement@
> attribute name __free;
> @@
> -u8 *tx_frame __free(kfree);
>  int i;
>  ... when any
>  if (ice_fltr_add_mac(test_vsi, ...))
>  { ... }
> +
> +{
> +u8 *tx_frame __free(kfree) = NULL;
>  if (ice_lbtest_create_frame(pf, &tx_frame, ...))
>  { ... }
>  ... when any
> +}
> +
>  valid_frames = ice_lbtest_receive_frames(...);

I believe you don't understand what the scope of the above can be.
Markus Elfring March 21, 2024, 6:14 p.m. UTC | #13
>> How do you think about to reduce the scope for the affected local variable instead
>> with the help of a small script (like the following) for the semantic patch language?
>>
>> @movement@
>> attribute name __free;
>> @@
>> -u8 *tx_frame __free(kfree);
>>  int i;
>>  ... when any
>>  if (ice_fltr_add_mac(test_vsi, ...))
>>  { ... }
>> +
>> +{
>> +u8 *tx_frame __free(kfree) = NULL;
>>  if (ice_lbtest_create_frame(pf, &tx_frame, ...))
>>  { ... }
>>  ... when any
>> +}
>> +
>>  valid_frames = ice_lbtest_receive_frames(...);
>
> I believe you don't understand what the scope of the above can be.

Will the understanding improve for the proposed source code transformation?

Regards,
Markus
Julia Lawall March 21, 2024, 8:20 p.m. UTC | #14
Does one prefer an initialization of null at the top of the function or an initialization to a meaningful value in the middle of the function ?

(Sorry for top posting)


Sent from my iPhone

> On 21 Mar 2024, at 14:14, Markus Elfring <Markus.Elfring@web.de> wrote:
> 
> 
>> 
>>> How do you think about to reduce the scope for the affected local variable instead
>>> with the help of a small script (like the following) for the semantic patch language?
>>> 
>>> @movement@
>>> attribute name __free;
>>> @@
>>> -u8 *tx_frame __free(kfree);
>>> int i;
>>> ... when any
>>> if (ice_fltr_add_mac(test_vsi, ...))
>>> { ... }
>>> +
>>> +{
>>> +u8 *tx_frame __free(kfree) = NULL;
>>> if (ice_lbtest_create_frame(pf, &tx_frame, ...))
>>> { ... }
>>> ... when any
>>> +}
>>> +
>>> valid_frames = ice_lbtest_receive_frames(...);
>> 
>> I believe you don't understand what the scope of the above can be.
> 
> Will the understanding improve for the proposed source code transformation?
> 
> Regards,
> Markus
Jesse Brandeburg March 21, 2024, 10:27 p.m. UTC | #15
On 3/21/2024 1:20 PM, Julia Lawall wrote:
> Does one prefer an initialization of null at the top of the function
> or an initialization to a meaningful value in the middle of the
> function ?

I think the latter.

There was a related patch explaining the direction, from Dan posted here:
https://lore.kernel.org/all/171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com/

We had been having some internal discussions about use of __free(kfree) 
in the ice driver.

The gist of it is that we should instead be using inline declarations, 
which I also agree is a reasonable style for this. It more clearly shows 
the __free(kfree) and the allocation (kzalloc, kcalloc, etc) on the same 
(or virtually the same) line of code.

I'm curious if Jakub would dislike this less? Accept?

as an example:
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c 
b/drivers/net/ethernet/intel/ice/ice_common.c
index 88c86de82e09..822628d25b2f 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1003,8 +1003,6 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw)
   */
  int ice_init_hw(struct ice_hw *hw)
  {
-       struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
         void *mac_buf __free(kfree) = NULL;
         u16 mac_buf_len;
         int status;

@@ -1083,7 +1081,8 @@ int ice_init_hw(struct ice_hw *hw)
         if (status)
                 goto err_unroll_sched;

-       pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL);
+       struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
+               kzalloc(sizeof(*pcaps), GFP_KERNEL);
         if (!pcaps) {
                 status = -ENOMEM;
                 goto err_unroll_sched;

Any thoughts?
Jakub Kicinski March 22, 2024, 1:48 a.m. UTC | #16
On Thu, 21 Mar 2024 15:27:47 -0700 Jesse Brandeburg wrote:
> The gist of it is that we should instead be using inline declarations, 
> which I also agree is a reasonable style for this. It more clearly shows 
> the __free(kfree) and the allocation (kzalloc, kcalloc, etc) on the same 
> (or virtually the same) line of code.
> 
> I'm curious if Jakub would dislike this less? Accept?

At present I find this construct unreadable.
I may get used to it, hard to say.

Also I don't see the benefit of the auto-freeing construct,
I'd venture a guess that all the bugs it may prevent would
have been caught by smatch. But I'm an old curmudgeon stuck
in my ways. Feel free to experiment in Intel drivers, and we'll
see how it works out 
Jakub Kicinski March 22, 2024, 1:56 a.m. UTC | #17
On Thu, 21 Mar 2024 18:48:28 -0700 Jakub Kicinski wrote:
> On Thu, 21 Mar 2024 15:27:47 -0700 Jesse Brandeburg wrote:
> > The gist of it is that we should instead be using inline declarations, 
> > which I also agree is a reasonable style for this. It more clearly shows 
> > the __free(kfree) and the allocation (kzalloc, kcalloc, etc) on the same 
> > (or virtually the same) line of code.
> > 
> > I'm curious if Jakub would dislike this less? Accept?  
> 
> At present I find this construct unreadable.
> I may get used to it, hard to say.
> 
> Also I don't see the benefit of the auto-freeing construct,
> I'd venture a guess that all the bugs it may prevent would
> have been caught by smatch. But I'm an old curmudgeon stuck
> in my ways. Feel free to experiment in Intel drivers, and we'll
> see how it works out 
Dan Carpenter March 22, 2024, 5:30 a.m. UTC | #18
On Thu, Mar 21, 2024 at 04:20:09PM -0400, Julia Lawall wrote:
> Does one prefer an initialization of null at the top of the function
> or an initialization to a meaningful value in the middle of the
> function?

I prefer at the top, but it will be interesting to see where the
consensus is.  Kent Overstreet has said we should move away from
declarations at the top generally.  I don't know if anyone else agrees
with him though.

regards,
dan carpenter
Julia Lawall March 22, 2024, 7:24 a.m. UTC | #19
On Thu, 21 Mar 2024, Jakub Kicinski wrote:

> On Thu, 21 Mar 2024 15:27:47 -0700 Jesse Brandeburg wrote:
> > The gist of it is that we should instead be using inline declarations,
> > which I also agree is a reasonable style for this. It more clearly shows
> > the __free(kfree) and the allocation (kzalloc, kcalloc, etc) on the same
> > (or virtually the same) line of code.
> >
> > I'm curious if Jakub would dislike this less? Accept?
>
> At present I find this construct unreadable.
> I may get used to it, hard to say.
>
> Also I don't see the benefit of the auto-freeing construct,
> I'd venture a guess that all the bugs it may prevent would
> have been caught by smatch. But I'm an old curmudgeon stuck
> in my ways. Feel free to experiment in Intel drivers, and we'll
> see how it works out 
Markus Elfring March 22, 2024, 8:48 a.m. UTC | #20
> Does one prefer an initialization of null at the top of the function

Several developers got used to such a programming approach.


> or an initialization to a meaningful value in the middle of the function ?

Coding style preferences are evolving more with the growing support for
the discussed scope-based resource management (cleanup functions and guards),
aren't they?

Further developers can handle variable definitions at the beginning of
a compound statement (a code block) at least.
Corresponding clarifications will influence the change acceptance for such definitions
without adding extra curly brackets.
Would you like to consider design possibilities with scope reductions?

Regards,
Markus
Jakub Kicinski March 22, 2024, 3:03 p.m. UTC | #21
On Fri, 22 Mar 2024 03:24:56 -0400 (EDT) Julia Lawall wrote:
> > At present I find this construct unreadable.
> > I may get used to it, hard to say.
> >
> > Also I don't see the benefit of the auto-freeing construct,
> > I'd venture a guess that all the bugs it may prevent would
> > have been caught by smatch. But I'm an old curmudgeon stuck
> > in my ways. Feel free to experiment in Intel drivers, and we'll
> > see how it works out 
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 4d8111aeb0ff..4b27d2bc2912 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1002,8 +1002,8 @@  static void ice_get_itr_intrl_gran(struct ice_hw *hw)
  */
 int ice_init_hw(struct ice_hw *hw)
 {
-	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;
 	u16 mac_buf_len;
 	int status;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 255a9c8151b4..78b833b3e1d7 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -941,11 +941,11 @@  static u64 ice_loopback_test(struct net_device *netdev)
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_vsi *orig_vsi = np->vsi, *test_vsi;
 	struct ice_pf *pf = orig_vsi->back;
+	u8 *tx_frame __free(kfree) = NULL;
 	u8 broadcast[ETH_ALEN], ret = 0;
 	int num_frames, valid_frames;
 	struct ice_tx_ring *tx_ring;
 	struct ice_rx_ring *rx_ring;
-	u8 *tx_frame __free(kfree);
 	int i;
 
 	netdev_info(netdev, "loopback test\n");