Re: Resurrecting the UEFI Watchdog
Dear Erwan, Am 27.08.21 um 17:23 schrieb Erwan Velu: I'd like to share again a patch that got posted a long while ago here and never seen to get attention on it: https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html I found this patch useful as it allows my servers to reboot automatically if grub is unable to boots within a defined time. Yes, some of my servers don't have the hardware bios watchdog. I can share here the patch is working fine on the infra I'm working on, using it for 8 months in production. 50K servers are using it with success. I get the patch downstream but think it would have its place upstream too. If anyone wants to get a look at it, I'd be happy to help. Please repost it to the mailing list with the maintainers in Cc. Kind regards, Paul ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Resurrecting the UEFI Watchdog
As per Paul Menzel email, I'm adding the 4 maintainers in CC of this original email. I can offer to refresh & resent this patch to the mailing list if this helps its adoption. Erwan, Le ven. 27 août 2021 à 17:23, Erwan Velu a écrit : > Hey list, > > I'd like to share again a patch that got posted a long while ago here and > never seen to get attention on it: > https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html > > I found this patch useful as it allows my servers to reboot automatically > if grub is unable to boots within a defined time. Yes, some of my servers > don't have the hardware bios watchdog. > > I can share here the patch is working fine on the infra I'm working on, > using it for 8 months in production. 50K servers are using it with success. > > I get the patch downstream but think it would have its place upstream too. > > If anyone wants to get a look at it, I'd be happy to help. > > Erwan, > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock
Good day list, Le jeu. 26 août 2021 à 15:26, Carlos Maiolino a écrit : > [..] > > Thanks for spotting this! > > I'm adding the maintainers in CC. Carlos who commit the patch I'm fixing, agreed on the content. This patch is important as it prevents the 2.06 version to turn XFS v4 system into an unbootable state. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] autogen.sh: Detect python
Hi Daniel, > On Wed, Aug 18, 2021 at 09:19:23AM +0200, Petr Vorel wrote: > > > On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote: > > > > On Fri, Aug 06, 2021 at 08:45:08AM +0200, Petr Vorel wrote: > > > > > It help to avoid error on distros which has only python3 binary: > > > > > ./autogen.sh: line 20: python: command not found > > > > > Using bash builtin 'command -v' to avoid requiring which as extra > > > > > dependency (usable on containers). > > > > It looks the bash dependency is not specified in the INSTALL file in > > > > "The Requirements" section. May I ask you to add it? > > > ...and python requirement is missing from the list too. Additionally, > > > please update autogen.sh python usage in the INSTALL file. > > There is Python 2.6 or later, not sure if it still works on 2.6 (which is > > EOL) > > or it requires now 2.7). > I think we should update this requirement to "Python 3.0.0 or later". I'll just note Python 3, because 1) IMHO it would not work on 3.0, first reasonable python 3 version which allowed 2 and 3 reasonably easily coexist together was IMHO 3.3, 2) no distro uses 3.0, they either use match higher 3 version (3.6 at least) or 2.7 (or 2.6 the oldest ones). > > Also "If you use a development snapshot or want to hack on GRUB you may > > need the following." is obsolete (from 2009), I'll move python, autoconf and > > automake to the list on the top. > OK... > There is also: > 3. Type `./bootstrap'. > * autogen.sh (called by bootstrap) uses python. By default the >invocation is "python", but it can be overridden by setting the >variable $PYTHON. > I think this should be updated too. +1. Kind regards, Petr ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 2/2] autogen.sh: Detect python
It help to avoid error on distros which has only python3 binary:
./autogen.sh: line 20: python: command not found
Use python3 as the default as python2 is EOL since Jan 2020, but check
also python which is on most distros if not all python2 because
code still works on python2.
Although it should not be needed keep the possibility to define PYTHON.
For detection use "command -v" which is POSIX [3] and supported on all
common shells (bash, zsh, dash, busybox sh, mksh) instead requiring
"which" as extra dependency (usable on containers).
Update INSTALL.
Signed-off-by: Petr Vorel
---
Changes v1->v2:
* update INSTALL (Require python 3, but still note python 2.6)
NOTE: 2.6 is dead most people who still use python 2 use python 2.7,
but I suggest to keep it ()
* drop python2
INSTALL| 7 +++
autogen.sh | 17 +++--
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/INSTALL b/INSTALL
index 3270743b7..bf7695e21 100644
--- a/INSTALL
+++ b/INSTALL
@@ -42,7 +42,7 @@ To build GRUB's graphical terminal (gfxterm), you need:
If you use a development snapshot or want to hack on GRUB you may
need the following.
-* Python 2.6 or later
+* Python 3 (NOTE: python 2.6 should still work, but it's not tested)
* Autoconf 2.63 or later
* Automake 1.11 or later
@@ -87,9 +87,8 @@ The simplest way to compile this package is:
3. Type `./bootstrap'.
- * autogen.sh (called by bootstrap) uses python. By default the
- invocation is "python", but it can be overridden by setting the
- variable $PYTHON.
+* autogen.sh (called by bootstrap) uses python. By default autodetect
+ it, but it can be overridden by setting the $PYTHON variable.
4. Type `./configure' to configure the package for your system.
If you're using `csh' on an old version of System V, you might
diff --git a/autogen.sh b/autogen.sh
index 31b0ced7e..81df21816 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -7,8 +7,21 @@ if [ ! -e grub-core/lib/gnulib/stdlib.in.h ]; then
exit 1
fi
-# Set ${PYTHON} to plain 'python' if not set already
-: ${PYTHON:=python}
+# Detect python
+if [ -z "$PYTHON" ]; then
+ for i in python3 python; do
+if command -v "$i" > /dev/null 2>&1; then
+ PYTHON="$i"
+ echo "Using $PYTHON" >&2
+ break
+fi
+ done
+
+ if [ -z "$PYTHON" ]; then
+echo "python not found" >&2
+exit 1
+ fi
+fi
export LC_COLLATE=C
unset LC_ALL
--
2.33.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 1/2] bootstrap: INSTALL: Require GNU patch
bootstrap.conf uses patch, let's require it. Better than multiple messages: ./bootstrap.conf: line 84: patch: command not found Mention it also in INSTALL. Signed-off-by: Petr Vorel --- Changes v1->v2: * mention patch in INSTALL INSTALL| 1 + bootstrap.conf | 1 + 2 files changed, 2 insertions(+) diff --git a/INSTALL b/INSTALL index 79a0af7d9..3270743b7 100644 --- a/INSTALL +++ b/INSTALL @@ -20,6 +20,7 @@ configuring the GRUB. * GNU binutils 2.9.1.0.23 or later * Flex 2.5.35 or later * pkg-config +* GNU patch * Other standard GNU/Unix tools * a libc with large file support (e.g. glibc 2.1 or later) diff --git a/bootstrap.conf b/bootstrap.conf index 6b043fc35..0dd893c5c 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -70,6 +70,7 @@ autoconf 2.63 automake 1.11 gettext0.18.3 git1.5.5 +patch - tar- " -- 2.33.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Resurrecting the UEFI Watchdog
Dear Erwan, Am 30.08.21 um 11:14 schrieb Erwan Velu: As per Paul Menzel email, I'm adding the 4 maintainers in CC of this original email. I am sorry for the misunderstanding. *it* referred to the patch. I can offer to refresh & resent this patch to the mailing list if this helps its adoption. Yes, that is what I meant and would be preferred. Kind regards, Paul ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Resurrecting the UEFI Watchdog
I'll do it, thx ! Le lun. 30 août 2021 à 12:02, Paul Menzel a écrit : > Dear Erwan, > > > Am 30.08.21 um 11:14 schrieb Erwan Velu: > > As per Paul Menzel email, I'm adding the 4 maintainers in CC of this > > original email. > > I am sorry for the misunderstanding. *it* referred to the patch. > > > I can offer to refresh & resent this patch to the mailing list if this > > helps its adoption. > > Yes, that is what I meant and would be preferred. > > > Kind regards, > > Paul > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock
Hi. On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote: >Good day list, >Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiol...@redhat.com> >a écrit : > > [..] > Thanks for spotting this! > >I'm adding the maintainers in CC. Carlos who commit the patch I'm >fixing, agreed on the content. I didn't test the patch itself yet, but I've reproduced the issue. I was quite sure I had tested this patch on a V4 fs, but looks like I miscalculated the sizing. Thanks again. I'll try to test the patch here asap. >This patch is important as it prevents the 2.06 version to turn XFS v4 >system into an unbootable state. > > References > >1. mailto:cmaiol...@redhat.com -- Carlos ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] kern/efi: Adding efi-watchdog command
This patch got written by Arthur Mesh from Juniper (now at Apple Sec team).
It was extracted from
https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html
Since this email, the this patch was :
- rebased against the current tree
- added a default timeout value
This commit adds a new command efi-watchdog to manage efi watchdogs.
On server platforms, this allow grub to reset the host automatically
if it wasn't able to succeed at booting in a given timeframe.
This usually covers the following issues :
- net boot is too slow and never ends
- grub is unable to find a proper configuration and fails
If EFI_WATCHDOG_MINUTES is set a compile time, this enable the watchdog
behavior at the early init of GRUB meaning that even if grub is not able
to load its configuration file, the watchdog will be triggered
automatically.
Please note that watchdog only covers GRUB itself.
Additional hardware watchdog are required if some want to protect the early
operating system loading phase.
By default grub disable the watchdog and so this patch.
Therefore, this commit have no impact on grub's behavior.
This patch is used in production for month on a 50K server platform with
success.
Signed-off-by: Erwan Velu
---
docs/grub.texi| 15 ++
grub-core/kern/efi/init.c | 63 ++-
include/grub/efi/efi.h| 2 ++
3 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index f8b4b3b21a7f..b52161a19b3b 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3991,6 +3991,7 @@ you forget a command, you can run the command
@command{help}
* distrust::Remove a pubkey from trusted keys
* drivemap::Map a drive to another
* echo::Display a line of text
+* efi-watchdog::Manipulate EFI watchdog
* eval::Evaluate agruments as GRUB commands
* export:: Export an environment variable
* false:: Do nothing, unsuccessfully
@@ -4442,6 +4443,20 @@ When interpreting backslash escapes, backslash followed
by any other
character will print that character.
@end deffn
+@node efi-watchdog
+@subsection efi-watchdog
+
+@deffn Command efi-watchdog @option{enable}|@option{disable} code timeout
+Enable or disable the system's watchdog timer.
+Only available in EFI targeted GRUB.
+
+If @option{enable} is used, the @var{code} is logged upon
+watchdog timeout event. The UEFI BIOS reserves codes 0x to 0x.
+The @var{timeout} represents number of seconds to set the watchdog timeout to.
+When the watchdog exceed the timeout, the system is reset.
+
+if @option{disable} is used, the EFI watchdog is disarmed.
+@end deffn
@node eval
@subsection eval
diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 7facacf09c7b..c8cda3854ce7 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -28,6 +28,8 @@
#include
#include
#include
+#include
+#include
#ifdef GRUB_STACK_PROTECTOR
@@ -82,6 +84,60 @@ stack_protector_init (void)
grub_addr_t grub_modbase;
+static grub_command_t cmd_list;
+
+static grub_err_t
+grub_cmd_efi_watchdog (grub_command_t cmd __attribute__ ((unused)),
+ int argc, char **args)
+{
+long input;
+grub_efi_status_t status;
+grub_efi_uintn_t timeout;
+grub_efi_uint64_t code;
+
+if (argc < 1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("usage: efi-watchdog (enable|disable) "));
+
+if (grub_strcasecmp (args[0], "enable") == 0) {
+
+ if (argc != 3)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("usage: efi-watchdog enable
"));
+
+ input = grub_strtol (args[1], 0, 0);
+
+ if (input >= 0) {
+ code = input;
+ } else {
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_(" must be non-negative"));
+ }
+
+} else if (grub_strcasecmp (args[0], "disable") == 0) {
+
+ if (argc != 1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("usage: efi-watchdog disable"));
+ timeout = 0;
+ code = 0;
+
+} else {
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("usage: efi-watchdog (enable|disable) "));
+}
+
+status = efi_call_4
(grub_efi_system_table->boot_services->set_watchdog_timer,
+timeout, code, sizeof(L"GRUB"), L"GRUB");
+
+if (status != GRUB_EFI_SUCCESS)
+ return grub_error (GRUB_ERR_BUG,
+ N_("Unexpected UEFI SetWatchdogTimer() error"));
+else
+ return GRUB_ERR_NONE;
+}
+
+
void
grub_efi_init (void)
{
@@ -105,10 +161,14 @@ grub_efi_init (void)
grub_shim_lock_verifier_setup ();
}
+ grub_printf("grub %s: Arming EFI watchdog at %d minutes\n", PACKAGE_VERSION,
EFI_WATCHDOG_MINUTES);
efi_call_4 (gru
Re: [PATCH 00/12] Grub-shell improvements
CC-ing Denis and Patrick... On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote: > Hi Daniel, > > On Thu, 26 Aug 2021 20:00:32 +0200 > Daniel Kiper wrote: > > > Hi Glenn, > > > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote: > > > Hi Daniel, > > > > > > What are the chances this patch series can be reviewed in the near > > > future? Some feedback would be greatly appreciated. > > > > I can see the following patches from you waiting in my review queue: > > - [CRYPTO-LUKS v1 00/19] Fixes and improvements for > > cryptodisks+luks2 and a few other things. > > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html > > - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount > > https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html > > - [PATCH 0/5] Testing improvements > > https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html > > - [CI 00/17] Gitlab CI and test framework improvements > > https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html > > - [PROCFS 0/5] Add and improve (proc) entries > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html > > - [PATCH 0/4] Various LUKS2 improvements > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html > > - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html > > - [PATCH] fs: Allow number of blocks in block list to be optional, > > defaulting length to device length > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html > > - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a > > dash-insensitive manner > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html > > - [PATCH] command: Add silent mode to read command to suppress > > input echo > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > > - [PATCH 0/2] Allow overriding commands > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html > > - [PATCH 00/12] Grub-shell improvements > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html > > - [PATCH v2 0/8] Various fixes/improvements for tests > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html > > - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto > > modules > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html > > > > Please sort them in the order of importance/preference/... Then I > > will be looking at them (more or less) in that order, one patch set > > at a time. > > > > I hope I did not miss any of your patches. > > As far as I can tell, this is the full list. Great! > My order preference is as follows: > > These two patches are only first because it should be a quick review. > - [PATCH] command: Add silent mode to read command to suppress > input echo > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > - [PATCH] fs: Allow number of blocks in block list to be optional, > defaulting length to device length > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html OK. > This is a high priority because I think it should be merged before the > keyfile and detached header support patch series. If this is merged, > I'll submit and updated keyfile and detached header patch series that > works with this patch series. > - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto > modules > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html Denis, Patrick, are you both OK with this? > Based on discussion with Thomas Schmitt, I think the first patch of > this series should be dropped. Should I make a v3, or would you review > it and if acceptable drop the first patch? > - [PATCH v2 0/8] Various fixes/improvements for tests > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html No, you do not need to repost. I will take a look at v2 and ignore first patch in it. > Then these in this order. > - [PATCH 00/12] Grub-shell improvements > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html > - [PATCH 0/4] Various LUKS2 improvements > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html > - [PROCFS 0/5] Add and improve (proc) entries > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html > - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a > dash-insensitive manner > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html > - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html > - [PATCH 0/2] Allow overriding commands > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html > > > Patch series with de
Re: [PATCH 00/12] Grub-shell improvements
On Mon, 30 Aug 2021 17:23:44 +0200 Daniel Kiper wrote: > CC-ing Denis and Patrick... > > On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote: > > Hi Daniel, > > > > On Thu, 26 Aug 2021 20:00:32 +0200 > > Daniel Kiper wrote: > > > > > Hi Glenn, > > > > > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote: > > > > Hi Daniel, > > > > > > > > What are the chances this patch series can be reviewed in the > > > > near future? Some feedback would be greatly appreciated. > > > > > > I can see the following patches from you waiting in my review > > > queue: > > > - [CRYPTO-LUKS v1 00/19] Fixes and improvements for > > > cryptodisks+luks2 and a few other things. > > > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html > > > - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount > > > https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html > > > - [PATCH 0/5] Testing improvements > > > https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html > > > - [CI 00/17] Gitlab CI and test framework improvements > > > https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html > > > - [PROCFS 0/5] Add and improve (proc) entries > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html > > > - [PATCH 0/4] Various LUKS2 improvements > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html > > > - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html > > > - [PATCH] fs: Allow number of blocks in block list to be > > > optional, defaulting length to device length > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html > > > - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a > > > dash-insensitive manner > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html > > > - [PATCH] command: Add silent mode to read command to suppress > > > input echo > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > > > - [PATCH 0/2] Allow overriding commands > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html > > > - [PATCH 00/12] Grub-shell improvements > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html > > > - [PATCH v2 0/8] Various fixes/improvements for tests > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html > > > - [PATCH 0/3] Refactor/improve cryptomount data passing to > > > crypto modules > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html > > > > > > Please sort them in the order of importance/preference/... Then I > > > will be looking at them (more or less) in that order, one patch > > > set at a time. > > > > > > I hope I did not miss any of your patches. > > > > As far as I can tell, this is the full list. > > Great! > > > My order preference is as follows: > > > > These two patches are only first because it should be a quick > > review. > > - [PATCH] command: Add silent mode to read command to suppress > > input echo > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > > - [PATCH] fs: Allow number of blocks in block list to be optional, > > defaulting length to device length > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html > > OK. > > > This is a high priority because I think it should be merged before > > the keyfile and detached header support patch series. If this is > > merged, I'll submit and updated keyfile and detached header patch > > series that works with this patch series. > > - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto > > modules > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html > > Denis, Patrick, are you both OK with this? I'm OK with it. The "Refactor/improve cryptomount data passing to crypto modules" looks way cleaner than what we had before: it can scale better than the previous design because it's more generic, it can be extended more easily, and we can have more fine grained communication between the cryptodisk and the backends. Glenn Washburn wrote: > My intention is for this patch series to lay the foundation for an > improved patch series providing detached header and keyfile support > (I already have the series updated and ready to send once this is > accepted). Thanks a lot for that work and for taking care of the patch serie I sent. Denis. pgpbZv1PXowJY.pgp Description: OpenPGP digital signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 00/12] Grub-shell improvements
On Mon, Aug 30, 2021 at 06:33:15PM +0200, Denis 'GNUtoo' Carikli wrote: > On Mon, 30 Aug 2021 17:23:44 +0200 > Daniel Kiper wrote: > > > CC-ing Denis and Patrick... > > > > On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote: > > > Hi Daniel, > > > > > > On Thu, 26 Aug 2021 20:00:32 +0200 > > > Daniel Kiper wrote: > > > > > > > Hi Glenn, > > > > > > > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote: > > > > > Hi Daniel, > > > > > > > > > > What are the chances this patch series can be reviewed in the > > > > > near future? Some feedback would be greatly appreciated. > > > > > > > > I can see the following patches from you waiting in my review > > > > queue: > > > > - [CRYPTO-LUKS v1 00/19] Fixes and improvements for > > > > cryptodisks+luks2 and a few other things. > > > > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html > > > > - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount > > > > https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html > > > > - [PATCH 0/5] Testing improvements > > > > https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html > > > > - [CI 00/17] Gitlab CI and test framework improvements > > > > https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html > > > > - [PROCFS 0/5] Add and improve (proc) entries > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html > > > > - [PATCH 0/4] Various LUKS2 improvements > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html > > > > - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html > > > > - [PATCH] fs: Allow number of blocks in block list to be > > > > optional, defaulting length to device length > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html > > > > - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a > > > > dash-insensitive manner > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html > > > > - [PATCH] command: Add silent mode to read command to suppress > > > > input echo > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > > > > - [PATCH 0/2] Allow overriding commands > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html > > > > - [PATCH 00/12] Grub-shell improvements > > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html > > > > - [PATCH v2 0/8] Various fixes/improvements for tests > > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html > > > > - [PATCH 0/3] Refactor/improve cryptomount data passing to > > > > crypto modules > > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html > > > > > > > > Please sort them in the order of importance/preference/... Then I > > > > will be looking at them (more or less) in that order, one patch > > > > set at a time. > > > > > > > > I hope I did not miss any of your patches. > > > > > > As far as I can tell, this is the full list. > > > > Great! > > > > > My order preference is as follows: > > > > > > These two patches are only first because it should be a quick > > > review. > > > - [PATCH] command: Add silent mode to read command to suppress > > > input echo > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html > > > - [PATCH] fs: Allow number of blocks in block list to be optional, > > > defaulting length to device length > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html > > > > OK. > > > > > This is a high priority because I think it should be merged before > > > the keyfile and detached header support patch series. If this is > > > merged, I'll submit and updated keyfile and detached header patch > > > series that works with this patch series. > > > - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto > > > modules > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html > > > > Denis, Patrick, are you both OK with this? > I'm OK with it. > > The "Refactor/improve cryptomount data passing to crypto modules" looks > way cleaner than what we had before: it can scale better than the > previous design because it's more generic, it can be extended more > easily, and we can have more fine grained communication between the > cryptodisk and the backends. Agreed, this has been a pain point in the current architecture. Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/6] Runtime allocation of memory regions
On Fri, Aug 27, 2021 at 01:39:05PM +1000, Daniel Axtens wrote: > Daniel Kiper writes: > > > Hey, > > > > Adding Daniel Axtens... > > > > On Sun, Aug 15, 2021 at 01:09:06PM +0200, Patrick Steinhardt wrote: > >> Hi, > >> > >> this is the third version of my patch set to implement runtime > >> allocation of additional memory regions. > >> > >> Changes compared to v2: > >> > >> - A new preparatory patch was added to remove unused code which > >> unloaded modules on OOM. > >> > >> - Patch 2/4 has been split up into two patches: one to drop the > >> logic where we request a quarter of available memory and then > >> put bounds to it, and one to split apart request of additional > >> regions and initialization of the EFI MM system. > >> > >> - Flags are now `unsigned int` instead of `unsigned`. > >> > >> - `add_memory_regions ()` now gets all flags instead of only a > >> single flag `consecutive`. > >> > >> - Flags are now defines and not an enum anymore. > >> > >> - The callback function is now called `grub_mm_add_region_func_t` > >> instead of `grub_mm_region_func_t`. Flags and its variable have > >> been renamed accordingly. > >> > >> Patrick > >> > >> Patrick Steinhardt (6): > >> mm: Drop unused unloading of modules on OOM > >> mm: Allow dynamically requesting additional memory regions > >> efi: mm: Always request a fixed number of pages on init > >> efi: mm: Extract function to add memory regions > >> efi: mm: Pass up errors from `add_memory_regions ()` > >> efi: mm: Implement runtime addition of pages > >> > >> grub-core/kern/dl.c | 20 -- > >> grub-core/kern/efi/mm.c | 83 +++-- > >> grub-core/kern/mm.c | 12 +++--- > >> include/grub/dl.h | 1 - > >> include/grub/mm.h | 16 > >> 5 files changed, 61 insertions(+), 71 deletions(-) > > > > Patrick, I went quickly through this patch series and in general it > > LGTM. There are some minor issues but we can fix them later. Thank > > you for doing this work. > > > > Stefan and/or Daniel Axtens, may I ask you to test these patches with > > your use case? If it works for you please repost this patch series with > > your changes added. Then I will merge it after final review. > > Sure, I'll have a look. > > My initial thoughts are: > > - with the CAS support patch. We would still need that, and would want >to do that call early as possible because it will cause the partition >to be rebooted. > > - We have to be careful where we ask for memory because the kernel >assumes that there will be some free memory below a particular address. > > - I'd also want to verify what the performance impact would be - not > just on powerpc-ieee1275 but also on efi - of going out to > OpenFirmware/UEFI for each new zone... Good point! I was thinking about performance at some point too. Sadly I forgot to say something about that... Patrick, did you compare performance before and after you patch series? If the impact is significant maybe we should not change initial allocation strategy and add a function to allocate more memory during runtime only... > I'll test this early next week and report back. Cool! Thanks a lot! Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
>
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 31 ++-
> grub-core/disk/geli.c | 4
> grub-core/disk/luks.c | 4
> grub-core/disk/luks2.c | 4
> include/grub/cryptodisk.h | 8
> 5 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..b966b19ab 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> /* TRANSLATORS: It's still restricted to cryptodisks only. */
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> +{"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> }
>
> static grub_err_t
> -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> +grub_cryptodisk_scan_device_real (const char *name,
> + grub_disk_t source,
> + grub_cryptomount_args_t cargs)
> {
>grub_err_t err;
>grub_cryptodisk_t dev;
> @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char *name,
> grub_disk_t source)
> if (!dev)
>continue;
>
> +*dev->cargs = *cargs;
> err = cr->recover_key (source, dev);
> +*dev->cargs = NULL;
> if (err)
> {
>cryptodisk_close (dev);
Is there any specific reason why you choose to set `cargs` as a struct
field? Seeing uses like this makes me wonder whether it wouldn't be
better to pass in `cargs` as explicit arguments whenever required. This
would also automatically answer any lifetime questions which arise.
[snip]
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 13103ea6a..e2a4a3bf5 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
>grub_size_t max_stripes = 1;
>char *tmp;
>
> + /* Keyfiles are not implemented yet */
> + if (dev->cargs->key_data || dev->cargs->key_len)
> + return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>if (err)
> return err;
Hum. This makes me wonder whether we'll have to adjust all backends
whenever we add a new parameter to `cargs` to return `NOT_IMPLEMENTED`.
I fear that it won't scale nicely, and that it is a recipe for passing
unsupported arguments to backends even though they don't know to handle
them.
Not sure about a nice alternative though. The only idea I have right now
is something like a capabilities bitfield exposed by backends: only if a
specific bit is set will we pass the corresponding field to such a
backend. This would allow us to easily centralize the logic into a
single function which only receives both the capabilities bitset and the
`cargs` struct.
Other than that I really like where this is going.
Patrick
signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct
On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 26 +-
> include/grub/cryptodisk.h | 3 +++
> 2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index b6cf1835d..00a671a59 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> -static int check_boot, have_it;
> -static char *search_uuid;
> -
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> {
> @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>
>FOR_CRYPTODISK_DEVS (cr)
>{
> -dev = cr->scan (source, search_uuid, check_boot);
> +dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> if (grub_errno)
>return grub_errno;
> if (!dev)
> @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>
> grub_cryptodisk_insert (dev, name, source);
>
> -have_it = 1;
> +cargs->found_uuid = 1;
>
> goto cleanup;
>}
> @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,
> const char *cheat)
>
>FOR_CRYPTODISK_DEVS (cr)
>{
> -dev = cr->scan (source, search_uuid, check_boot);
> +dev = cr->scan (source, NULL, 0);
> if (grub_errno)
>return grub_errno;
> if (!dev)
> @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
>
>if (err)
> grub_print_error ();
> - return have_it && search_uuid ? 1 : 0;
> + return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> }
>
> static grub_err_t
> @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>cargs.key_len = grub_strlen(state[3].arg);
> }
>
> - have_it = 0;
>if (state[0].set) /* uuid */
> {
>grub_cryptodisk_t dev;
> @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> return GRUB_ERR_NONE;
> }
>
> - check_boot = state[2].set;
> - search_uuid = args[0];
> + cargs.check_boot = state[2].set;
> + cargs.search_uuid = args[0];
>grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> - search_uuid = NULL;
>
> - if (!have_it)
> + if (!cargs.found_uuid)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>return GRUB_ERR_NONE;
> }
>else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> {
> - search_uuid = NULL;
> - check_boot = state[2].set;
> + cargs.check_boot = state[2].set;
>grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> - search_uuid = NULL;
>return GRUB_ERR_NONE;
> }
>else
> @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>char *disklast = NULL;
>grub_size_t len;
>
> - search_uuid = NULL;
> - check_boot = state[2].set;
> + cargs.check_boot = state[2].set;
>diskname = args[0];
>len = grub_strlen (diskname);
>if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 1070140d9..11062f43a 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -69,6 +69,9 @@ typedef gcry_err_code_t
>
> struct grub_cryptomount_args
> {
> + grub_uint32_t check_boot : 1;
> + grub_uint32_t found_uuid : 1;
> + char *search_uuid;
>grub_uint8_t *key_data;
>grub_size_t key_len;
> };
Aren't these parameters in a different scope than the key data? These
are only used for device discovery via `scan()`, while the other ones
are for decrypting the key. Do we want to split those up into two
different structs?
Patrick
signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 0/6] Runtime allocation of memory regions
On Mon, Aug 30, 2021 at 07:49:07PM +0200, Daniel Kiper wrote: > On Fri, Aug 27, 2021 at 01:39:05PM +1000, Daniel Axtens wrote: > > Daniel Kiper writes: > > > > > Hey, > > > > > > Adding Daniel Axtens... > > > > > > On Sun, Aug 15, 2021 at 01:09:06PM +0200, Patrick Steinhardt wrote: > > >> Hi, > > >> > > >> this is the third version of my patch set to implement runtime > > >> allocation of additional memory regions. > > >> > > >> Changes compared to v2: > > >> > > >> - A new preparatory patch was added to remove unused code which > > >> unloaded modules on OOM. > > >> > > >> - Patch 2/4 has been split up into two patches: one to drop the > > >> logic where we request a quarter of available memory and then > > >> put bounds to it, and one to split apart request of additional > > >> regions and initialization of the EFI MM system. > > >> > > >> - Flags are now `unsigned int` instead of `unsigned`. > > >> > > >> - `add_memory_regions ()` now gets all flags instead of only a > > >> single flag `consecutive`. > > >> > > >> - Flags are now defines and not an enum anymore. > > >> > > >> - The callback function is now called `grub_mm_add_region_func_t` > > >> instead of `grub_mm_region_func_t`. Flags and its variable have > > >> been renamed accordingly. > > >> > > >> Patrick > > >> > > >> Patrick Steinhardt (6): > > >> mm: Drop unused unloading of modules on OOM > > >> mm: Allow dynamically requesting additional memory regions > > >> efi: mm: Always request a fixed number of pages on init > > >> efi: mm: Extract function to add memory regions > > >> efi: mm: Pass up errors from `add_memory_regions ()` > > >> efi: mm: Implement runtime addition of pages > > >> > > >> grub-core/kern/dl.c | 20 -- > > >> grub-core/kern/efi/mm.c | 83 +++-- > > >> grub-core/kern/mm.c | 12 +++--- > > >> include/grub/dl.h | 1 - > > >> include/grub/mm.h | 16 > > >> 5 files changed, 61 insertions(+), 71 deletions(-) > > > > > > Patrick, I went quickly through this patch series and in general it > > > LGTM. There are some minor issues but we can fix them later. Thank > > > you for doing this work. > > > > > > Stefan and/or Daniel Axtens, may I ask you to test these patches with > > > your use case? If it works for you please repost this patch series with > > > your changes added. Then I will merge it after final review. > > > > Sure, I'll have a look. > > > > My initial thoughts are: > > > > - with the CAS support patch. We would still need that, and would want > >to do that call early as possible because it will cause the partition > >to be rebooted. > > > > - We have to be careful where we ask for memory because the kernel > >assumes that there will be some free memory below a particular address. > > > > - I'd also want to verify what the performance impact would be - not > > just on powerpc-ieee1275 but also on efi - of going out to > > OpenFirmware/UEFI for each new zone... > > Good point! I was thinking about performance at some point too. Sadly > I forgot to say something about that... Patrick, did you compare > performance before and after you patch series? If the impact is > significant maybe we should not change initial allocation strategy > and add a function to allocate more memory during runtime only... > > > I'll test this early next week and report back. > > Cool! Thanks a lot! > > Daniel The question is how to measure performance. Are there any benchmarks that result in somewhat reproducible results? Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
