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

Message-ID: <ZsWjpuoszvApM1I0@gmail.com>
Date: Wed, 21 Aug 2024 01:21:58 -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 Tue, Aug 20, 2024 at 04:27:25PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Aug 2024 03:36:13 -0700 Breno Leitao wrote:
> > -	if (err)
> > -		goto fail;
> > +	if (!err) {
> > +		nt->enabled = true;
> > +	} else {
> > +		pr_err("Not enabling netconsole. Netpoll setup failed\n");
> > +		if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> > +			/* only fail if dynamic reconfiguration is set,
> > +			 * otherwise, keep the target in the list, but disabled.
> > +			 */
> > +			goto fail;
> > +	}
> 
> This will be better written as:
> 
> 	if (err) {
> 		/* handle err */
> 	}
> 
> 	nt->enabled = true;

I tried to do something like this, but, I was not able to come up with
something like this.

There are three cases we need to check here:

netpoll_setup returned 0:
	nt->enabled = true           				(1)
netpoll_setup returned !=0
	if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)
		continue with nt->enabled disabeld  		(2)
	if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC) is disabld:
		goto fail.					(3)


The cases are:

1) Everything is fine
2) netpoll failed, but we want to keep configfs enabled
3) netpoll failed and we don't care about configfs.

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?
	
> 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.

Let me know if I am missing something here.

Thanks for the review,
--breno