| 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: <31a45fb4-acb6-4eb6-9ffb-ff1be798a064@ti.com>
Date: Tue, 7 Jan 2025 16:17:35 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Paolo Abeni <pabeni@...hat.com>, Jeongjun Park <aha310510@...il.com>,
Alexander Lobakin <aleksander.lobakin@...el.com>,
Lukasz Majewski
<lukma@...x.de>, Meghana Malladi <m-malladi@...com>,
Diogo Ivo
<diogo.ivo@...mens.com>, Simon Horman <horms@...nel.org>,
Jakub Kicinski
<kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
"David S. Miller"
<davem@...emloft.net>,
Andrew Lunn <andrew+netdev@...n.ch>,
Roger Quadros
<rogerq@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
Vignesh Raghavendra
<vigneshr@...com>,
Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
Larysa Zaremba <larysa.zaremba@...el.com>
Subject: Re: [PATCH net-next v3 2/3] net: ti: icssg-prueth: Add Multicast
Filtering support for VLAN in MAC mode
Hi Paolo,
On 07/01/25 3:12 pm, Paolo Abeni wrote:
> On 1/3/25 10:20 AM, MD Danish Anwar wrote:
>> Add multicast filtering support for VLAN interfaces in dual EMAC mode
>> for ICSSG driver.
>>
>> The driver uses vlan_for_each() API to get the list of available
>> vlans. The driver then sync mc addr of vlan interface with a locally
>> mainatined list emac->vlan_mcast_list[vid] using __hw_addr_sync_multiple()
>> API.
>>
>> The driver then calls the sync / unsync callbacks and based on whether
>> the ndev is vlan or not, driver passes appropriate vid to FDB helper
>> functions.
>>
>> This commit also exports __hw_addr_sync_multiple() in order to use it
>> from the ICSSG driver.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>> ---
>> drivers/net/ethernet/ti/icssg/icssg_prueth.c | 67 ++++++++++++++++----
>> drivers/net/ethernet/ti/icssg/icssg_prueth.h | 6 ++
>> include/linux/netdevice.h | 3 +
>> net/core/dev_addr_lists.c | 7 +-
>> 4 files changed, 66 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 1663941e59e3..ed8b5a3184d6 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -472,30 +472,44 @@ const struct icss_iep_clockops prueth_iep_clockops = {
>>
>> static int icssg_prueth_add_mcast(struct net_device *ndev, const u8 *addr)
>> {
>> - struct prueth_emac *emac = netdev_priv(ndev);
>> - int port_mask = BIT(emac->port_id);
>> + struct net_device *real_dev;
>> + struct prueth_emac *emac;
>> + int port_mask;
>> + u8 vlan_id;
>>
>> - port_mask |= icssg_fdb_lookup(emac, addr, 0);
>> - icssg_fdb_add_del(emac, addr, 0, port_mask, true);
>> - icssg_vtbl_modify(emac, 0, port_mask, port_mask, true);
>> + vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
>> + real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
>> + emac = netdev_priv(real_dev);
>> +
>> + port_mask = BIT(emac->port_id) | icssg_fdb_lookup(emac, addr, vlan_id);
>> + icssg_fdb_add_del(emac, addr, vlan_id, port_mask, true);
>> + icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, true);
>>
>> return 0;
>> }
>>
>> static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
>> {
>> - struct prueth_emac *emac = netdev_priv(ndev);
>> - int port_mask = BIT(emac->port_id);
>> + struct net_device *real_dev;
>> + struct prueth_emac *emac;
>> int other_port_mask;
>> + int port_mask;
>> + u8 vlan_id;
>> +
>> + vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
>> + real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
>> + emac = netdev_priv(real_dev);
>>
>> - other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, 0);
>> + port_mask = BIT(emac->port_id);
>> + other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, vlan_id);
>>
>> - icssg_fdb_add_del(emac, addr, 0, port_mask, false);
>> - icssg_vtbl_modify(emac, 0, port_mask, port_mask, false);
>> + icssg_fdb_add_del(emac, addr, vlan_id, port_mask, false);
>> + icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, false);
>>
>> if (other_port_mask) {
>> - icssg_fdb_add_del(emac, addr, 0, other_port_mask, true);
>> - icssg_vtbl_modify(emac, 0, other_port_mask, other_port_mask, true);
>> + icssg_fdb_add_del(emac, addr, vlan_id, other_port_mask, true);
>> + icssg_vtbl_modify(emac, vlan_id, other_port_mask,
>> + other_port_mask, true);
>> }
>>
>> return 0;
>> @@ -531,6 +545,25 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
>> return 0;
>> }
>>
>> +static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
>> + void *args)
>> +{
>> + struct prueth_emac *emac = args;
>> +
>> + if (!vdev || !vid)
>> + return 0;
>> +
>> + netif_addr_lock_bh(vdev);
>> + __hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
>> + vdev->addr_len);
>> + netif_addr_unlock_bh(vdev);
>
> At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?
>
>> +
>> + __hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>> + icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>
> If so, can this function be reduced to just:
>
> __dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>
> ?
>
I don't know but for some reason __dev_mc_sync() doesn't work here. My
initial approach was to use __dev_mc_sync(vdev, sync, unsync) however it
didn't work.
When I use __dev_mc_sync() and print the vlan_id in function
icssg_prueth_add_mcast(). It always prints vlan_id as 0 implying
__dev_mc_sync from here never gets called. Whereas when using
__hw_addr_sync_dev() I see the appropriate vlan_id in
icssg_prueth_add_mcast()
Anyways, Even if I use __dev_mc_sync(), we will still need the export. I
am exporting __hw_addr_sync_multiple() not __hw_addr_sync_dev(). The API
being used by me `__hw_addr_sync_dev()` is already exported.
>> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
>> index 166e404f7c03..90716bd736f3 100644
>> --- a/net/core/dev_addr_lists.c
>> +++ b/net/core/dev_addr_lists.c
>> @@ -242,9 +242,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
>> __hw_addr_del_entry(from_list, ha, false, false);
>> }
>>
>> -static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> - struct netdev_hw_addr_list *from_list,
>> - int addr_len)
>> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> + struct netdev_hw_addr_list *from_list,
>> + int addr_len)
>> {
>> int err = 0;
>> struct netdev_hw_addr *ha, *tmp;
>> @@ -260,6 +260,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> }
>> return err;
>> }
>> +EXPORT_SYMBOL(__hw_addr_sync_multiple);
>
> I'm asking because this additional export looks suspect. How other
> drivers cope with similar situation?
>
To avoid exporting I will need to use dev_mc_sync_multiple() which was
in fact suggested by Michal Swiatkowski in [1]. However that won't work
in this case as explained by me in the reply to Michal [2]
[1]
https://lore.kernel.org/all/Z2PLDqqrLdXhLtAF@mev-dev.igk.intel.com/#:~:text=Only%20question%2C%20why%20dev_mc_sync_multiple%20can%27t%20be%20used%20here%3F
[2]
https://lore.kernel.org/all/cb319ffd-ac67-42b3-9786-e8c9970086d2@ti.com/#:~:text=nice%20to%20have.%0A%3E-,I,-don%27t%20think%20that%27s
> Thanks!
>
> Paolo
>
--
Thanks and Regards,
Danish
Powered by blists - more mailing lists