netdev - Re: [PATCH net 2/3] nfp: flower: prevent re-adding mac index for bonded port

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] [day] [month] [year] [list]

Message-ID: <20240205175829.GN960600@kernel.org>
Date: Mon, 5 Feb 2024 17:58:29 +0000
From: Simon Horman <horms@...nel.org>
To: Louis Peens <louis.peens@...igine.com>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	James Hershaw <james.hershaw@...igine.com>,
	Daniel Basilio <daniel.basilio@...igine.com>,
	netdev@...r.kernel.org, stable@...r.kernel.org,
	oss-drivers@...igine.com
Subject: Re: [PATCH net 2/3] nfp: flower: prevent re-adding mac index for
 bonded port

On Mon, Feb 05, 2024 at 04:15:08PM +0200, Louis Peens wrote:
> On Mon, Feb 05, 2024 at 01:32:03PM +0000, Simon Horman wrote:
> > On Fri, Feb 02, 2024 at 01:37:18PM +0200, Louis Peens wrote:
> > > From: Daniel de Villiers <daniel.devilliers@...igine.com>
> > > 
> > > When physical ports are reset (either through link failure or manually
> > > toggled down and up again) that are slaved to a Linux bond with a tunnel
> > > endpoint IP address on the bond device, not all tunnel packets arriving
> > > on the bond port are decapped as expected.
> > > 
> > > The bond dev assigns the same MAC address to itself and each of its
> > > slaves. When toggling a slave device, the same MAC address is therefore
> > > offloaded to the NFP multiple times with different indexes.
> > > 
> > > The issue only occurs when re-adding the shared mac. The
> > > nfp_tunnel_add_shared_mac() function has a conditional check early on
> > > that checks if a mac entry already exists and if that mac entry is
> > > global: (entry && nfp_tunnel_is_mac_idx_global(entry->index)). In the
> > > case of a bonded device (For example br-ex), the mac index is obtained,
> > > and no new index is assigned.
> > > 
> > > We therefore modify the conditional in nfp_tunnel_add_shared_mac() to
> > > check if the port belongs to the LAG along with the existing checks to
> > > prevent a new global mac index from being re-assigned to the slave port.
> > > 
> > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs")
> > > CC: stable@...r.kernel.org # 5.1+
> > > Signed-off-by: Daniel de Villiers <daniel.devilliers@...igine.com>
> > > Signed-off-by: Louis Peens <louis.peens@...igine.com>
> > 
> > Hi Daniel and Louis,
> > 
> > I'd like to encourage you to update the wording of the commit message
> > to use more inclusive language; I'd suggest describing the patch
> > in terms of members of a LAG.
> Thanks Simon, this have not even crossed my mind this time and I feel
> bad - I should be more aware. Thanks for politely pointing this out.
> This did get merged earlier today as-is unfortunately, I'm not sure if
> there is a good way (or if it is pressing enough) to have it retracted.
> I will try to be more cognizant of this in the future.

Hi Louis,

thanks for acknowledging my concern.

Given that the patch has been applied,
I think it would be best to do as you suggest,
and keep this in mind for next time.