netdev - Re: [PATCH iwl-next v5] 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]

Message-ID: <ZJARN5Nj/IpdWNWQ@boxer>
Date: Mon, 19 Jun 2023 10:26:31 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, Ahmed Zaki <ahmed.zaki@...el.com>, "Michal
 Swiatkowski" <michal.swiatkowski@...ux.intel.com>, Rafal Romanowski
	<rafal.romanowski@...el.com>, Piotr Gardocki <piotrx.gardocki@...el.com>
Subject: Re: [PATCH iwl-next v5] iavf: fix err handling for MAC replace

On Mon, Jun 19, 2023 at 04:06:35AM -0400, Przemek Kitszel wrote:
> Defer removal of current primary MAC until a replacement is successfully
> added. Previous implementation would left filter list with no primary MAC.
> 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 ([1] by Piotr)
> 
> Piotr has also applied some review suggestions during our internal patch
> submittal process.
> 
> [1] https://lore.kernel.org/netdev/20230614145302.902301-2-piotrx.gardocki@intel.com/
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@...el.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

I was reviewing that:
https://lore.kernel.org/netdev/ZH8JBgiZAvNdfg4+@boxer/

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>

> ---
> v5: v4 re-sent after dependencies ([1] above) has been applied to net-next
> ---
>  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 f262487109f6..44304f16cdfa 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1040,40 +1040,36 @@ static int iavf_replace_primary_mac(struct iavf_adapter *adapter,
>  				    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.40.1
>