* [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-21 14:42 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-21 14:42 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Przemek Kitszel, intel-wired-lan,
netdev, linux-kernel, kernel-janitors
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>
---
v2: I missed a couple pointers in v1.
The change to ice_update_link_info() isn't required because it's
assigned on the very next line... But I did that because it's harmless
and makes __free() stuff easier to verify. I felt like moving the
declarations into the code would be controversial and it also ends up
making the lines really long.
goto goto err_unroll_sched;
struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
kzalloc(sizeof(*pcaps), GFP_KERNEL);
drivers/net/ethernet/intel/ice/ice_common.c | 10 +++++-----
drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
2 file changed, 6 insertion(+), 6 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 4d8111aeb0ff..6f2db603b36e 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;
@@ -3272,7 +3272,7 @@ int ice_update_link_info(struct ice_port_info *pi)
return status;
if (li->link_info & ICE_AQ_MEDIA_AVAILABLE) {
- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL);
if (!pcaps)
@@ -3420,7 +3420,7 @@ ice_cfg_phy_fc(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
int
ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update)
{
- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
struct ice_aqc_set_phy_cfg_data cfg = { 0 };
struct ice_hw *hw;
int status;
@@ -3561,7 +3561,7 @@ int
ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
enum ice_fec_mode fec)
{
- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
struct ice_hw *hw;
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 14:42 ` Dan Carpenter
@ 2024-03-21 15:34 ` Jiri Pirko
-1 siblings, 0 replies; 20+ messages in thread
From: Jiri Pirko @ 2024-03-21 15:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Maciej Fijalkowski, Przemek Kitszel, kernel-janitors,
linux-kernel, Eric Dumazet, netdev, Tony Nguyen, intel-wired-lan,
Jakub Kicinski, Paolo Abeni, David S. Miller
Thu, Mar 21, 2024 at 03:42:12PM 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>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>---
>v2: I missed a couple pointers in v1.
>
>The change to ice_update_link_info() isn't required because it's
>assigned on the very next line... But I did that because it's harmless
>and makes __free() stuff easier to verify. I felt like moving the
>declarations into the code would be controversial and it also ends up
>making the lines really long.
>
> goto goto err_unroll_sched;
>
> struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> kzalloc(sizeof(*pcaps), GFP_KERNEL);
Yeah, that is why I'm proposing KZALLOC_FREE helper:
https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
>
> drivers/net/ethernet/intel/ice/ice_common.c | 10 +++++-----
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> 2 file changed, 6 insertion(+), 6 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
>index 4d8111aeb0ff..6f2db603b36e 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;
>
>@@ -3272,7 +3272,7 @@ int ice_update_link_info(struct ice_port_info *pi)
> return status;
>
> if (li->link_info & ICE_AQ_MEDIA_AVAILABLE) {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
>
> pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL);
> if (!pcaps)
>@@ -3420,7 +3420,7 @@ ice_cfg_phy_fc(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
> int
> ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update)
> {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
> struct ice_aqc_set_phy_cfg_data cfg = { 0 };
> struct ice_hw *hw;
> int status;
>@@ -3561,7 +3561,7 @@ int
> ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
> enum ice_fec_mode fec)
> {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
> struct ice_hw *hw;
> 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
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-21 15:34 ` Jiri Pirko
0 siblings, 0 replies; 20+ messages in thread
From: Jiri Pirko @ 2024-03-21 15:34 UTC (permalink / raw)
To: Dan Carpenter
Cc: Maciej Fijalkowski, Jesse Brandeburg, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, intel-wired-lan, netdev, linux-kernel,
kernel-janitors
Thu, Mar 21, 2024 at 03:42:12PM 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>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>---
>v2: I missed a couple pointers in v1.
>
>The change to ice_update_link_info() isn't required because it's
>assigned on the very next line... But I did that because it's harmless
>and makes __free() stuff easier to verify. I felt like moving the
>declarations into the code would be controversial and it also ends up
>making the lines really long.
>
> goto goto err_unroll_sched;
>
> struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> kzalloc(sizeof(*pcaps), GFP_KERNEL);
Yeah, that is why I'm proposing KZALLOC_FREE helper:
https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
>
> drivers/net/ethernet/intel/ice/ice_common.c | 10 +++++-----
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> 2 file changed, 6 insertion(+), 6 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
>index 4d8111aeb0ff..6f2db603b36e 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;
>
>@@ -3272,7 +3272,7 @@ int ice_update_link_info(struct ice_port_info *pi)
> return status;
>
> if (li->link_info & ICE_AQ_MEDIA_AVAILABLE) {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
>
> pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL);
> if (!pcaps)
>@@ -3420,7 +3420,7 @@ ice_cfg_phy_fc(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
> int
> ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update)
> {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
> struct ice_aqc_set_phy_cfg_data cfg = { 0 };
> struct ice_hw *hw;
> int status;
>@@ -3561,7 +3561,7 @@ int
> ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg,
> enum ice_fec_mode fec)
> {
>- struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>+ struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
> struct ice_hw *hw;
> 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
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 15:34 ` Jiri Pirko
@ 2024-03-21 15:49 ` Dan Carpenter
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-21 15:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Maciej Fijalkowski, Przemek Kitszel, kernel-janitors,
linux-kernel, Eric Dumazet, netdev, Tony Nguyen, intel-wired-lan,
Jakub Kicinski, Paolo Abeni, David S. Miller
On Thu, Mar 21, 2024 at 04:34:37PM +0100, Jiri Pirko wrote:
> >The change to ice_update_link_info() isn't required because it's
> >assigned on the very next line... But I did that because it's harmless
> >and makes __free() stuff easier to verify. I felt like moving the
> >declarations into the code would be controversial and it also ends up
> >making the lines really long.
> >
> > goto goto err_unroll_sched;
> >
> > struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> > kzalloc(sizeof(*pcaps), GFP_KERNEL);
>
> Yeah, that is why I'm proposing KZALLOC_FREE helper:
> https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
>
I like the idea, but I'm not keen on the format. What about something
like?
#define __ALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL)
struct ice_aqc_get_phy_caps_data *__ALLOC(pcaps);
I'm not a huge fan of putting functions which can fail into the
declaration block but I feel like we're going to officially say that
small allocations can't fail.
https://lwn.net/Articles/964793/
https://lore.kernel.org/all/170925937840.24797.2167230750547152404@noble.neil.brown.name/
Normally we would try to delay the allocations until after all the
sanity checks have run but that's optimizing for the failure case. In
the normal case we're going to want these allocations.
regards,
damn carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-21 15:49 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-21 15:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Maciej Fijalkowski, Jesse Brandeburg, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, intel-wired-lan, netdev, linux-kernel,
kernel-janitors
On Thu, Mar 21, 2024 at 04:34:37PM +0100, Jiri Pirko wrote:
> >The change to ice_update_link_info() isn't required because it's
> >assigned on the very next line... But I did that because it's harmless
> >and makes __free() stuff easier to verify. I felt like moving the
> >declarations into the code would be controversial and it also ends up
> >making the lines really long.
> >
> > goto goto err_unroll_sched;
> >
> > struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> > kzalloc(sizeof(*pcaps), GFP_KERNEL);
>
> Yeah, that is why I'm proposing KZALLOC_FREE helper:
> https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
>
I like the idea, but I'm not keen on the format. What about something
like?
#define __ALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL)
struct ice_aqc_get_phy_caps_data *__ALLOC(pcaps);
I'm not a huge fan of putting functions which can fail into the
declaration block but I feel like we're going to officially say that
small allocations can't fail.
https://lwn.net/Articles/964793/
https://lore.kernel.org/all/170925937840.24797.2167230750547152404@noble.neil.brown.name/
Normally we would try to delay the allocations until after all the
sanity checks have run but that's optimizing for the failure case. In
the normal case we're going to want these allocations.
regards,
damn carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 14:42 ` Dan Carpenter
@ 2024-03-21 20:05 ` Markus Elfring
-1 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-21 20:05 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan,
Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen
Cc: Pucha Himasekhar Reddy, Paolo Abeni, Jiri Pirko, Kees Cook,
Lukasz Czapnik, LKML, Julia Lawall, Alexander Lobakin,
Eric Dumazet, David Laight, Jakub Kicinski, Andy Shevchenko,
David S. Miller, Jonathan Cameron
> 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.
Will any adjustments become relevant also for this change description
if scope reductions would become more appealing for affected local variables?
How much can a small script (like the following) for the semantic patch language
(Coccinelle software) help to achieve a better common understanding for
possible source code transformations?
// See also:
// drivers/net/ethernet/intel/ice/ice_common.c
@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), ...);
// See also:
// drivers/net/ethernet/intel/ice/ice_ethtool.c
@movement3@
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-21 20:05 ` Markus Elfring
0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-21 20:05 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan,
Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen
Cc: LKML, Alexander Lobakin, Andy Shevchenko, David Laight,
David S. Miller, Eric Dumazet, Jakub Kicinski, Jesse Brandeburg,
Jiri Pirko, Jonathan Cameron, Julia Lawall, Kees Cook,
Lukasz Czapnik, Paolo Abeni, Pucha Himasekhar Reddy
> 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.
Will any adjustments become relevant also for this change description
if scope reductions would become more appealing for affected local variables?
How much can a small script (like the following) for the semantic patch language
(Coccinelle software) help to achieve a better common understanding for
possible source code transformations?
// See also:
// drivers/net/ethernet/intel/ice/ice_common.c
@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), ...);
// See also:
// drivers/net/ethernet/intel/ice/ice_ethtool.c
@movement3@
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 20:05 ` Markus Elfring
@ 2024-03-22 5:32 ` Dan Carpenter
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-22 5:32 UTC (permalink / raw)
To: Markus Elfring
Cc: Jiri Pirko, Pucha Himasekhar Reddy, Paolo Abeni,
Maciej Fijalkowski, Kees Cook, netdev, Lukasz Czapnik,
kernel-janitors, LKML, Julia Lawall, Alexander Lobakin,
Eric Dumazet, David Laight, Tony Nguyen, Jakub Kicinski,
Przemek Kitszel, intel-wired-lan, Andy Shevchenko,
David S. Miller, Jonathan Cameron
Markus please don't do this. Don't take a controversial opinion and
start trying to force it on everyone via review comments and an
automatic converstion script.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-22 5:32 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-22 5:32 UTC (permalink / raw)
To: Markus Elfring
Cc: kernel-janitors, netdev, intel-wired-lan, Maciej Fijalkowski,
Przemek Kitszel, Tony Nguyen, LKML, Alexander Lobakin,
Andy Shevchenko, David Laight, David S. Miller, Eric Dumazet,
Jakub Kicinski, Jesse Brandeburg, Jiri Pirko, Jonathan Cameron,
Julia Lawall, Kees Cook, Lukasz Czapnik, Paolo Abeni,
Pucha Himasekhar Reddy
Markus please don't do this. Don't take a controversial opinion and
start trying to force it on everyone via review comments and an
automatic converstion script.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [v2] ice: Fix freeing uninitialized pointers
2024-03-22 5:32 ` Dan Carpenter
@ 2024-03-22 10:10 ` Markus Elfring
-1 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-22 10:10 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan
Cc: Jiri Pirko, Pucha Himasekhar Reddy, Paolo Abeni,
Maciej Fijalkowski, Kees Cook, Przemek Kitszel, Lukasz Czapnik,
LKML, Julia Lawall, Alexander Lobakin, Eric Dumazet, David Laight,
Tony Nguyen, Jakub Kicinski, Dan Williams, Andy Shevchenko,
David S. Miller, Jonathan Cameron
> Markus please don't do this. Don't take a controversial opinion and
> start trying to force it on everyone via review comments and an
> automatic converstion script.
I dare also to point additional change possibilities out.
I hope that further collateral evolution will become better supported.
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v2] ice: Fix freeing uninitialized pointers
@ 2024-03-22 10:10 ` Markus Elfring
0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-22 10:10 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan
Cc: Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen, LKML,
Alexander Lobakin, Andy Shevchenko, David Laight, David S. Miller,
Eric Dumazet, Jakub Kicinski, Jesse Brandeburg, Jiri Pirko,
Jonathan Cameron, Julia Lawall, Kees Cook, Lukasz Czapnik,
Paolo Abeni, Pucha Himasekhar Reddy, Dan Williams
> Markus please don't do this. Don't take a controversial opinion and
> start trying to force it on everyone via review comments and an
> automatic converstion script.
I dare also to point additional change possibilities out.
I hope that further collateral evolution will become better supported.
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 14:42 ` Dan Carpenter
@ 2024-03-22 12:57 ` Simon Horman
-1 siblings, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-03-22 12:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Maciej Fijalkowski, Przemek Kitszel, kernel-janitors,
linux-kernel, Eric Dumazet, netdev, Tony Nguyen, intel-wired-lan,
Jakub Kicinski, Paolo Abeni, David S. Miller
On Thu, Mar 21, 2024 at 05:42:12PM +0300, Dan Carpenter 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>
> ---
> v2: I missed a couple pointers in v1.
>
> The change to ice_update_link_info() isn't required because it's
> assigned on the very next line... But I did that because it's harmless
> and makes __free() stuff easier to verify. I felt like moving the
> declarations into the code would be controversial and it also ends up
> making the lines really long.
>
> goto goto err_unroll_sched;
>
> struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> kzalloc(sizeof(*pcaps), GFP_KERNEL);
Thanks Dan,
I agree with the approach you have taken here.
And I apologise that it's quite likely that I skipped warnings regarding
these problems when reviewing patches that introduced them - I did not
understand the issue that this patch resolves.
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-22 12:57 ` Simon Horman
0 siblings, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-03-22 12:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Maciej Fijalkowski, Jesse Brandeburg, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, intel-wired-lan, netdev, linux-kernel,
kernel-janitors
On Thu, Mar 21, 2024 at 05:42:12PM +0300, Dan Carpenter 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>
> ---
> v2: I missed a couple pointers in v1.
>
> The change to ice_update_link_info() isn't required because it's
> assigned on the very next line... But I did that because it's harmless
> and makes __free() stuff easier to verify. I felt like moving the
> declarations into the code would be controversial and it also ends up
> making the lines really long.
>
> goto goto err_unroll_sched;
>
> struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) =
> kzalloc(sizeof(*pcaps), GFP_KERNEL);
Thanks Dan,
I agree with the approach you have taken here.
And I apologise that it's quite likely that I skipped warnings regarding
these problems when reviewing patches that introduced them - I did not
understand the issue that this patch resolves.
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-21 14:42 ` Dan Carpenter
@ 2024-03-23 16:56 ` Markus Elfring
-1 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-23 16:56 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan, smatch
Cc: Pucha Himasekhar Reddy, Paolo Abeni, Jiri Pirko, Kees Cook,
Tony Nguyen, Przemek Kitszel, Lukasz Czapnik, LKML, Julia Lawall,
Alexander Lobakin, Eric Dumazet, David Laight, Maciej Fijalkowski,
Jakub Kicinski, Dan Williams, Andy Shevchenko, David S. Miller,
Jonathan Cameron
> 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.
* May we expect that compilers should report that affected variables
were only declared here instead of appropriately defined
(despite of attempts for scope-based resource management)?
* Did you extend detection support in the source code analysis tool “Smatch”
for a questionable implementation detail?
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-23 16:56 ` Markus Elfring
0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-23 16:56 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan, smatch
Cc: LKML, Alexander Lobakin, Andy Shevchenko, David Laight,
David S. Miller, Eric Dumazet, Jakub Kicinski, Jesse Brandeburg,
Jiri Pirko, Jonathan Cameron, Julia Lawall, Kees Cook,
Lukasz Czapnik, Paolo Abeni, Pucha Himasekhar Reddy, Dan Williams,
Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen
> 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.
* May we expect that compilers should report that affected variables
were only declared here instead of appropriately defined
(despite of attempts for scope-based resource management)?
* Did you extend detection support in the source code analysis tool “Smatch”
for a questionable implementation detail?
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 net] ice: Fix freeing uninitialized pointers
2024-03-23 16:56 ` Markus Elfring
@ 2024-03-24 10:43 ` Dan Carpenter
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-24 10:43 UTC (permalink / raw)
To: Markus Elfring
Cc: kernel-janitors, smatch, Eric Dumazet, Tony Nguyen,
Maciej Fijalkowski, Przemek Kitszel, intel-wired-lan,
Jakub Kicinski, Paolo Abeni, Pucha Himasekhar Reddy, Jiri Pirko,
Kees Cook, Lukasz Czapnik, Dan Williams, Andy Shevchenko, netdev,
LKML, Julia Lawall, Alexander Lobakin, David Laight,
David S. Miller, Jonathan Cameron
On Sat, Mar 23, 2024 at 05:56:29PM +0100, Markus Elfring 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.
>
> * May we expect that compilers should report that affected variables
> were only declared here instead of appropriately defined
> (despite of attempts for scope-based resource management)?
>
We disabled GCC's check for uninitialized variables a long time ago
because it had too many false positives.
> * Did you extend detection support in the source code analysis tool “Smatch”
> for a questionable implementation detail?
Yes. Smatch detects this as an uninitialized variable.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers
@ 2024-03-24 10:43 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2024-03-24 10:43 UTC (permalink / raw)
To: Markus Elfring
Cc: kernel-janitors, netdev, intel-wired-lan, smatch, LKML,
Alexander Lobakin, Andy Shevchenko, David Laight, David S. Miller,
Eric Dumazet, Jakub Kicinski, Jesse Brandeburg, Jiri Pirko,
Jonathan Cameron, Julia Lawall, Kees Cook, Lukasz Czapnik,
Paolo Abeni, Pucha Himasekhar Reddy, Dan Williams,
Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen
On Sat, Mar 23, 2024 at 05:56:29PM +0100, Markus Elfring 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.
>
> * May we expect that compilers should report that affected variables
> were only declared here instead of appropriately defined
> (despite of attempts for scope-based resource management)?
>
We disabled GCC's check for uninitialized variables a long time ago
because it had too many false positives.
> * Did you extend detection support in the source code analysis tool “Smatch”
> for a questionable implementation detail?
Yes. Smatch detects this as an uninitialized variable.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [v2] ice: Fix freeing uninitialized pointers
2024-03-24 10:43 ` Dan Carpenter
@ 2024-03-24 13:22 ` Markus Elfring
-1 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-24 13:22 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan, smatch
Cc: Pucha Himasekhar Reddy, Paolo Abeni, Jiri Pirko, Kees Cook,
Tony Nguyen, Przemek Kitszel, Lukasz Czapnik, LKML, Julia Lawall,
Alexander Lobakin, Eric Dumazet, David Laight, Maciej Fijalkowski,
Jakub Kicinski, Dan Williams, Andy Shevchenko, David S. Miller,
Jonathan Cameron
>>> 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.
>>
>> * May we expect that compilers should report that affected variables
>> were only declared here instead of appropriately defined
>> (despite of attempts for scope-based resource management)?
>>
>
> We disabled GCC's check for uninitialized variables a long time ago
> because it had too many false positives.
Can further case distinctions (and compilation parameters) become more helpful
according to the discussed handling of the attribute “__cleanup” (or “__free”)?
>> * Did you extend detection support in the source code analysis tool “Smatch”
>> for a questionable implementation detail?
>
> Yes. Smatch detects this as an uninitialized variable.
Does the corresponding warning indicate requirements for scope-based resource management?
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v2] ice: Fix freeing uninitialized pointers
@ 2024-03-24 13:22 ` Markus Elfring
0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2024-03-24 13:22 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, netdev, intel-wired-lan, smatch
Cc: LKML, Alexander Lobakin, Andy Shevchenko, David Laight,
David S. Miller, Eric Dumazet, Jakub Kicinski, Jesse Brandeburg,
Jiri Pirko, Jonathan Cameron, Julia Lawall, Kees Cook,
Lukasz Czapnik, Paolo Abeni, Pucha Himasekhar Reddy, Dan Williams,
Maciej Fijalkowski, Przemek Kitszel, Tony Nguyen
>>> 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.
>>
>> * May we expect that compilers should report that affected variables
>> were only declared here instead of appropriately defined
>> (despite of attempts for scope-based resource management)?
>>
>
> We disabled GCC's check for uninitialized variables a long time ago
> because it had too many false positives.
Can further case distinctions (and compilation parameters) become more helpful
according to the discussed handling of the attribute “__cleanup” (or “__free”)?
>> * Did you extend detection support in the source code analysis tool “Smatch”
>> for a questionable implementation detail?
>
> Yes. Smatch detects this as an uninitialized variable.
Does the corresponding warning indicate requirements for scope-based resource management?
Regards,
Markus
^ permalink raw reply [flat|nested] 20+ messages in thread