* [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool_fdir.c
@ 2023-07-11 10:04 Jedrzej Jagielski
2023-07-12 6:29 ` Leon Romanovsky
0 siblings, 1 reply; 3+ messages in thread
From: Jedrzej Jagielski @ 2023-07-11 10:04 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, anthony.l.nguyen, Jedrzej Jagielski, Michal Schmidt,
Przemek Kitszel
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.
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
v4: restore devm() approach
---
.../net/ethernet/intel/ice/ice_ethtool_fdir.c | 30 +++++++++++--------
1 file changed, 18 insertions(+), 12 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..b6308780362b 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
@@ -1281,16 +1281,25 @@ 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 err_exit;
+ } else if (ret) {
/* could not write filter, free memory */
+ ret = -EOPNOTSUPP;
goto err_exit;
+ }
/* make tunneled filter HW entries if possible */
memcpy(&tun_seg[1], seg, sizeof(*seg));
@@ -1305,18 +1314,13 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
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);
-
return ret;
err_exit:
devm_kfree(dev, tun_seg);
devm_kfree(dev, seg);
- return -EOPNOTSUPP;
+ return ret;
}
/**
@@ -1914,7 +1918,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)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool_fdir.c
2023-07-11 10:04 [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool_fdir.c Jedrzej Jagielski
@ 2023-07-12 6:29 ` Leon Romanovsky
2023-07-12 13:08 ` Jagielski, Jedrzej
0 siblings, 1 reply; 3+ messages in thread
From: Leon Romanovsky @ 2023-07-12 6:29 UTC (permalink / raw)
To: Jedrzej Jagielski
Cc: intel-wired-lan, netdev, anthony.l.nguyen, Michal Schmidt,
Przemek Kitszel
On Tue, Jul 11, 2023 at 12:04:50PM +0200, 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.
>
> 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
> v4: restore devm() approach
> ---
> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 30 +++++++++++--------
> 1 file changed, 18 insertions(+), 12 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..b6308780362b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
> @@ -1281,16 +1281,25 @@ 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);
> +
The code above is assign_bit(fltr_idx, hw->fdir_perfect_fltr, perfect_filter);
> + /* 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 err_exit;
> + } else if (ret) {
> /* could not write filter, free memory */
> + ret = -EOPNOTSUPP;
I see that original code returned -EOPNOTSUPP, but why?
Why do you rewrite return value? Why can't you return "ret" as is?
Thanks
> goto err_exit;
> + }
>
> /* make tunneled filter HW entries if possible */
> memcpy(&tun_seg[1], seg, sizeof(*seg));
> @@ -1305,18 +1314,13 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
> 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);
> -
> return ret;
>
> err_exit:
> devm_kfree(dev, tun_seg);
> devm_kfree(dev, seg);
>
> - return -EOPNOTSUPP;
> + return ret;
> }
>
> /**
> @@ -1914,7 +1918,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)
> --
> 2.31.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool_fdir.c
2023-07-12 6:29 ` Leon Romanovsky
@ 2023-07-12 13:08 ` Jagielski, Jedrzej
0 siblings, 0 replies; 3+ messages in thread
From: Jagielski, Jedrzej @ 2023-07-12 13:08 UTC (permalink / raw)
To: Leon Romanovsky
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
Nguyen, Anthony L, mschmidt, Kitszel, Przemyslaw
From: Leon Romanovsky <leon@kernel.org>
Sent: Wed, 12 July 2023 08:30
>On Tue, Jul 11, 2023 at 12:04:50PM +0200, 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.
>>
>> 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
>> v4: restore devm() approach
>> ---
>> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 30 +++++++++++--------
>> 1 file changed, 18 insertions(+), 12 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..b6308780362b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
>> @@ -1281,16 +1281,25 @@ 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);
>> +
>
>The code above is assign_bit(fltr_idx, hw->fdir_perfect_fltr, perfect_filter);
>
>> + /* 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 err_exit;
>> + } else if (ret) {
>> /* could not write filter, free memory */
>> + ret = -EOPNOTSUPP;
>
>I see that original code returned -EOPNOTSUPP, but why?
>Why do you rewrite return value? Why can't you return "ret" as is?
>
>Thanks
My intention was just to not interfere with returned value.
However i see no reason why an unchanged value shouldn't be returned
Thanks
>
>> goto err_exit;
>> + }
>>
>> /* make tunneled filter HW entries if possible */
>> memcpy(&tun_seg[1], seg, sizeof(*seg));
>> @@ -1305,18 +1314,13 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>> 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);
>> -
>> return ret;
>>
>> err_exit:
>> devm_kfree(dev, tun_seg);
>> devm_kfree(dev, seg);
>>
>> - return -EOPNOTSUPP;
>> + return ret;
>> }
>>
>> /**
>> @@ -1914,7 +1918,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)
>> --
>> 2.31.1
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-12 13:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-11 10:04 [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool_fdir.c Jedrzej Jagielski
2023-07-12 6:29 ` Leon Romanovsky
2023-07-12 13:08 ` Jagielski, Jedrzej
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).