Re: [PATCH 04/19] mm: assert that we preserve header vs region alignment
On Wed, Mar 23, 2022 at 04:47:51PM +1100, Daniel Axtens wrote:
> > s/struct grub_mm_region/grub_mm_region_t/
> > s/struct grub_mm_header/grub_mm_header_t/
>
> The problem is that grub_mm_{region,header}_t is a pointer type, not a
> struct type. So sizeof (grub_mm_region_t) == sizeof(void *). You also
> can't do sizeof (*grub_mm_region_t), because you can't dereference a
> type. If there's a better way to express this I am open to it, but for
> now I will stick with the struct type...
OK.
> >> + sizeof(struct grub_mm_header)) == 0);
> >
> > I think this check is insufficient. The GRUB_MM_ALIGN should be checked
> > somehow too. E.g. if sizeof(struct grub_mm_region) == 16,
> > sizeof(struct grub_mm_header) == 8 and GRUB_MM_ALIGN == 32 then assert
> > above will pass but the GRUB will blow up later. Of course numbers are not
> > real but I chose them to show the problem. Hmmm... If we could choose
> > grub_mm_region_t and grub_mm_header_t sizes properly then probably we
> > could drop GRUB_MM_ALIGN...
> >
>
> I have added that sizeof(struct header) == GRUB_MM_ALIGN. GRUB_MM_ALIGN
> is supposed to align you to a cell, and a header is supposed to be 1
> cell.
Cool!
> We probably _could_ do away with the constant but I think that requires
> a bit more close thought.
Yeah, I think additional GRUB_MM_ALIGN check is enough right now.
It does not make sense to spent more time on this and block the
other series any longer.
Thanks,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 04/19] mm: assert that we preserve header vs region alignment
> s/struct grub_mm_region/grub_mm_region_t/
> s/struct grub_mm_header/grub_mm_header_t/
The problem is that grub_mm_{region,header}_t is a pointer type, not a
struct type. So sizeof (grub_mm_region_t) == sizeof(void *). You also
can't do sizeof (*grub_mm_region_t), because you can't dereference a
type. If there's a better way to express this I am open to it, but for
now I will stick with the struct type...
>
>> + sizeof(struct grub_mm_header)) == 0);
>
> I think this check is insufficient. The GRUB_MM_ALIGN should be checked
> somehow too. E.g. if sizeof(struct grub_mm_region) == 16,
> sizeof(struct grub_mm_header) == 8 and GRUB_MM_ALIGN == 32 then assert
> above will pass but the GRUB will blow up later. Of course numbers are not
> real but I chose them to show the problem. Hmmm... If we could choose
> grub_mm_region_t and grub_mm_header_t sizes properly then probably we
> could drop GRUB_MM_ALIGN...
>
I have added that sizeof(struct header) == GRUB_MM_ALIGN. GRUB_MM_ALIGN
is supposed to align you to a cell, and a header is supposed to be 1
cell.
We probably _could_ do away with the constant but I think that requires
a bit more close thought.
Kind regards,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 04/19] mm: assert that we preserve header vs region alignment
On Tue, Oct 12, 2021 at 06:29:53PM +1100, Daniel Axtens wrote:
> grub_mm_region_init() does:
>
> h = (grub_mm_header_t) (r + 1);
>
> where h is a grub_mm_header_t and r is a grub_mm_region_t.
>
> Cells are supposed to be GRUB_MM_ALIGN aligned, but while grub_mm_dump
> ensures this vs the region header, grub_mm_region_init() does not.
>
> It's better to be explicit than implicit here: rather than changing
> grub_mm_region_init() to ALIGN_UP(), require that the struct is
> explictly a multiple of the header size.
>
> Signed-off-by: Daniel Axtens
> ---
> include/grub/mm_private.h | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
> index e80a059dd4e4..533b47173e18 100644
> --- a/include/grub/mm_private.h
> +++ b/include/grub/mm_private.h
> @@ -20,6 +20,7 @@
> #define GRUB_MM_PRIVATE_H1
>
> #include
> +#include
>
> /* Magic words. */
> #define GRUB_MM_FREE_MAGIC 0x2d3c2808
> @@ -82,4 +83,11 @@ typedef struct grub_mm_region
> extern grub_mm_region_t EXPORT_VAR (grub_mm_base);
> #endif
>
> +static inline void grub_mm_size_sanity_check(void) {
static inline void
grub_mm_size_sanity_check (void)
{
> + /* Ensure we preserve alignment when doing h = (grub_mm_header_t) (r + 1)
> */
> + COMPILE_TIME_ASSERT((sizeof(struct grub_mm_region) %
Missing spaces before "(" above and below...
s/struct grub_mm_region/grub_mm_region_t/
s/struct grub_mm_header/grub_mm_header_t/
> +sizeof(struct grub_mm_header)) == 0);
I think this check is insufficient. The GRUB_MM_ALIGN should be checked
somehow too. E.g. if sizeof(struct grub_mm_region) == 16,
sizeof(struct grub_mm_header) == 8 and GRUB_MM_ALIGN == 32 then assert
above will pass but the GRUB will blow up later. Of course numbers are not
real but I chose them to show the problem. Hmmm... If we could choose
grub_mm_region_t and grub_mm_header_t sizes properly then probably we
could drop GRUB_MM_ALIGN...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
