netdev - Re: [Intel-wired-lan] [PATCH iwl-net v2] idpf: set mac type when adding and removing MAC filters

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: <a154eb9b-809e-4b8c-8b77-89c80d6658e1@molgen.mpg.de>
Date: Wed, 13 Aug 2025 06:55:52 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Emil Tantilov <emil.s.tantilov@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, willemb@...gle.com, decot@...gle.com,
 netdev@...r.kernel.org, joshua.a.hay@...el.com,
 Aleksandr.Loktionov@...el.com, andrew+netdev@...n.ch, edumazet@...gle.com,
 jianliu@...hat.com, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, kuba@...nel.org, pabeni@...hat.com,
 davem@...emloft.net
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2] idpf: set mac type when
 adding and removing MAC filters

Dear Emil,


Thank you for the patch.

Am 13.08.25 um 04:42 schrieb Emil Tantilov:
> On control planes that allow changing the MAC address of the interface,
> the driver must provide a MAC type to avoid errors such as:
> 
> idpf 0000:0a:00.0: Transaction failed (op 535)
> idpf 0000:0a:00.0: Received invalid MAC filter payload (op 535) (len 0)
> idpf 0000:0a:00.0: Transaction failed (op 536)
> 
> These errors occur during driver load or when changing the MAC via:
> ip link set <iface> address <mac>
> 
> Add logic to set the MAC type when sending ADD/DEL (opcodes 535/536) to
> the control plane. Since only one primary MAC is supported per vport, the
> driver only needs to send an ADD opcode when setting it. Remove the old
> address by calling __idpf_del_mac_filter(), which skips the message and
> just clears the entry from the internal list.

Could this be split into two patches?

1.  Set the type
2.  Improve logic

> Fixes: ce1b75d0635c ("idpf: add ptypes and MAC filter support")
> Reported-by: Jian Liu <jianliu@...hat.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@...el.com>
> ---
> Changelog:
> v2:
> - Make sure to clear the primary MAC from the internal list, following
>    successful change.
> - Update the description to include the error on 536 opcode and
>    mention the removal of the old address.
> 
> v1:
> https://lore.kernel.org/intel-wired-lan/20250806192130.3197-1-emil.s.tantilov@intel.com/
> ---
>   drivers/net/ethernet/intel/idpf/idpf_lib.c      |  9 ++++++---
>   drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 11 +++++++++++
>   2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 2c2a3e85d693..26edd2cda70b 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -2345,6 +2345,7 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
>   	struct idpf_vport_config *vport_config;
>   	struct sockaddr *addr = p;
>   	struct idpf_vport *vport;
> +	u8 old_addr[ETH_ALEN];

old_mac_addr?

>   	int err = 0;
>   
>   	idpf_vport_ctrl_lock(netdev);
> @@ -2367,17 +2368,19 @@ static int idpf_set_mac(struct net_device *netdev, void *p)
>   	if (ether_addr_equal(netdev->dev_addr, addr->sa_data))
>   		goto unlock_mutex;
>   
> +	ether_addr_copy(old_addr, vport->default_mac_addr);
> +	ether_addr_copy(vport->default_mac_addr, addr->sa_data);
>   	vport_config = vport->adapter->vport_config[vport->idx];
>   	err = idpf_add_mac_filter(vport, np, addr->sa_data, false);
>   	if (err) {
>   		__idpf_del_mac_filter(vport_config, addr->sa_data);
> +		ether_addr_copy(vport->default_mac_addr, netdev->dev_addr);
>   		goto unlock_mutex;
>   	}
>   
> -	if (is_valid_ether_addr(vport->default_mac_addr))
> -		idpf_del_mac_filter(vport, np, vport->default_mac_addr, false);
> +	if (is_valid_ether_addr(old_addr))
> +		__idpf_del_mac_filter(vport_config, old_addr);
>   
> -	ether_addr_copy(vport->default_mac_addr, addr->sa_data);
>   	eth_hw_addr_set(netdev, addr->sa_data);
>   
>   unlock_mutex:
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index a028c69f7fdc..e60438633cc4 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -3765,6 +3765,15 @@ u32 idpf_get_vport_id(struct idpf_vport *vport)
>   	return le32_to_cpu(vport_msg->vport_id);
>   }
>   
> +static void idpf_set_mac_type(struct idpf_vport *vport,
> +			      struct virtchnl2_mac_addr *mac_addr)
> +{
> +	if (ether_addr_equal(vport->default_mac_addr, mac_addr->addr))
> +		mac_addr->type = VIRTCHNL2_MAC_ADDR_PRIMARY;
> +	else
> +		mac_addr->type = VIRTCHNL2_MAC_ADDR_EXTRA;

I’d use the ternary operator. That way, it’s clear the same variable is 
assigned a value in each branch.

> +}
> +
>   /**
>    * idpf_mac_filter_async_handler - Async callback for mac filters
>    * @adapter: private data struct
> @@ -3894,6 +3903,7 @@ int idpf_add_del_mac_filters(struct idpf_vport *vport,
>   			    list) {
>   		if (add && f->add) {
>   			ether_addr_copy(mac_addr[i].addr, f->macaddr);
> +			idpf_set_mac_type(vport, &mac_addr[i]);
>   			i++;
>   			f->add = false;
>   			if (i == total_filters)
> @@ -3901,6 +3911,7 @@ int idpf_add_del_mac_filters(struct idpf_vport *vport,
>   		}
>   		if (!add && f->remove) {
>   			ether_addr_copy(mac_addr[i].addr, f->macaddr);
> +			idpf_set_mac_type(vport, &mac_addr[i]);
>   			i++;
>   			f->remove = false;
>   			if (i == total_filters)

The overall diff looks good. Feel free to add:

Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>


Kind regards,

Paul