Re: [PATCH v8 1/1] plainmount: Support plain encryption mode

2023-01-14 Thread Maxim Fomin 
--- Original Message ---
On Tuesday, January 10th, 2023 at 6:19 PM, Glenn Washburn 
 wrote:
>
> On Wed, 28 Dec 2022 18:05:11 +
> Maxim Fomin ma...@fomin.one wrote:
>
> > --- Original Message ---
> > On Saturday, December 24th, 2022 at 2:09 AM, Glenn Washburn
> > developm...@efficientek.com wrote:
> >
> > > On Fri, 23 Dec 2022 19:54:47 -0600
> > > Glenn Washburn developm...@efficientek.com wrote:
> > >
> > > > On Fri, 02 Dec 2022 17:11:23 +
> > > > Maxim Fomin ma...@fomin.one wrote:
> > > >
> > > > > --- Original Message ---
> > > > > On Friday, December 2nd, 2022 at 0:00, Glenn Washburn
> > > > > developm...@efficientek.com wrote:
> > > > >
> > > > > > I'm now compiling this patch and found a few compile issues
> > > > > > below. You're compile testing this right?
> > > > >
> > > > > First versions of the patch were tested in pure grub src
> > > > > directory. Later I switched to directly making and installing
> > > > > GRUB package for my distro using its source script syntax. It
> > > > > seems this process was affected by environment options which
> > > > > hided these errors/warnings.
> > > > >
> > > > > I test the patch on my two old laptops - one with UEFI BIOS
> > > > > (x86_64-efi) and one is pre-UEFI (i386-pc). I was compiling
> > > > > i386-pc target too, because otherwise the second laptop was
> > > > > unbootable. During i386-pc compilation I noticed some warnings
> > > > > related to 'PRIuGRUB_XXX' macros which were absent during efi
> > > > > target compilation. I noticed that there are similar warnings
> > > > > in other modules and decided that there are issues with
> > > > > 'PRIuGRUB_XXX' macros at i386-pc platform at global level. In
> > > > > any case, these issues didn't cause compilation fail in my
> > > > > working environment because I would not be able to compile and
> > > > > boot pre-UEFI lap. Do you use -Werror?
> > > >
> > > > I didn't see this until just now. In case you're still
> > > > interested, no I don't use -Werror or any special compiler flags.
> > > > And I'm using gcc version 10.2.1 from a Debian 11 container.
> > >
> > > Correction, -Werror is being used. Perhaps that's a default compiler
> > > flag on Debian systems.
> > >
> > > Glenn
> >
> > This explains why you have found these issues. However, it does not
> > explain how you can compile grub with -Werror because currently there
> > are following warnings in x86_64-efi mode:
> > grub-core/lib/libgcrypt-grub/mpi/mpi-internal.h:150:24: warning:
> > variable ‘_ql’ set but not used [-Wunused-but-set-variable]
> > grub-core/lib/libgcrypt-grub/mpi/mpih-div.c:53:9: warning: variable
> > ‘dummy’ set but not used [-Wunused-but-set-variable]
>
>
> Ok I looked at this a little more, I'm getting these warnings when
> compiling the speedtest module. They are not being treated as errors.
> By default GRUB will use -Werror when building the target, unless
> --disable-werror is specified. However, the gcc command for the
> speedtest module doesn't have -Werror but a bunch of -W* and does not
> include -Wunused-but-set-variable, which is why the compile doesn't
> error. So it seems that -Werror is being changed to constituent -W*
> options and some -W* are left out in my case (haven't found where this
> happens yet). I'm not setting any CFLAGS that might affect this, are
> you? Since, I've not seen anyone else complaining about it here, I
> suspect this is something odd about your build environment.
>
> What is your build environment? (distro, GCC version)

It has two steps. Firstly I compile GRUB sources in local git folder
to see errors and warnings. Then I make GRUB package for my linux
distro. Until October I was building GRUB package with custom archlinux
PKGBUILD which approximately follows this script:
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=grub-luks-keyfile-git

CFLAGS in local makepkg.conf were CFLAGS="-march=sandybridge -mtune=generic
-O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -Wformat 
-Werror=format-security
-fstack-clash-protection -fcf-protection"

Since October I used a local copy of this gentoo ebuild to build GRUB:
https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-boot/grub/grub-.ebuild

CFLAGS in make.conf is set to "-O2 -pipe -march=sandybridge"

In both local scripts src uri was changed from mainstream to local git repo
which hosted plainmount commits. Both scripts enable --disable-Werror
configure option, which means several warnings (including 'libgcrypt-grub/mpi/')
were ignored.

gcc -v on current build environment:

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-pc-linux-gnu/12/lto-wrapper
Target: x86_64-pc-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 12.2.1 20221231 (Gentoo 12.2.1_p20221231 p8)

> > When I was working with the patch earlier this year I remember having
> > these and several more warnings which prevented me from using
> > -Werror. Back then I have rem

Re: [PATCH v3 1/2] mm: Adjust new region size to take management overhead into account

2023-01-14 Thread Zhang Boyang 

Hi,

On 2023/1/12 21:52, Daniel Kiper wrote:

On Thu, Dec 22, 2022 at 05:18:23PM +0800, Zhang Boyang wrote:

When grub_memalign() encounters out-of-memory, it will try
grub_mm_add_region_fn() to request more memory from system firmware.
However, the size passed to it doesn't take region management overhead
into account. Adding a memory area of "size" bytes may result in a heap
region of less than "size" bytes truely avaliable. Thus, the new region
may not be adequate for current allocation request, confusing
out-of-memory handling code.

This patch introduces GRUB_MM_MGMT_OVERHEAD to address the region
management overhead (e.g. metadata, padding). The value of this new
constant must be large enough to make sure grub_malloc(size) always
success after a successful call to grub_mm_init_region(addr, size +
GRUB_MM_MGMT_OVERHEAD), for any given addr and size (assuming no
interger overflow).

The size passed to grub_mm_add_region_fn() is now set to "size + align +
GRUB_MM_MGMT_OVERHEAD", thus if grub_mm_add_region_fn() succeeded,


This comment is not in line with the code...



Fixed in v4.


current allocation request can always success.

Signed-off-by: Zhang Boyang 
---
  grub-core/kern/mm.c | 54 +++--
  1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index ae2279133..5c0e5bbbf 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -83,6 +83,43 @@

  

+/*
+ * GRUB_MM_MGMT_OVERHEAD is an upper bound of management overhead of
+ * each region, with any possible padding taken into account.
+ *
+ * The value must be large enough to make sure grub_malloc(size) always
+ * success after a successful call to
+ * grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD), for any given
+ * addr and size (assuming no interger overflow).
+ *
+ * The worst case which has maximum overhead is shown in the figure below:
+ *
+ * +-- addr
+ * v   |<-   size   ->|
+ * +-+++--+-+
+ * | padding | grub_mm_region | grub_mm_header | usable bytes | padding |
+ * +-+++--+-+
+ * |<-  a  ->|<- b  ->|<- c  ->|<-d ->|<-  e  ->|
+ * ^
+ *addr % GRUB_MM_ALIGN == 1+--/ This will be the pointer


s/addr % GRUB_MM_ALIGN == 1/assuming addr % GRUB_MM_ALIGN == 1/



Fixed in v4.


It took me some time to understand what do you mean by that.


+ *a == GRUB_MM_ALIGN - 1  | returned by next
+ *| grub_malloc(size),
+ *b == sizeof (struct grub_mm_region) | if no other suitable free
+ *c == sizeof (struct grub_mm_header) \ block is available.
+ *
+ *size % GRUB_MM_ALIGN == 1


Ditto...


+ *d == size
+ *e == GRUB_MM_ALIGN - 1
+ *
+ * Therefore, the maximum overhead is:
+ *a + b + c + e == (GRUB_MM_ALIGN - 1) + sizeof (struct grub_mm_region)
+ * + sizeof (struct grub_mm_header) + (GRUB_MM_ALIGN - 1)
+ */


Thank you for this comment! It clarifies a lot...


+#define GRUB_MM_MGMT_OVERHEAD ((GRUB_MM_ALIGN - 1) \
+  + sizeof (struct grub_mm_region) \
+  + sizeof (struct grub_mm_header) \
+  + (GRUB_MM_ALIGN - 1))


It is correct from math POV but the value may not be very friendly to
the CPU/MMU. Please look below for more details...


+
  grub_mm_region_t grub_mm_base;
  grub_mm_add_region_func_t grub_mm_add_region_fn;

@@ -230,6 +267,11 @@ grub_mm_init_region (void *addr, grub_size_t size)

grub_dprintf ("regions", "No: considering a new region at %p of size %" PRIxGRUB_SIZE 
"\n",
addr, size);
+  /*
+   * If you want to modify the code below, please also take a look at
+   * GRUB_MM_MGMT_OVERHEAD and make sure it is synchronized with the code.
+   */
+
/* Allocate a region from the head.  */
r = (grub_mm_region_t) ALIGN_UP ((grub_addr_t) addr, GRUB_MM_ALIGN);

@@ -410,6 +452,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
  {
grub_mm_region_t r;
grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
+  grub_size_t grow;
int count = 0;

if (!grub_mm_base)
@@ -424,6 +467,13 @@ grub_memalign (grub_size_t align, grub_size_t size)
if ((size + align) > ~(grub_size_t) 0x10)
  goto fail;

+  /*
+   * Pre-calculate the size of heap growth (if applicable),
+   * with region management overhead taken into account.
+   */
+  if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, &grow))
+goto fail;


This should go before "if ((size + align) > ~(grub_size_t) 0x10)"
and then you should replace "size" with "grow". Additionally, as I said
above due to GRUB_MM_MGMT_OVERHEAD value final "grow" value may not be
very CPU/MMU frien

[PATCH] f2fs: fix off-by-one error in nat journal entries check

2023-01-14 Thread Daniel Axtens 
Oops. You're allowed to have up to n = NAT_JOURNAL_ENTRIES entries
_inclusive_, because the loop below uses i < n, not i <= n. D'oh.

Fixes: 4bd9877f6216 ("fs/f2fs: Do not read past the end of nat journal entries")
Reported-by: программист нект 
Tested-by: программист нект 
Signed-off-by: Daniel Axtens 
---
 grub-core/fs/f2fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
index df6beb544cbd..855e24618c2b 100644
--- a/grub-core/fs/f2fs.c
+++ b/grub-core/fs/f2fs.c
@@ -650,7 +650,7 @@ get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, 
grub_uint32_t nid,
   grub_uint16_t n = grub_le_to_cpu16 (data->nat_j.n_nats);
   grub_uint16_t i;
 
-  if (n >= NAT_JOURNAL_ENTRIES)
+  if (n > NAT_JOURNAL_ENTRIES)
 return grub_error (GRUB_ERR_BAD_FS,
"invalid number of nat journal entries");
 
-- 
2.25.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v4 0/2] mm: Better handling of adding new regions

2023-01-14 Thread Zhang Boyang 
Hi,

This is the V4 patchset.

V3 is at:
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00271.html

V2 is at:
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00164.html

V1 is at:
https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00147.html

For changes in V3->V4, please see my reply at:
https://lists.gnu.org/archive/html/grub-devel/2023-01/msg00099.html

Best Regards,
Zhang Boyang


Zhang Boyang (2):
  mm: Adjust new region size to take management overhead into account
  mm: Preallocate some space when adding new regions

 grub-core/kern/mm.c | 70 +++--
 1 file changed, 67 insertions(+), 3 deletions(-)

-- 
2.30.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v4 2/2] mm: Preallocate some space when adding new regions

2023-01-14 Thread Zhang Boyang 
When grub_memalign() encounters out-of-memory, it will try
grub_mm_add_region_fn() to request more memory from system firmware.
However, it doesn't preallocate memory space for future allocation
requests. In extreme cases, it requires one call to
grub_mm_add_region_fn() for each memory allocation request. This can be
very slow.

This patch introduces GRUB_MM_HEAP_GROW_EXTRA, the minimal heap growth
granularity. The new region size is now set to the bigger one of its
original value and GRUB_MM_HEAP_GROW_EXTRA. Thus, it will result in some
memory space preallocated if current allocations request is small.

The value of GRUB_MM_HEAP_GROW_EXTRA is set to 1MB. If this value is
smaller, the cost of small memory allocations will be higher. If this
value is larger, more memory will be wasted and it might cause
out-of-memory on machines with small amount of RAM.

Signed-off-by: Zhang Boyang 
---
 grub-core/kern/mm.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index 278756dea..4e220bbff 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -123,6 +123,9 @@
 /* The size passed to grub_mm_add_region_fn() is aligned up by this value. */
 #define GRUB_MM_HEAP_GROW_ALIGN4096
 
+/* Minimal heap growth granularity when existing heap space is exhausted. */
+#define GRUB_MM_HEAP_GROW_EXTRA0x10
+
 grub_mm_region_t grub_mm_base;
 grub_mm_add_region_func_t grub_mm_add_region_fn;
 
@@ -471,6 +474,9 @@ grub_memalign (grub_size_t align, grub_size_t size)
   if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, &grow))
 goto fail;
 
+  /* Preallocate some extra space if heap growth is small. */
+  grow = grub_max (grow, GRUB_MM_HEAP_GROW_EXTRA);
+
   /* Align up heap growth to make it friendly to CPU/MMU. */
   if (grow > ~(grub_size_t) (GRUB_MM_HEAP_GROW_ALIGN - 1))
 goto fail;
-- 
2.30.2


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v4 1/2] mm: Adjust new region size to take management overhead into account

2023-01-14 Thread Zhang Boyang 
When grub_memalign() encounters out-of-memory, it will try
grub_mm_add_region_fn() to request more memory from system firmware.
However, the size passed to it doesn't take region management overhead
into account. Adding a memory area of "size" bytes may result in a heap
region of less than "size" bytes truely avaliable. Thus, the new region
may not be adequate for current allocation request, confusing
out-of-memory handling code.

This patch introduces GRUB_MM_MGMT_OVERHEAD to address the region
management overhead (e.g. metadata, padding). The value of this new
constant must be large enough to make sure grub_malloc(size) always
success after a successful call to grub_mm_init_region(addr, size +
GRUB_MM_MGMT_OVERHEAD), for any given addr and size (assuming no
interger overflow).

The size passed to grub_mm_add_region_fn() is now correctly adjusted,
thus if grub_mm_add_region_fn() succeeded, current allocation request
can always success.

Signed-off-by: Zhang Boyang 
---
 grub-core/kern/mm.c | 64 ++---
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index ae2279133..278756dea 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -83,6 +83,46 @@
 
 
 
+/*
+ * GRUB_MM_MGMT_OVERHEAD is an upper bound of management overhead of
+ * each region, with any possible padding taken into account.
+ *
+ * The value must be large enough to make sure grub_malloc(size) always
+ * success after a successful call to
+ * grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD), for any given
+ * addr and size (assuming no interger overflow).
+ *
+ * The worst case which has maximum overhead is shown in the figure below:
+ *
+ * +-- addr
+ * v   |<-   size   ->|
+ * +-+++--+-+
+ * | padding | grub_mm_region | grub_mm_header | usable bytes | padding |
+ * +-+++--+-+
+ * |<-  a  ->|<- b  ->|<- c  ->|<-d ->|<-  e  ->|
+ * ^
+ *b == sizeof (struct grub_mm_region)  +--/ This will be the pointer
+ *c == sizeof (struct grub_mm_header) | returned by next
+ *d == size   | grub_malloc(size),
+ *| if no other suitable free
+ * Assuming addr % GRUB_MM_ALIGN == 1, then   \ block is available.
+ *a == GRUB_MM_ALIGN - 1
+ *
+ * Assuming size % GRUB_MM_ALIGN == 1, then
+ *e == GRUB_MM_ALIGN - 1
+ *
+ * Therefore, the maximum overhead is:
+ *a + b + c + e == (GRUB_MM_ALIGN - 1) + sizeof (struct grub_mm_region)
+ * + sizeof (struct grub_mm_header) + (GRUB_MM_ALIGN - 1)
+ */
+#define GRUB_MM_MGMT_OVERHEAD  ((GRUB_MM_ALIGN - 1) \
++ sizeof (struct grub_mm_region) \
++ sizeof (struct grub_mm_header) \
++ (GRUB_MM_ALIGN - 1))
+
+/* The size passed to grub_mm_add_region_fn() is aligned up by this value. */
+#define GRUB_MM_HEAP_GROW_ALIGN4096
+
 grub_mm_region_t grub_mm_base;
 grub_mm_add_region_func_t grub_mm_add_region_fn;
 
@@ -230,6 +270,11 @@ grub_mm_init_region (void *addr, grub_size_t size)
 
   grub_dprintf ("regions", "No: considering a new region at %p of size %" 
PRIxGRUB_SIZE "\n",
addr, size);
+  /*
+   * If you want to modify the code below, please also take a look at
+   * GRUB_MM_MGMT_OVERHEAD and make sure it is synchronized with the code.
+   */
+
   /* Allocate a region from the head.  */
   r = (grub_mm_region_t) ALIGN_UP ((grub_addr_t) addr, GRUB_MM_ALIGN);
 
@@ -410,6 +455,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
 {
   grub_mm_region_t r;
   grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
+  grub_size_t grow;
   int count = 0;
 
   if (!grub_mm_base)
@@ -418,10 +464,22 @@ grub_memalign (grub_size_t align, grub_size_t size)
   if (size > ~(grub_size_t) align)
 goto fail;
 
+  /*
+   * Pre-calculate the necessary size of heap growth (if applicable),
+   * with region management overhead taken into account.
+   */
+  if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, &grow))
+goto fail;
+
+  /* Align up heap growth to make it friendly to CPU/MMU. */
+  if (grow > ~(grub_size_t) (GRUB_MM_HEAP_GROW_ALIGN - 1))
+goto fail;
+  grow = ALIGN_UP (grow, GRUB_MM_HEAP_GROW_ALIGN);
+
   /* We currently assume at least a 32-bit grub_size_t,
  so limiting allocations to  - 1MiB
  in name of sanity is beneficial. */
-  if ((size + align) > ~(grub_size_t) 0x10)
+  if (grow > ~(grub_size_t) 0x10)
 goto fail;
 
   align = (align >> GRUB_MM_ALIGN_LOG2);
@@ -447,7 +505,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
   count++;
 
   if (grub_mm_add_region_fn != NULL &&
-