Re: [PATCH v4 19/23] cmd: Fix memory-mapping in cmp command

Hi Quentin,

On Tue, 3 Sept 2024 at 11:42, Quentin Schulz quentin.schulz@cherry.de wrote:

Hi Simon,

On 9/2/24 12:26 AM, Simon Glass wrote:

...

This unmaps a different address from what was mapped. Fix it.

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v1)

cmd/mem.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c index 274348068c2..4d6fde28531 100644 --- a/cmd/mem.c +++ b/cmd/mem.c @@ -245,7 +245,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc, int size; int rcode = 0; const char *type;

• const void *buf1, *buf2, *base;
  

• const void *buf1, *buf2, *base, *ptr1, *ptr2;
  ulong word1, word2;  /* 64-bit if MEM_SUPPORT_64BIT_DATA */
  
  if (argc != 4)
  

@@ -270,22 +270,22 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc, bytes = size * count; base = buf1 = map_sysmem(addr1, bytes);

"base" isn't changed in the rest of the code, so we could just reuse it instead of declaring yet another variable.

...

  buf2 = map_sysmem(addr2, bytes);

We could also set ptr2 here... Allowing to avoid the diff from here to....

...

• for (ngood = 0; ngood < count; ++ngood) {
  

• for (ngood = 0, ptr1 = buf1, ptr2 = buf2; ngood < count; ++ngood) { >                   if (size == 4) {
  

•                 word1 = *(u32 *)buf1;
  

•                 word2 = *(u32 *)buf2;
  

•                 word1 = *(u32 *)ptr1;
  

•                 word2 = *(u32 *)ptr2;
          } else if (MEM_SUPPORT_64BIT_DATA && size == 8) {
  

•                 word1 = *(ulong *)buf1;
  

•                 word2 = *(ulong *)buf2;
  

•                 word1 = *(ulong *)ptr1;
  

•                 word2 = *(ulong *)ptr2;
          } else if (size == 2) {
  

•                 word1 = *(u16 *)buf1;
  

•                 word2 = *(u16 *)buf2;
  

•                 word1 = *(u16 *)ptr1;
  

•                 word2 = *(u16 *)ptr2;
          } else {
  

•                 word1 = *(u8 *)buf1;
  

•                 word2 = *(u8 *)buf2;
  

•                 word1 = *(u8 *)ptr1;
  

•                 word2 = *(u8 *)ptr2;
          }
          if (word1 != word2) {
  

•                 ulong offset = buf1 - base;
  

•                 ulong offset = ptr1 - base;
                  printf("%s at 0x%08lx (%#0*lx) != %s at 0x%08lx (%#0*lx)\n",
                          type, (ulong)(addr1 + offset), size, word1,
                          type, (ulong)(addr2 + offset), size, word2);
  

@@ -293,8 +293,8 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc, break; }

•         buf1 += size;
  

•         buf2 += size;
  

•         ptr1 += size;
  

•         ptr2 += size;
  
  

here - making the commit all the more explicit (for me this commit is basically only renaming a variable, since unmap_system doesn't appear in the git context) - by only changing:

unmap_system(buf1); unmap_system(buf2);

to

unmap_system(base); unmap_system(ptr2);

I believe?

Additionally, my linter tells me that:

buf1 += size; buf2 += size;

is undefined behavior:

arithOperationsOnVoidPointer: 'buf1' is of type 'const void *'. When using void pointers in calculations, the behaviour is undefined.

Hmmm I thought that got changed...or at least I rely on void * being taken as char * when diffing pointers.

I suggest the following:

buf1 = ((u8 *)buf1) + size; buf2 = ((u8 *)buf2) + size;

since size seems to be size in bytes?

What do you think?

Well I think you should send a patch! It seems reasonable to me.

We already have test/cmd/mem.c is this something we can augment to test the unmapping is proper too?

Yes, it would be possible to check the list of mapped pointers in sandbox, to make sure it has gone back to its original state (i.e. any new mappings in the test were deleted before the test ended).

Regards, Simon