[Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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;
 
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

[PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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;
 
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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jiri Pirko]

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
>
>

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

[Jiri Pirko]

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
>
>

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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

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

[Dan Carpenter]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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.

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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

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

[Dan Carpenter]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Julia Lawall]

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

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

[Julia Lawall]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jonathan Cameron]

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

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

[Jonathan Cameron]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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.

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Przemek Kitszel]

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.

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

[Przemek Kitszel]

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.

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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

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

[Dan Carpenter]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Markus Elfring]

> 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

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

[Markus Elfring]

> 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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Markus Elfring]

> 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

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

[Markus Elfring]

> 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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Markus Elfring]

>> 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

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

[Markus Elfring]

>> 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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Julia Lawall]

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

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

[Julia Lawall]

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

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jesse Brandeburg]

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?

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

[Jesse Brandeburg]

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?

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jakub Kicinski]

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 🤷️

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

[Jakub Kicinski]

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 🤷️

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jakub Kicinski]

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 🤷️

On further reflection, yes, of all the bad options moving the
declarations inline in this particular case is probably the
least bad option.

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

[Jakub Kicinski]

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 🤷️

On further reflection, yes, of all the bad options moving the
declarations inline in this particular case is probably the
least bad option.

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]



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 🤷️

In my experiments with of_node_put, there seem to be many functions where
removing the frees makes the function much more readable.  But
kmalloc/kfree may be used in different contexts, where the management of
the memory is a smaller percentage of the overall code.  So the tradeoffs
may be different.

julia

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

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]



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 🤷️

In my experiments with of_node_put, there seem to be many functions where
removing the frees makes the function much more readable.  But
kmalloc/kfree may be used in different contexts, where the management of
the memory is a smaller percentage of the overall code.  So the tradeoffs
may be different.

julia

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Jakub Kicinski]

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 🤷️  
> 
> In my experiments with of_node_put, there seem to be many functions where
> removing the frees makes the function much more readable.  But
> kmalloc/kfree may be used in different contexts, where the management of
> the memory is a smaller percentage of the overall code.  So the tradeoffs
> may be different.

Good point! References are likely a very good use case for this sort
of thing. The act of bumping a counter lacks the feeling of lifetime
we get with an object :(

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

[Jakub Kicinski]

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 🤷️  
> 
> In my experiments with of_node_put, there seem to be many functions where
> removing the frees makes the function much more readable.  But
> kmalloc/kfree may be used in different contexts, where the management of
> the memory is a smaller percentage of the overall code.  So the tradeoffs
> may be different.

Good point! References are likely a very good use case for this sort
of thing. The act of bumping a counter lacks the feeling of lifetime
we get with an object :(

Re: [Intel-wired-lan] [PATCH net] ice: Fix freeing uninitialized pointers

[Dan Carpenter]

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

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

[Dan Carpenter]

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

Re: [Intel-wired-lan] ice: Fix freeing uninitialized pointers

[Markus Elfring]

> 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

Re: ice: Fix freeing uninitialized pointers

[Markus Elfring]

> 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