Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory regions
>>I think you get away with this on EFI because you use BYTES_TO_PAGES >>and get page-aligned memory, but I think you should probably round up >>to the next power of 2 for smaller allocations or to the next page or >>so for larger allocations. > > I think we could allocate at least e.g. 128 MiB from firmware if there is > not enough memory available in the GRUB mm. This way we would avoid frequent > calls to firmware and could satisfy requests for larger alignments. 128 MiB and 64 MiB cause some tests to fail (cannot allocate memory in echo1 or compression tests because there isn't enough free memory to get a 64MiB chunk). 32 MiB chunk size seems to work and seems fast enough. [It's a bit hard to tell because at some point in time time the powerpc machine stopped shutting down when we got to the end of the tests. oh well.] >> - After fixing that in the ieee1275 code, all_functional_test >>hangs trying to run the cmdline_cat test. I think this is from a slow >>algorithm somewhere - the grub allocator isn't exactly optimised for >>a proliferation of regions. > > Could you try the solution proposed above? Maybe it will solve problem of > frequent additions of memory to the GRUB mm. > >> - I noticed that nearly all the allocations were under 1MB. This seems >>inefficient for a trip out to firmware. So I made the ieee1275 code >>allocate at least max(4MB, (size of allocation rounded up nearest >>1MB) + 4kB). This makes the tests run with only the usual failures, >>at least on pseries with debug on... still chasing some bugs beyond >>that. > > Yeah, this is similar to what I proposed above. Though I would want to see > larger numbers tested as I said earlier. > >> - The speed impact depends on the allocation size. I'll post something >>on that tomorrow, hopefully, but larger minimum allocations work >>noticably better. >> >> - We only have 4GB max to play with because (at least) powerpc-ieee1275 >>is technically defined to be 32 bit. So I'm a bit nervous about >>further large allocations unless we have a way to release them back >>to _firmware_, not just to grub. > > Ugh... This can be difficult. I am not sure the GRUB mm is smart enough > to release memory regions if they are not used anymore by it. > >> I would think a better overall approach would be to allocate the 1/4 of >> ram when grub starts, and create a whole new interface for large slabs > > I am not very happy with allocating 1/4 of memory at start of the day. > I think allocating larger chunks of memory from firmware should be > enough to make things working as expected. Maybe the per-platform memory chunk allocator just needs to be smart enough to make sure that there is enough memory left over to load a "normal sized" kernel and initrd... although the sizes of distro images keep going up so that's going to be a bit fraught. >> of memory that are directly allocated from, and directly returned to, >> the firmware. I still would really prefer to bypass grub mm completely as described in my other mail. If we are able to give memory back to fw, we can claim 1GB chunks (on SLOF, PFW is going to be another issue) without having to worry about where we put them and if we have enough memory to load a kernel or initrd. It makes it much harder accidentally render your system unbootable. Kind regards, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
submenu fails to see variables
For some reason global variables are not seen in a submenu {} section.
Does anyone happen to know why this behavior is useful?
Pseudocode:
set var=val
menuentry "me" {
set
echo "var ${var}"
sleep 3
}
submenu "sm" {
set
echo "var ${var}"
sleep 3
}
Seen with grub 2.02 and 2.04.
Thanks,
Olaf
pgpIYPSG2tBEJ.pgp
Description: Digitale Signatur von OpenPGP
___
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
On Wed, Aug 25, 2021 at 03:31:52PM +0200, Erwan Velu wrote: > Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the support > of bigtime by adding the some features in inodes V3. > > This change extended grub_xfs_inode struct by 76 bytes but also changed the > computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE. > > Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit it's 84 > bytes. > XFS_V2_INODE_SIZE becomes 16 bytes too small. > > As a result, the data structure aren't properly aligned and generates > "attempt to read or write outside of partition" errors when trying to > read the filesystem. > > GNU GRUB version 2.11 > > grub> set debug=efi,gpt,xfs > grub> insmod part_gpt > grub> ls (hd0,gpt1)/ > partmap/gpt.c:93: Read a valid GPT header > partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125 > fs/xfs.c:931: Reading sb > fs/xfs.c:270: Validating superblock > fs/xfs.c:295: XFS v4 superblock detected > fs/xfs.c:962: Reading root ino 128 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:515: Reading inode (739521961424144223) - 344365866970255880, > 3840 > error: attempt to read or write outside of partition. > > This commit change the XFS_V2_INODE_SIZE computation by substracting 76 > instead of 92 from the actual size of grub_xfs_inode. > This 76 value is coming from the added : > 20 uint8 unused5 >1 uint64 flags2 > 48 uint8 unused6 > > This patch explicit the split between the v2 and v3 parts of structure. > The unused4 is still ending to the v2 structures and the v3 starts at unused5. > This will avoid future corruption of v2 or v3. > > The XFS_V2_INODE_SIZE is returning to its expected size and the > filesystem is back to a readable state. > > GNU GRUB version 2.11 > > grub> set debug=efi,gpt,xfs > grub> insmod part_gpt > grub> ls (hd0,gpt1)/ > partmap/gpt.c:93: Read a valid GPT header > partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125 > fs/xfs.c:931: Reading sb > fs/xfs.c:270: Validating superblock > fs/xfs.c:295: XFS v4 superblock detected > fs/xfs.c:962: Reading root ino 128 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:931: Reading sb > fs/xfs.c:270: Validating superblock > fs/xfs.c:295: XFS v4 superblock detected > fs/xfs.c:962: Reading root ino 128 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:515: Reading inode (128) - 64, 0 > fs/xfs.c:515: Reading inode (131) - 64, 768 > efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0 > grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024 > grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816 > grub> > > Signed-off-by: Erwan Velu Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: submenu fails to see variables
Le lun. 6 sept. 2021 à 12:49, Olaf Hering a écrit :
> For some reason global variables are not seen in a submenu {} section.
> Does anyone happen to know why this behavior is useful?
>
You need to export variable to make it visible in submenu
>
> Pseudocode:
>
> set var=val
> menuentry "me" {
> set
> echo "var ${var}"
> sleep 3
> }
>
> submenu "sm" {
> set
> echo "var ${var}"
> sleep 3
> }
>
> Seen with grub 2.02 and 2.04.
>
>
> Thanks,
> Olaf
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
___
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 Mon, 30 Aug 2021 20:02:26 +0200
Patrick Steinhardt wrote:
> 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?
This struct is meant to be used for any data passed to the crypto
backend from cryptomount. All of those members are affected by
cryptomount options. So this struct isn't about anything in particular,
just a common set of data passed to the crypto backends via
cryptomount. So I don't think two structs would improve anything here.
Am I missing something?
Glenn
___
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 Mon, 30 Aug 2021 19:55:59 +0200
Patrick Steinhardt wrote:
> 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.
I'm not opposed to this. One of my original goals was to try and not
change the function interfaces between cryptomount and the backends. I
also was originally going to have the dev->cargs stick around for the
lifetime of the dev, but I'm not remembering a good use case for that.
I'll send another series with cargs being passed as an argument.
> [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.
I see your concern, and it would be nice to have an elegant solution to
it. The capability bitfield idea seems workable. I don't think this
needs to be solved right now though. This is a problem with all
proposed approaches. I think that this *could* lead to scalability
issues, but that's likely way down the road (considering the rate at
which we're adding args to cryptomount). Also, I don't think this patch
series hampers any such solution. So I think we can punt on this for
now. Does that sound reasonable?
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
