linux-kernel - Re: [PATCH net-next v2 2/5] net: xilinx: axienet: Fix dangling multicast addresses

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: <6342f461-1b9b-4740-b261-384be1843f5a@linux.dev>
Date: Tue, 20 Aug 2024 10:43:10 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
 netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
 Andrew Lunn <andrew@...n.ch>, linux-arm-kernel@...ts.infradead.org,
 Michal Simek <michal.simek@....com>, Daniel Borkmann <daniel@...earbox.net>,
 linux-kernel@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v2 2/5] net: xilinx: axienet: Fix dangling
 multicast addresses

On 8/19/24 21:33, Jakub Kicinski wrote:
> On Thu, 15 Aug 2024 15:36:11 -0400 Sean Anderson wrote:
>> If a multicast address is removed but there are still some multicast
>> addresses, that address would remain programmed into the frame filter.
>> Fix this by explicitly setting the enable bit for each filter.
>> 
>> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
>> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> Reviewed-by: Simon Horman <horms@...nel.org>
> 
> Again, I'd go for net.
> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> index 0d5b300107e0..03fef656478e 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> @@ -170,6 +170,7 @@
>>  #define XAE_UAW0_OFFSET		0x00000700 /* Unicast address word 0 */
>>  #define XAE_UAW1_OFFSET		0x00000704 /* Unicast address word 1 */
>>  #define XAE_FMI_OFFSET		0x00000708 /* Filter Mask Index */
>> +#define XAE_FFE_OFFSET		0x0000070C /* Frame Filter Enable */
>>  #define XAE_AF0_OFFSET		0x00000710 /* Address Filter 0 */
>>  #define XAE_AF1_OFFSET		0x00000714 /* Address Filter 1 */
> 
> There is a conflict with current net / net-next here, because of
> 9ff2f816e2aa65ca9, you'll need to rebase / repost (which is why 
> I'm allowing myself the nit picks ;))
> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index e664611c29cf..1bcabb016ca9 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -433,7 +433,7 @@ static int netdev_set_mac_address(struct net_device *ndev, void *p)
>>   */
>>  static void axienet_set_multicast_list(struct net_device *ndev)
>>  {
>> -	int i;
>> +	int i = 0;
> 
> Consider renaming i to addr_cnt ? or addr_num ?

Well, this doesn't really have anything to do with addresses. It selects
the "Filter Index" (as named by the datasheet) so I'd rename it `filter`
if anything.

This hardware is actually really unusual since the CAM acts on the
entire first 64 bytes of the packet. So you could theoretically filter
for the source address too, but I don't know why you'd want to since all
you can do is drop the packet. Seems like a big waste of resources to
me.

--Sean