netdev - Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 6 Jun 2023 13:59:41 +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>, Piotr Gardocki <piotrx.gardocki@...el.com>, "Michal
 Swiatkowski" <michal.swiatkowski@...ux.intel.com>, Rafal Romanowski
	<rafal.romanowski@...el.com>
Subject: Re: [PATCH net-next 2/3] iavf: fix err handling for MAC replace

On 6/6/23 12:23, Maciej Fijalkowski wrote:
> On Tue, Jun 06, 2023 at 12:14:49PM +0200, Przemek Kitszel wrote:
>> On 6/5/23 21:17, Maciej Fijalkowski wrote:
>>> On Fri, Jun 02, 2023 at 10:13:01AM -0700, Tony Nguyen wrote:
>>>> From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>>>
>>>> Defer removal of current primary MAC until a replacement is successfully added.
>>>> Previous implementation would left filter list with no primary MAC.
>>>
>>> and this opens up for what kind of issues? do you mean that
>>> iavf_add_filter() could break and existing primary filter has been marked
>>> for removal?
>>
>> Yes, prior to the patch the flow was:
>> 1. mark all MACs non-primary;
>> 2. mark current HW MAC for removal;
>> 3. try to add new MAC, say it fails, so that's an end with -ENOMEM;
>> 4. ::is_primary and ::remove fields for the ::mac_filter_list, alongside
>> with ::aq_required are left modified, to be finalized next time
>> user/watchdog processes that.
>>
>> For me it was enough to treat it as a bug, and for sure a "bad smell".
> 
> Thanks,
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> 
>>
>>
>>>
>>>> This was found while reading the code.
>>>>
>>>> The patch takes advantage of the fact that there can only be a single primary
>>>> MAC filter at any time.
>>>>
>>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@...el.com>
>>>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>>>> Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/iavf/iavf_main.c | 42 ++++++++++-----------
>>>>    1 file changed, 19 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> index 420aaca548a0..3a78f86ba4f9 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>>> @@ -1010,40 +1010,36 @@ int iavf_replace_primary_mac(struct iavf_adapter *adapter,
>>>
>>> from what i'm looking at, iavf_replace_primary_mac() could be scoped only
>>> to iavf_main.c and become static func.
>>>
>>
>> makes sense, thanks
> 
> are you going to followup on this? probably there are some more low
> hanging fruits out in iavf such as this one.

Sure, after some digging it looks like static-whats-possible warrants 
another patch ;) (will send soon as separate series)

> 
>>
>>>>    			     const u8 *new_mac)
>>>>    {
>>>>    	struct iavf_hw *hw = &adapter->hw;
>>>> -	struct iavf_mac_filter *f;
>>>> +	struct iavf_mac_filter *new_f;
>>>> +	struct iavf_mac_filter *old_f;
>>>>    	spin_lock_bh(&adapter->mac_vlan_list_lock);
>>>> -	list_for_each_entry(f, &adapter->mac_filter_list, list) {
>>>> -		f->is_primary = false;
>>>> +	new_f = iavf_add_filter(adapter, new_mac);
>>>> +	if (!new_f) {
>>>> +		spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>> +		return -ENOMEM;
>>>>    	}
>>>> -	f = iavf_find_filter(adapter, hw->mac.addr);
>>>> -	if (f) {
>>>> -		f->remove = true;
>>>> +	old_f = iavf_find_filter(adapter, hw->mac.addr);
>>>> +	if (old_f) {
>>>> +		old_f->is_primary = false;
>>>> +		old_f->remove = true;
>>>>    		adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
>>>>    	}
>>>> -
>>>> -	f = iavf_add_filter(adapter, new_mac);
>>>> -
>>>> -	if (f) {
>>>> -		/* Always send the request to add if changing primary MAC
>>>> -		 * even if filter is already present on the list
>>>> -		 */
>>>> -		f->is_primary = true;
>>>> -		f->add = true;
>>>> -		adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> -		ether_addr_copy(hw->mac.addr, new_mac);
>>>> -	}
>>>> +	/* Always send the request to add if changing primary MAC,
>>>> +	 * even if filter is already present on the list
>>>> +	 */
>>>> +	new_f->is_primary = true;
>>>> +	new_f->add = true;
>>>> +	adapter->aq_required |= IAVF_FLAG_AQ_ADD_MAC_FILTER;
>>>> +	ether_addr_copy(hw->mac.addr, new_mac);
>>>>    	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>>>>    	/* schedule the watchdog task to immediately process the request */
>>>> -	if (f) {
>>>> -		mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> -		return 0;
>>>> -	}
>>>> -	return -ENOMEM;
>>>> +	mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0);
>>>> +	return 0;
>>>>    }
>>>>    /**
>>>> -- 
>>>> 2.38.1
>>>>
>>>>
>>