| Message ID | 0efe132b-b343-4438-bb00-5a4b82722ed3@moroto.mountain |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [v2,net] ice: Fix freeing uninitialized pointers | expand |
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 > > >
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
> 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
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
> 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
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>
> 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
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
>>> 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
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");
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(-)