NetBSD Problem Report #56862

From wiz@yt.nih.at  Sun Jun  5 13:15:48 2022
Return-Path: <wiz@yt.nih.at>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1DD201A921F
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  5 Jun 2022 13:15:48 +0000 (UTC)
Message-Id: <20220605131537.732931CB6AB4@yt.nih.at>
Date: Sun,  5 Jun 2022 15:15:37 +0200 (CEST)
From: Thomas Klausner <wiz@NetBSD.org>
Reply-To: Thomas Klausner <wiz@NetBSD.org>
To: gnats-bugs@NetBSD.org
Subject: boot.cfg bug with userconf
X-Send-Pr-Version: 3.95

>Number:         56862
>Category:       bin
>Synopsis:       boot.cfg bug with userconf
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Jun 05 13:20:00 +0000 2022
>Last-Modified:  Wed Jun 08 22:00:01 +0000 2022
>Originator:     Thomas Klausner
>Release:        NetBSD 9.99.97
>Organization:

>Environment:


Architecture: x86_64
Machine: amd64
>Description:
Using the following boot.cfg:

menu=Boot without nouveau:rndseed /var/db/entropy-file;userconf disable nouveau*;boot
menu=Boot old without nouveau:rndseed /var/db/entropy-file;userconf disable nouveau*;boot /netbsd.old
menu=Boot normally:rndseed /var/db/entropy-file;boot
menu=Boot single user:rndseed /var/db/entropy-file;boot -s
menu=Drop to boot prompt:prompt
default=1
timeout=5
clear=1

when I choose to boot the third entry, it still disables nouveau.
RVP found:

Looks like a bug when a bare `boot' is encountered. Work around it by
forcing a kernel filename:

--- boot.cfg.orig       2022-06-05 00:48:51.476790000 +0000
+++ boot.cfg    2022-06-05 00:49:18.797459000 +0000
@@ -1,6 +1,6 @@
-menu=Boot without nouveau:rndseed /var/db/entropy-file;userconf disable nouveau*;boot
+menu=Boot without nouveau:rndseed /var/db/entropy-file;userconf disable nouveau*;boot /netbsd
 menu=Boot old without nouveau:rndseed /var/db/entropy-file;userconf disable nouveau*;boot /netbsd.old
-menu=Boot normally:rndseed /var/db/entropy-file;boot
+menu=Boot normally:rndseed /var/db/entropy-file;boot /netbsd
 menu=Boot single user:rndseed /var/db/entropy-file;boot -s
 menu=Drop to boot prompt:prompt
 default=1

and provided this (workaround?) patch:

diff -urN sys/arch/i386/stand/efiboot.orig/boot.c sys/arch/i386/stand/efiboot/boot.c
--- sys/arch/i386/stand/efiboot.orig/boot.c     2021-09-07 11:41:31.000000000 +0000
+++ sys/arch/i386/stand/efiboot/boot.c  2022-06-05 06:50:39.139514564 +0000
@@ -453,8 +453,10 @@
        } else {
                int i;

+#if 0
                if (howto == 0)
                        bootdefault();
+#endif
                for (i = 0; i < NUMNAMES; i++) {
                        bootit(names[i][0], howto);
                        bootit(names[i][1], howto);

>How-To-Repeat:
choose '3' using the above boot.cfg file
>Fix:
Please verify if the fix above is correct.

>Audit-Trail:
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56862: boot.cfg bug with userconf
Date: Sun, 5 Jun 2022 20:41:03 +0300

 On Sun, Jun 05, 2022 at 13:20:00 +0000, Thomas Klausner wrote:

 > Looks like a bug when a bare `boot' is encountered. Work around it
 > by forcing a kernel filename:

 This is not a bug, this is a (mis)feature.  See PR 53128 that RVP also
 mentioned.  To quote my comment from that PR:

  This seems to have been introduced in:

  sys/arch/i386/stand/boot/boot2.c

  revision 1.59
  date: 2013-07-28 12:50:09 +0400;  author: he;  state: Exp;  lines: +29 -1;
  Two changes for the i386 boot loader related to the boot menu which
  can be defined in boot.cfg:

   * Add a "menu" command which re-displays the menu and initiates
     the timed countdown
   * Use any default command defined in boot.cfg as default args
     if the user runs "boot" with no arguments

  This is useful in circumstances where you e.g. need to interrupt
  the normal boot process to switch to serial console, and where
  simply "boot netbsd" is no longer sufficient (e.g. as with install
  media which needs the miniroot kernel module loaded).


 -uwe

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: Thomas Klausner <wiz@NetBSD.org>, Valery Ushakov <uwe@stderr.spb.ru>
Subject: Re: bin/56862: boot.cfg bug with userconf
Date: Sun, 5 Jun 2022 20:59:01 +0000 (UTC)

 On Sun, 5 Jun 2022, Valery Ushakov wrote:

 >   * Use any default command defined in boot.cfg as default args
 >     if the user runs "boot" with no arguments
 >
 >  This is useful in circumstances where you e.g. need to interrupt
 >  the normal boot process to switch to serial console, and where
 >  simply "boot netbsd" is no longer sufficient (e.g. as with install
 >  media which needs the miniroot kernel module loaded).
 >

 OK, if you want to boot the default entry, then the simplest thing to
 do here is to have a special arg like "default" for the "boot" command:

 ---START---
 diff -urN sys/arch/i386/stand/efiboot.orig/boot.c sys/arch/i386/stand/efiboot/boot.c
 --- sys/arch/i386/stand/efiboot.orig/boot.c	2021-09-07 11:41:31.000000000 +0000
 +++ sys/arch/i386/stand/efiboot/boot.c	2022-06-05 20:50:41.381791596 +0000
 @@ -445,6 +445,9 @@
   	char *filename;
   	int howto;

 +	if (arg && !strcmp(arg, "default"))
 +		bootdefault();
 +
   	if (!parseboot(arg, &filename, &howto))
   		return;

 @@ -453,8 +456,6 @@
   	} else {
   		int i;

 -		if (howto == 0)
 -			bootdefault();
   		for (i = 0; i < NUMNAMES; i++) {
   			bootit(names[i][0], howto);
   			bootit(names[i][1], howto);
 ---END---

 Or, should the code be re-written to preserve current syntax?

 -RVP

From: Valery Ushakov <uwe@stderr.spb.ru>
To: RVP <rvp@SDF.ORG>
Cc: gnats-bugs@netbsd.org, Thomas Klausner <wiz@NetBSD.org>
Subject: Re: bin/56862: boot.cfg bug with userconf
Date: Mon, 6 Jun 2022 01:03:21 +0300

 On Sun, Jun 05, 2022 at 20:59:01 +0000, RVP wrote:

 > OK, if you want to boot the default entry, then the simplest thing to
 > do here is to have a special arg like "default" for the "boot" command:

 Thinking about it a bit, I guess that it complicates things without
 much benefit.  There is "menu" command (introduced in the same commit)
 that gets you back into the menu choice, so you can then select the
 entry you want.

 And to get to the boot prompt you have to make some effort (so it's
 unlikely you end up there by accident), and "help" is available

 Also, #ifndef SMALL around that original hunk was ... not thought
 through, as it doesn't just compile out some functionality, but
 changes the semantic of the command based on which boot block you use.

 So I think your original patch to to drop that hunk is the right
 course of action, but we need to find all the places - your patch
 changes sys/arch/i386/stand/efiboot/boot.c but the other PR is about
 the original change in stand/boot that was later cloned to efiboot it
 seems.

 -uwe

From: Thomas Klausner <wiz@NetBSD.org>
To: Valery Ushakov <uwe@stderr.spb.ru>
Cc: RVP <rvp@SDF.ORG>, gnats-bugs@netbsd.org
Subject: Re: bin/56862: boot.cfg bug with userconf
Date: Mon, 6 Jun 2022 15:58:37 +0200

 On Mon, Jun 06, 2022 at 01:03:21AM +0300, Valeriy E. Ushakov wrote:
 > So I think your original patch to to drop that hunk is the right
 > course of action, but we need to find all the places - your patch
 > changes sys/arch/i386/stand/efiboot/boot.c but the other PR is about
 > the original change in stand/boot that was later cloned to efiboot it
 > seems.

 I found two places that call bootdefault():

 sys/arch/i386/stand/boot/boot2.c
 493:                    bootdefault();

 sys/arch/i386/stand/efiboot/boot.c
 457:                    bootdefault();

 and two implementations:

 sys/stand/efiboot/bootmenu.c
 137:bootdefault(void)

 sys/arch/i386/stand/lib/bootmenu.c
 132:bootdefault(void)

  Thomas

From: "Thomas Klausner" <wiz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56862 CVS commit: src/sys/arch/i386/stand
Date: Wed, 8 Jun 2022 21:43:45 +0000

 Module Name:	src
 Committed By:	wiz
 Date:		Wed Jun  8 21:43:45 UTC 2022

 Modified Files:
 	src/sys/arch/i386/stand/boot: boot2.c
 	src/sys/arch/i386/stand/efiboot: boot.c

 Log Message:
 Do not use default entry's parameters for for plain "boot" command

 Go back to the "menu" instead of you want that.

 Patch from RVP in PR 56862, ok uwe@


 To generate a diff of this commit:
 cvs rdiff -u -r1.78 -r1.79 src/sys/arch/i386/stand/boot/boot2.c
 cvs rdiff -u -r1.20 -r1.21 src/sys/arch/i386/stand/efiboot/boot.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Thomas Klausner" <wiz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56862 CVS commit: src/sys
Date: Wed, 8 Jun 2022 21:55:51 +0000

 Module Name:	src
 Committed By:	wiz
 Date:		Wed Jun  8 21:55:51 UTC 2022

 Modified Files:
 	src/sys/arch/i386/stand/lib: bootmenu.c bootmenu.h
 	src/sys/stand/efiboot: bootmenu.c bootmenu.h

 Log Message:
 Remove now unused bootdefault() function.

 Part of PR 56862.


 To generate a diff of this commit:
 cvs rdiff -u -r1.17 -r1.18 src/sys/arch/i386/stand/lib/bootmenu.c
 cvs rdiff -u -r1.6 -r1.7 src/sys/arch/i386/stand/lib/bootmenu.h
 cvs rdiff -u -r1.4 -r1.5 src/sys/stand/efiboot/bootmenu.c
 cvs rdiff -u -r1.1 -r1.2 src/sys/stand/efiboot/bootmenu.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: Valery Ushakov <uwe@stderr.spb.ru>, Thomas Klausner <wiz@NetBSD.org>
Subject: Re: bin/56862: boot.cfg bug with userconf
Date: Wed, 8 Jun 2022 21:59:01 +0000 (UTC)

 On Sun, 5 Jun 2022, Valery Ushakov wrote:

 > Also, #ifndef SMALL around that original hunk was ... not thought
 > through, as it doesn't just compile out some functionality, but
 > changes the semantic of the command based on which boot block you use.
 >
 > So I think your original patch to to drop that hunk is the right
 > course of action, but we need to find all the places - your patch
 > changes sys/arch/i386/stand/efiboot/boot.c but the other PR is about
 > the original change in stand/boot that was later cloned to efiboot it
 > seems.
 >

 There is another problem here: if we have a boot.cfg like
 this:

 ```
 menu=Boot normally:gop 0;rndseed /var/db/entropy-file; userconf disable alc*;boot
 menu=Boot single user:rndseed /var/db/entropy-file;boot -s
 menu=Boot TEST:gop 0;rndseed /var/db/entropy-file;boot NAME=NetBSD_test:
 menu=Boot TEST single user:rndseed /var/db/entropy-file;boot NAME=NetBSD_test: -s
 menu=Drop to boot prompt:prompt
 default=1
 timeout=10
 clear=1
 userconf=disable i915drmkms*
 ```

 then if item 1 is selected, 2 devices (i915drmkms* & alc*) are
 disabled. Fine so far. But, if item 1 fails for some reason, and
 user selects item 3, then this too boots with alc* disabled, which
 is not intuitive.

 It should be pretty easy to "roll-back" to the global strings each
 time a menu item is re-entered, either by keeping separate global
 and per-menu linked lists or by adding a "marker" to denote the end
 of the global strings in the current single list. Can't test this
 until the weekend, though...

 On Mon, 6 Jun 2022, Thomas Klausner wrote:

 > I found two places that call bootdefault():
 > 
 > sys/arch/i386/stand/boot/boot2.c
 > 493:                    bootdefault();
 > 
 > sys/arch/i386/stand/efiboot/boot.c
 > 457:                    bootdefault();
 > 
 > and two implementations:
 > 
 > sys/stand/efiboot/bootmenu.c
 > 137:bootdefault(void)
 > 
 > sys/arch/i386/stand/lib/bootmenu.c
 > 132:bootdefault(void)
 >

 Yep. That looks like what I got.

 -RVP

>Unformatted:

Home	PR Database Search