| Message ID | 20230706091910.124498-1-jedrzej.jagielski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net,v3] ice: Fix memory management in ice_ethtool_fdir.c | expand |
On 7/6/2023 2:19 AM, Jedrzej Jagielski wrote: > Fix ethtool FDIR logic to not use memory after its release. > In the ice_ethtool_fdir.c file there are 2 spots where code can > refer to pointers which may be missing. > > In the ice_cfg_fdir_xtrct_seq() function seg may be freed but > even then may be still used by memcpy(&tun_seg[1], seg, sizeof(*seg)). > > In the ice_add_fdir_ethtool() function struct ice_fdir_fltr *input > may first fail to be added via ice_fdir_update_list_entry() but then > may be deleted by ice_fdir_update_list_entry. > > Terminate in both cases when the returned value of the previous > operation is other than 0, free memory and don't use it anymore. > > Replace managed memory alloc with kzalloc/kfree in > ice_cfg_fdir_xtrct_seq() since seg/tun_seg are used only by > ice_fdir_set_hw_fltr_rule(). > > Reported-by: Michal Schmidt <mschmidt@redhat.com> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2208423 > Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com> > --- > v2: extend CC list, fix freeing memory before return > v3: correct typos in the commit msg > --- > .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 62 +++++++++---------- > 1 file changed, 28 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c > index ead6d50fc0ad..619b32f4bc53 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c > @@ -1204,21 +1204,16 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, > struct ice_rx_flow_userdef *user) > { > struct ice_flow_seg_info *seg, *tun_seg; > - struct device *dev = ice_pf_to_dev(pf); > enum ice_fltr_ptype fltr_idx; > struct ice_hw *hw = &pf->hw; > bool perfect_filter; > int ret; > > - seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL); > - if (!seg) > - return -ENOMEM; > - > - tun_seg = devm_kcalloc(dev, ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), > - GFP_KERNEL); > - if (!tun_seg) { > - devm_kfree(dev, seg); > - return -ENOMEM; > + seg = kzalloc(sizeof(*seg), GFP_KERNEL); > + tun_seg = kcalloc(ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), GFP_KERNEL); > + if (!tun_seg || !seg) { > + ret = -ENOMEM; > + goto exit; IIRC individual checks and goto's are preferred over combining them. > } > > switch (fsp->flow_type & ~FLOW_EXT) { > @@ -1264,7 +1259,7 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, > ret = -EINVAL; > } > if (ret) > - goto err_exit; > + goto exit; > > /* tunnel segments are shifted up one. */ > memcpy(&tun_seg[1], seg, sizeof(*seg)); > @@ -1281,42 +1276,39 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, > ICE_FLOW_FLD_OFF_INVAL); > } > > - /* add filter for outer headers */ > fltr_idx = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT); > + > + if (perfect_filter) > + set_bit(fltr_idx, hw->fdir_perfect_fltr); > + else > + clear_bit(fltr_idx, hw->fdir_perfect_fltr); > + > + /* add filter for outer headers */ > ret = ice_fdir_set_hw_fltr_rule(pf, seg, fltr_idx, > ICE_FD_HW_SEG_NON_TUN); > - if (ret == -EEXIST) > - /* Rule already exists, free memory and continue */ > - devm_kfree(dev, seg); > - else if (ret) > + if (ret == -EEXIST) { > + /* Rule already exists, free memory and count as success */ > + ret = 0; > + goto exit; > + } else if (ret) { > /* could not write filter, free memory */ > - goto err_exit; > + ret = -EOPNOTSUPP; > + goto exit; > + } > > /* make tunneled filter HW entries if possible */ > memcpy(&tun_seg[1], seg, sizeof(*seg)); > ret = ice_fdir_set_hw_fltr_rule(pf, tun_seg, fltr_idx, > ICE_FD_HW_SEG_TUN); > - if (ret == -EEXIST) { > + if (ret == -EEXIST) > /* Rule already exists, free memory and count as success */ > - devm_kfree(dev, tun_seg); > ret = 0; > - } else if (ret) { > - /* could not write tunnel filter, but outer filter exists */ > - devm_kfree(dev, tun_seg); > - } > > - if (perfect_filter) > - set_bit(fltr_idx, hw->fdir_perfect_fltr); > - else > - clear_bit(fltr_idx, hw->fdir_perfect_fltr); > +exit: > + kfree(tun_seg); > + kfree(seg); Previously, success would not free these. They look to be set into hw_prof via ice_fdir_set_hw_fltr_rule(). Is it safe to be freeing them now? > return ret; > - > -err_exit: > - devm_kfree(dev, tun_seg); > - devm_kfree(dev, seg); > - > - return -EOPNOTSUPP; > } > > /** > @@ -1914,7 +1906,9 @@ int ice_add_fdir_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd) > input->comp_report = ICE_FXD_FLTR_QW0_COMP_REPORT_SW_FAIL; > > /* input struct is added to the HW filter list */ > - ice_fdir_update_list_entry(pf, input, fsp->location); > + ret = ice_fdir_update_list_entry(pf, input, fsp->location); > + if (ret) > + goto release_lock; > > ret = ice_fdir_write_all_fltr(pf, input, true); > if (ret)
From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> Sent: Fri, 7 July 2023 19:16 >On 7/6/2023 2:19 AM, Jedrzej Jagielski wrote: >> Fix ethtool FDIR logic to not use memory after its release. >> In the ice_ethtool_fdir.c file there are 2 spots where code can refer >> to pointers which may be missing. >> >> In the ice_cfg_fdir_xtrct_seq() function seg may be freed but even >> then may be still used by memcpy(&tun_seg[1], seg, sizeof(*seg)). >> >> In the ice_add_fdir_ethtool() function struct ice_fdir_fltr *input may >> first fail to be added via ice_fdir_update_list_entry() but then may >> be deleted by ice_fdir_update_list_entry. >> >> Terminate in both cases when the returned value of the previous >> operation is other than 0, free memory and don't use it anymore. >> >> Replace managed memory alloc with kzalloc/kfree in >> ice_cfg_fdir_xtrct_seq() since seg/tun_seg are used only by >> ice_fdir_set_hw_fltr_rule(). >> >> Reported-by: Michal Schmidt <mschmidt@redhat.com> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2208423 >> Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com> >> --- >> v2: extend CC list, fix freeing memory before return >> v3: correct typos in the commit msg >> --- >> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 62 +++++++++---------- >> 1 file changed, 28 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c >> b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c >> index ead6d50fc0ad..619b32f4bc53 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c >> @@ -1204,21 +1204,16 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, >> struct ice_rx_flow_userdef *user) >> { >> struct ice_flow_seg_info *seg, *tun_seg; >> - struct device *dev = ice_pf_to_dev(pf); >> enum ice_fltr_ptype fltr_idx; >> struct ice_hw *hw = &pf->hw; >> bool perfect_filter; >> int ret; >> >> - seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL); >> - if (!seg) >> - return -ENOMEM; >> - >> - tun_seg = devm_kcalloc(dev, ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), >> - GFP_KERNEL); >> - if (!tun_seg) { >> - devm_kfree(dev, seg); >> - return -ENOMEM; >> + seg = kzalloc(sizeof(*seg), GFP_KERNEL); >> + tun_seg = kcalloc(ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), GFP_KERNEL); >> + if (!tun_seg || !seg) { >> + ret = -ENOMEM; >> + goto exit; > >IIRC individual checks and goto's are preferred over combining them. For both cases there is the same behavior so it was done due to limit the line redundancy, but if you think it is better to split them up i can do this > >> } >> >> switch (fsp->flow_type & ~FLOW_EXT) { @@ -1264,7 +1259,7 @@ >> ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, >> ret = -EINVAL; >> } >> if (ret) >> - goto err_exit; >> + goto exit; >> >> /* tunnel segments are shifted up one. */ >> memcpy(&tun_seg[1], seg, sizeof(*seg)); @@ -1281,42 +1276,39 @@ >> ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, >> ICE_FLOW_FLD_OFF_INVAL); >> } >> >> - /* add filter for outer headers */ >> fltr_idx = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT); >> + >> + if (perfect_filter) >> + set_bit(fltr_idx, hw->fdir_perfect_fltr); >> + else >> + clear_bit(fltr_idx, hw->fdir_perfect_fltr); >> + >> + /* add filter for outer headers */ >> ret = ice_fdir_set_hw_fltr_rule(pf, seg, fltr_idx, >> ICE_FD_HW_SEG_NON_TUN); >> - if (ret == -EEXIST) >> - /* Rule already exists, free memory and continue */ >> - devm_kfree(dev, seg); >> - else if (ret) >> + if (ret == -EEXIST) { >> + /* Rule already exists, free memory and count as success */ >> + ret = 0; >> + goto exit; >> + } else if (ret) { >> /* could not write filter, free memory */ >> - goto err_exit; >> + ret = -EOPNOTSUPP; >> + goto exit; >> + } >> >> /* make tunneled filter HW entries if possible */ >> memcpy(&tun_seg[1], seg, sizeof(*seg)); >> ret = ice_fdir_set_hw_fltr_rule(pf, tun_seg, fltr_idx, >> ICE_FD_HW_SEG_TUN); >> - if (ret == -EEXIST) { >> + if (ret == -EEXIST) >> /* Rule already exists, free memory and count as success */ >> - devm_kfree(dev, tun_seg); >> ret = 0; >> - } else if (ret) { >> - /* could not write tunnel filter, but outer filter exists */ >> - devm_kfree(dev, tun_seg); >> - } >> >> - if (perfect_filter) >> - set_bit(fltr_idx, hw->fdir_perfect_fltr); >> - else >> - clear_bit(fltr_idx, hw->fdir_perfect_fltr); >> +exit: >> + kfree(tun_seg); >> + kfree(seg); > >Previously, success would not free these. They look to be set into hw_prof via ice_fdir_set_hw_fltr_rule(). Is it safe to be freeing them now? Yeah, I will restore the previous approach to avoid confusion > >> return ret; >> - >> -err_exit: >> - devm_kfree(dev, tun_seg); >> - devm_kfree(dev, seg); >> - >> - return -EOPNOTSUPP; >> } >> >> /** >> @@ -1914,7 +1906,9 @@ int ice_add_fdir_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd) >> input->comp_report = ICE_FXD_FLTR_QW0_COMP_REPORT_SW_FAIL; >> >> /* input struct is added to the HW filter list */ >> - ice_fdir_update_list_entry(pf, input, fsp->location); >> + ret = ice_fdir_update_list_entry(pf, input, fsp->location); >> + if (ret) >> + goto release_lock; >> >> ret = ice_fdir_write_all_fltr(pf, input, true); >> if (ret)
On 7/10/2023 5:53 AM, Jagielski, Jedrzej wrote: ... >>> - seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL); >>> - if (!seg) >>> - return -ENOMEM; >>> - >>> - tun_seg = devm_kcalloc(dev, ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), >>> - GFP_KERNEL); >>> - if (!tun_seg) { >>> - devm_kfree(dev, seg); >>> - return -ENOMEM; >>> + seg = kzalloc(sizeof(*seg), GFP_KERNEL); >>> + tun_seg = kcalloc(ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), GFP_KERNEL); >>> + if (!tun_seg || !seg) { >>> + ret = -ENOMEM; >>> + goto exit; >> >> IIRC individual checks and goto's are preferred over combining them. > > For both cases there is the same behavior so it was done due to limit > the line redundancy, but if you think it is better to split them up i > can do this Please go back to individual checks; I found the previous comment [1]. "Don't do combined error checking, add appropriate labels for each case." Also, as you're changing the allocation to not use the managed allocation, you're going to need to find the related (non-error) free and change those to be "regular" kfree as well. Thanks, Tony [1] https://lore.kernel.org/netdev/20221115210239.3a1c05ba@kernel.org/
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c index ead6d50fc0ad..619b32f4bc53 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c @@ -1204,21 +1204,16 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, struct ice_rx_flow_userdef *user) { struct ice_flow_seg_info *seg, *tun_seg; - struct device *dev = ice_pf_to_dev(pf); enum ice_fltr_ptype fltr_idx; struct ice_hw *hw = &pf->hw; bool perfect_filter; int ret; - seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL); - if (!seg) - return -ENOMEM; - - tun_seg = devm_kcalloc(dev, ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), - GFP_KERNEL); - if (!tun_seg) { - devm_kfree(dev, seg); - return -ENOMEM; + seg = kzalloc(sizeof(*seg), GFP_KERNEL); + tun_seg = kcalloc(ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), GFP_KERNEL); + if (!tun_seg || !seg) { + ret = -ENOMEM; + goto exit; } switch (fsp->flow_type & ~FLOW_EXT) { @@ -1264,7 +1259,7 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, ret = -EINVAL; } if (ret) - goto err_exit; + goto exit; /* tunnel segments are shifted up one. */ memcpy(&tun_seg[1], seg, sizeof(*seg)); @@ -1281,42 +1276,39 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp, ICE_FLOW_FLD_OFF_INVAL); } - /* add filter for outer headers */ fltr_idx = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT); + + if (perfect_filter) + set_bit(fltr_idx, hw->fdir_perfect_fltr); + else + clear_bit(fltr_idx, hw->fdir_perfect_fltr); + + /* add filter for outer headers */ ret = ice_fdir_set_hw_fltr_rule(pf, seg, fltr_idx, ICE_FD_HW_SEG_NON_TUN); - if (ret == -EEXIST) - /* Rule already exists, free memory and continue */ - devm_kfree(dev, seg); - else if (ret) + if (ret == -EEXIST) { + /* Rule already exists, free memory and count as success */ + ret = 0; + goto exit; + } else if (ret) { /* could not write filter, free memory */ - goto err_exit; + ret = -EOPNOTSUPP; + goto exit; + } /* make tunneled filter HW entries if possible */ memcpy(&tun_seg[1], seg, sizeof(*seg)); ret = ice_fdir_set_hw_fltr_rule(pf, tun_seg, fltr_idx, ICE_FD_HW_SEG_TUN); - if (ret == -EEXIST) { + if (ret == -EEXIST) /* Rule already exists, free memory and count as success */ - devm_kfree(dev, tun_seg); ret = 0; - } else if (ret) { - /* could not write tunnel filter, but outer filter exists */ - devm_kfree(dev, tun_seg); - } - if (perfect_filter) - set_bit(fltr_idx, hw->fdir_perfect_fltr); - else - clear_bit(fltr_idx, hw->fdir_perfect_fltr); +exit: + kfree(tun_seg); + kfree(seg); return ret; - -err_exit: - devm_kfree(dev, tun_seg); - devm_kfree(dev, seg); - - return -EOPNOTSUPP; } /** @@ -1914,7 +1906,9 @@ int ice_add_fdir_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd) input->comp_report = ICE_FXD_FLTR_QW0_COMP_REPORT_SW_FAIL; /* input struct is added to the HW filter list */ - ice_fdir_update_list_entry(pf, input, fsp->location); + ret = ice_fdir_update_list_entry(pf, input, fsp->location); + if (ret) + goto release_lock; ret = ice_fdir_write_all_fltr(pf, input, true); if (ret)
Fix ethtool FDIR logic to not use memory after its release. In the ice_ethtool_fdir.c file there are 2 spots where code can refer to pointers which may be missing. In the ice_cfg_fdir_xtrct_seq() function seg may be freed but even then may be still used by memcpy(&tun_seg[1], seg, sizeof(*seg)). In the ice_add_fdir_ethtool() function struct ice_fdir_fltr *input may first fail to be added via ice_fdir_update_list_entry() but then may be deleted by ice_fdir_update_list_entry. Terminate in both cases when the returned value of the previous operation is other than 0, free memory and don't use it anymore. Replace managed memory alloc with kzalloc/kfree in ice_cfg_fdir_xtrct_seq() since seg/tun_seg are used only by ice_fdir_set_hw_fltr_rule(). Reported-by: Michal Schmidt <mschmidt@redhat.com> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2208423 Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com> --- v2: extend CC list, fix freeing memory before return v3: correct typos in the commit msg --- .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 62 +++++++++---------- 1 file changed, 28 insertions(+), 34 deletions(-)