| 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: <e9877f67-70c5-4c14-9fd6-539a58ab5262@intel.com>
Date: Wed, 13 Aug 2025 08:51:53 -0700
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
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
On 8/12/2025 9:55 PM, Paul Menzel wrote:
> 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
Both changes fix the errors. In the second change DEL/536 opcode
following ADD/535 will also result in "Transaction failed (op 536)" as
it attempt to remove an address which was already cleared by the
preceding ADD/535 opcode. I will update the description to make it clear
the change is more than improvement.
>
>> 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@...el.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?
Sure.
>
>> 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.
The assignment is fairly isolated in just this function, but if this is
the preferred style I will change it in v3 along with the old_addr
rename.
>
>> +}
>> +
>> /**
>> * 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>
Thank you,
Emil
>
>
> Kind regards,
>
> Paul
Powered by blists - more mailing lists