Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: Fix memory management in ice_ethtool_fdir.c

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 7/3/23 11:59, Jagielski, Jedrzej wrote:
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Sent: Mon, 3 July 2023 10:34
>> On 7/3/23 09:44, Jedrzej Jagielski wrote:
>>> Fix ethtool FDIR logic to not use momory 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 firstly fail to be added via ice_fdir_update_list_entry() but then
>>> may be tried to being 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>
>>> ---
>>>    .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 55 ++++++++++---------
>>>    1 file changed, 28 insertions(+), 27 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..89d6a1d2e7e3 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 err_exit;
>>>    	}
>>>    
>>>    	switch (fsp->flow_type & ~FLOW_EXT) {
>>> @@ -1281,16 +1276,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));
>>> @@ -1298,25 +1302,20 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>>>    					ICE_FD_HW_SEG_TUN);
>>>    	if (ret == -EEXIST) {
>>>    		/* Rule already exists, free memory and count as success */
>>> -		devm_kfree(dev, tun_seg);
>>> +		kfree(tun_seg);
>>>    		ret = 0;
>>>    	} else if (ret) {
>>>    		/* could not write tunnel filter, but outer filter exists */
>>> -		devm_kfree(dev, tun_seg);
>>> +		kfree(tun_seg);
>>>    	}
>>>    
>>> -	if (perfect_filter)
>>> -		set_bit(fltr_idx, hw->fdir_perfect_fltr);
>>> -	else
>>> -		clear_bit(fltr_idx, hw->fdir_perfect_fltr);
>>> -
>>> -	return ret;
>>> +		return ret;
>>
>> Sorry for late report, but now you leak `seg`.
> 
> Actually i haven't changed the flow of freeing seg when
> firstly it is applied and then applying tun_seg returns error.

But, per my suggestion, you have switched from managed devm to 
kzalloc(), and now you have to kfree() explicitly here (devm would free 
it on unload).

> There is wrong indentation at the final return line,
> it will be fixed.
> 
>>
>> I would rename 'err_exit' to just 'exit', and keep all memory freeing
>> there. (That would simplify the if (ret ...) above even more.
>>
>> Remember to cc netdev and our intel Maintainer for v2.
> 
> Sure
> 
>>
>>>    
>>>    err_exit:
>>> -	devm_kfree(dev, tun_seg);
>>> -	devm_kfree(dev, seg);
>>> +	kfree(tun_seg);
>>> +	kfree(seg);
>>>    
>>> -	return -EOPNOTSUPP;
>>> +	return ret;
>>>    }
>>>    
>>>    /**
>>> @@ -1914,7 +1913,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)

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan