linux-kernel - Re: [PATCH net-next v2 3/3] netconsole: Populate dynamic entry even if netpoll fails

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: <ZscaMfmnU3HmZWm5@gmail.com>
Date: Thu, 22 Aug 2024 04:00:01 -0700
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Aijay Adams <aijay@...a.com>
Subject: Re: [PATCH net-next v2 3/3] netconsole: Populate dynamic entry even
 if netpoll fails

Hello Jakub,

On Wed, Aug 21, 2024 at 03:49:26PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Aug 2024 01:21:58 -0700 Breno Leitao wrote:
> > Another way to write this is:
> > 
> >         err = netpoll_setup(&nt->np);
> >         if (err) {
> >                 pr_err("Not enabling netconsole. Netpoll setup failed\n");
> >                 if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> >                         goto fail
> >         } else {
> >                 nt->enabled = true;
> >         }
> > 
> > is it better? Or, Is there a even better way to write this?
> 
> Yes, I think this is better! Or at least I wouldn't have made the same
> mistake reading it if it was written this way :)
> 
> > > As for the message would it be more helpful to indicate target will be
> > > disabled? Move the print after the check for dynamic and say "Netpoll
> > > setup failed, netconsole target will be disabled" ?  
> > 
> > In both cases the target will be disabled, right? In one case, it will
> > populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set),
> > otherwise it will fail completely. Either way, netconsole will be
> > disabled.
> 
> No strong feelings. I was trying to highlight that it's a single target
> that ends up being disabled "netconsole disabled" sounds like the whole
> netconsole module is completely out of commission.

That is fair, let me print the cmdline number, so, we can see something
as:

	netpoll: netconsole: local port 6666
	netpoll: netconsole: local IPv6 address 2401:db00:3120:21a9:face:0:270:0
	netpoll: netconsole: interface 'ethX'
	netpoll: netconsole: remote port 1514
	netpoll: netconsole: remote IPv6 address 2803:6080:a89c:a670::1
	netpoll: netconsole: remote ethernet address 02:90:fb:66:aa:e5
	netpoll: netconsole: ethX doesn't exist, aborting
	netconsole: Not enabling netconsole for cmdline0. Netpoll setup failed