| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4278f944-57fe-6382-132d-728fa8c8f582@intel.com>
Date: Fri, 23 Jun 2023 15:21:30 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
<netdev@...r.kernel.org>, Michal Swiatkowski
<michal.swiatkowski@...ux.intel.com>, Michal Wilczynski
<michal.wilczynski@...el.com>, Simon Horman <simon.horman@...igine.com>,
Arpana Arland <arpanax.arland@...el.com>
Subject: Re: [PATCH net-next 4/6] ice: remove null checks before devm_kfree()
calls
On 6/23/23 13:13, Maciej Fijalkowski wrote:
> On Fri, Jun 23, 2023 at 12:44:29PM +0200, Przemek Kitszel wrote:
>> On 6/23/23 12:23, Maciej Fijalkowski wrote:
>>> On Thu, Jun 22, 2023 at 11:35:59AM -0700, Tony Nguyen wrote:
>>>> From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>>>
>>>> We all know they are redundant.
>>>
>>> Przemek,
>>>
>>> Ok, they are redundant, but could you also audit the driver if these devm_
>>> allocations could become a plain kzalloc/kfree calls?
>>
>> Olek was also motivating such audit :)
>>
>> I have some cases collected with intention to send in bulk for next window,
>> list is not exhaustive though.
>
> When rev-by count tag would be considered too much? I have a mixed
> feelings about providing yet another one, however...
>
>>
>>>
>>>>
>>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>>>> Reviewed-by: Michal Wilczynski <michal.wilczynski@...el.com>
>>>> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>>> Tested-by: Arpana Arland <arpanax.arland@...el.com> (A Contingent worker at Intel)
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>>>> ---
>>>> drivers/net/ethernet/intel/ice/ice_common.c | 6 +--
>>>> drivers/net/ethernet/intel/ice/ice_controlq.c | 3 +-
>>>> drivers/net/ethernet/intel/ice/ice_flow.c | 23 ++--------
>>>> drivers/net/ethernet/intel/ice/ice_lib.c | 42 +++++++------------
>>>> drivers/net/ethernet/intel/ice/ice_sched.c | 11 ++---
>>>> drivers/net/ethernet/intel/ice/ice_switch.c | 19 +++------
>>>> 6 files changed, 29 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
>>>> index eb2dc0983776..6acb40f3c202 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>>>> @@ -814,8 +814,7 @@ static void ice_cleanup_fltr_mgmt_struct(struct ice_hw *hw)
>>>> devm_kfree(ice_hw_to_dev(hw), lst_itr);
>>>> }
>>>> }
>>>> - if (recps[i].root_buf)
>>>> - devm_kfree(ice_hw_to_dev(hw), recps[i].root_buf);
>>>> + devm_kfree(ice_hw_to_dev(hw), recps[i].root_buf);
>>>> }
>>>> ice_rm_all_sw_replay_rule_info(hw);
>>>> devm_kfree(ice_hw_to_dev(hw), sw->recp_list);
>>>> @@ -1011,8 +1010,7 @@ static int ice_cfg_fw_log(struct ice_hw *hw, bool enable)
>>>> }
>>>> out:
>>>> - if (data)
>>>> - devm_kfree(ice_hw_to_dev(hw), data);
>>>> + devm_kfree(ice_hw_to_dev(hw), data);
>>>> return status;
>>>> }
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
>>>> index 385fd88831db..e7d2474c431c 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_controlq.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
>>>> @@ -339,8 +339,7 @@ do { \
>>>> } \
>>>> } \
>>>> /* free the buffer info list */ \
>>>> - if ((qi)->ring.cmd_buf) \
>>>> - devm_kfree(ice_hw_to_dev(hw), (qi)->ring.cmd_buf); \
>>>> + devm_kfree(ice_hw_to_dev(hw), (qi)->ring.cmd_buf); \
>>>> /* free DMA head */ \
>>>> devm_kfree(ice_hw_to_dev(hw), (qi)->ring.dma_head); \
>>>> } while (0)
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>>> index ef103e47a8dc..85cca572c22a 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>>> @@ -1303,23 +1303,6 @@ ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
>>>> return NULL;
>>>> }
>>>> -/**
>>>> - * ice_dealloc_flow_entry - Deallocate flow entry memory
>>>> - * @hw: pointer to the HW struct
>>>> - * @entry: flow entry to be removed
>>>> - */
>>>> -static void
>>>> -ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
>>>> -{
>>>> - if (!entry)
>>>> - return;
>>>> -
>>>> - if (entry->entry)
>
> ...would you be able to point me to the chunk of code that actually sets
> ice_flow_entry::entry? because from a quick glance I can't seem to find
> it.
Simon was asking very similar question [1],
albeit "where is the *check* for entry not being null?" (not set),
and it is just above the default three lines of context provided by git
(pasted below for your convenience, [3])
To answer, "where it's set?", see ice_flow_add_entry(), [2].
[1] https://lore.kernel.org/netdev/ZHb5AIgL5SzBa5FA@corigine.com/
[2]
https://elixir.bootlin.com/linux/v6.4-rc7/source/drivers/net/ethernet/intel/ice/ice_flow.c#L1632
--
BTW, is there any option to add some of patch generation options (like,
context size, anchored lines, etc), that there are my disposal locally,
but in a way, that it would not be lost after patch is applied to one
tree (Tony's) and then send again (here)?
(My assumption is that Tony is (re)generating patches from git (opposed
to copy-pasting+decorating of what he has received from me).
>
>>>> - devm_kfree(ice_hw_to_dev(hw), entry->entry);
>>>> -
>>>> - devm_kfree(ice_hw_to_dev(hw), entry);
>>>> -}
>>>> -
>>>> /**
>>>> * ice_flow_rem_entry_sync - Remove a flow entry
>>>> * @hw: pointer to the HW struct
>>>> @@ -1335,7 +1318,8 @@ ice_flow_rem_entry_sync(struct ice_hw *hw, enum ice_block __always_unused blk,
[3] More context would include following:
if (!entry)
return -EINVAL;
>>>> list_del(&entry->l_entry);
>>>> - ice_dealloc_flow_entry(hw, entry);
>>>> + devm_kfree(ice_hw_to_dev(hw), entry->entry);
>>>> + devm_kfree(ice_hw_to_dev(hw), entry);
>>>> return 0;
>>>> }
>>>> @@ -1662,8 +1646,7 @@ ice_flow_add_entry(struct ice_hw *hw, enum ice_block blk, u64 prof_id,
>>>> out:
>>>> if (status && e) {
>>>> - if (e->entry)
>>>> - devm_kfree(ice_hw_to_dev(hw), e->entry);
>>>> + devm_kfree(ice_hw_to_dev(hw), e->entry);
>>>> devm_kfree(ice_hw_to_dev(hw), e);
>>>> }
>
> (...)
Powered by blists - more mailing lists