linux-kernel - Re: [PATCH net-next v2 5/6] netconsole: resume previously deactivated target

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: <aukchuzsfvztulvy4ibpfsw7srpbqm635e24azpcvnlgpmqxjm@e4mm3xoyvnu7>
Date: Wed, 24 Sep 2025 01:26:16 -0700
From: Breno Leitao <leitao@...ian.org>
To: Andre Carvalho <asantostc@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/6] netconsole: resume previously
 deactivated target

Hello Andre, 

On Tue, Sep 23, 2025 at 08:30:39PM +0100, Andre Carvalho wrote:
> On Tue, Sep 23, 2025 at 05:22:25AM -0700, Breno Leitao wrote:
> > For targets that are set by the mac address, they don't necessarily get
> > np.dev_name populated, do they?
> > 
> > I am double checking netpoll_setup(), and if
> > is_valid_ether_addr(np->dev_mac), I don't see np.dev_name being
> > populated.
> 
> I was not expecting it to be the case either, bu my understanding is that 
> np.dev_name does get populated by __netpoll_setup, which is called unconditionally 
> at the end of netpoll_setup. __netpoll_setup eventually does:
> 
> np->dev = ndev;
> strscpy(np->dev_name, ndev->name, IFNAMSIZ);
> 
> I've confirmed that for targets bound by mac, np->dev_name is empty before these
> lines but then it is correctly populated here. For targets create by name,
> np->dev_name is already correctly set prior to this. 
> Please, let me know if I'm missing something.

Thanks for confirming it. I think this might cause some semantics
confusion for the user, given it is asking it to bind to mac, and later,
netconsole is binding by dev_name.

Let's say the following case:

 1) netconsole is configured to bind to mac X which happens to be on eth0.
 2) there is a PCI downstream failure which causes a re-enumeration
 3) netconsole will get DEACTIVATED during phase 2
 4) After the re-enumeration, eth0 becomes some other and interface (not
 the one with mac X).
 5) Now you are going to bind do eth0 which is not the one with mac X.

> > Should we also compare that the mac doesn't match before returning?
> 
> Even though the above seem to work on my tests, I was not 100% sure we wanted
> to also check the dev_name when we initially bound by mac.
> I've also considered the approach below, which I think achieves what you are
> suggesting:
> 
> if (!is_broadcast_ether_addr(nt->np.dev_mac)) {
> 	if(memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN))
> 		return;
> } else if (strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ)) {
>         return;
> }
> 
> Let me know if you prefer this approach, it would allow resuming targets in case
> even if their dev_name changes.

I would prefer this approach than the current one, this would avoid the
problem above.

The other option is to always populate the mac during netpoll setup and
then always resume based on mac. This seems a more precise resume.

In this case, if the device goes to DEACTIVATED, then np.dev_mac will be
populated, and you only compare it to check if you want to resume it.