Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

[Andrew Fish]

It looks to me like if you Free pages, after you free pages you hit this DEBUG message. 


https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790

      if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0)) {
        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
        return EFI_NOT_FOUND;
      }

I'm not sure I've thought out all the paths, but would it make more sense if you are trying to convert EfiConventionalMemory to EfiConventionalMemory that you are trying to free pages that are already freed. That is not very obvious from the above DEBUG print.  Could there be an if in the error path to print a better DEBUG message for a free pages bug? 

Also to be pedantic the function change names to: CoreConvertPagesEx(). 

Thanks,


Andrew Fish

Re: Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

[Zeng, Star]

Andrew,

Another path may hit this DEBUG message is AllocatePages() by AllocateAddress type.
I think it is a good suggestion to enhance the DEBUG message. How about the update like below?

-        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
+        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types, "));
+        if (Entry->Type == EfiConventionalMemory) {
+          DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to free have been freed\n"));
+        } else {
+          DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to allocate have been allocated\n"));
+        }


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrew Fish
Sent: Thursday, August 10, 2017 9:04 AM
To: edk2-devel <edk2-devel@lists.01.org>
Subject: [edk2] Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

It looks to me like if you Free pages, after you free pages you hit this DEBUG message. 


https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790

      if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0)) {
        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
        return EFI_NOT_FOUND;
      }

I'm not sure I've thought out all the paths, but would it make more sense if you are trying to convert EfiConventionalMemory to EfiConventionalMemory that you are trying to free pages that are already freed. That is not very obvious from the above DEBUG print.  Could there be an if in the error path to print a better DEBUG message for a free pages bug? 

Also to be pedantic the function change names to: CoreConvertPagesEx(). 

Thanks,


Andrew Fish



_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

[Andrew Fish]

Also I forgot to mention the double page free was in the DXE Core LoadImage() on an error path. 


CoreLoadPeImage()
...
  return EFI_SUCCESS;

Done:

  //
  // Free memory.
  //
  if (DstBufAlocated) {
    CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
  }
...

CoreUnloadAndCloseImage()
...
  if ((Image->ImageBasePage != 0) && FreePage) {
    CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
  }
...


I fixed by setting Image->ImageContext.ImageAddress and Image->ImageBasePage to 0 after the free in CoreLoadPeImage().

Bug 667 <https://bugzilla.tianocore.org/show_bug.cgi?id=667>

Thanks,

Andrew Fish


> On Aug 9, 2017, at 6:33 PM, Zeng, Star <star.zeng@intel.com> wrote:
> 
> Andrew,
> 
> Another path may hit this DEBUG message is AllocatePages() by AllocateAddress type.
> I think it is a good suggestion to enhance the DEBUG message. How about the update like below?
> 
> -        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
> +        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types, "));
> +        if (Entry->Type == EfiConventionalMemory) {
> +          DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to free have been freed\n"));
> +        } else {
> +          DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to allocate have been allocated\n"));
> +        }
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrew Fish
> Sent: Thursday, August 10, 2017 9:04 AM
> To: edk2-devel <edk2-devel@lists.01.org>
> Subject: [edk2] Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.
> 
> It looks to me like if you Free pages, after you free pages you hit this DEBUG message. 
> 
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790
> 
>      if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0)) {
>        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
>        return EFI_NOT_FOUND;
>      }
> 
> I'm not sure I've thought out all the paths, but would it make more sense if you are trying to convert EfiConventionalMemory to EfiConventionalMemory that you are trying to free pages that are already freed. That is not very obvious from the above DEBUG print.  Could there be an if in the error path to print a better DEBUG message for a free pages bug? 
> 
> Also to be pedantic the function change names to: CoreConvertPagesEx(). 
> 
> Thanks,
> 
> 
> Andrew Fish
> 
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

[Laszlo Ersek]

On 08/10/17 03:03, Andrew Fish wrote:
> It looks to me like if you Free pages, after you free pages you hit this DEBUG message. 
> 
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790
> 
>       if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0)) {
>         DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
>         return EFI_NOT_FOUND;
>       }
> 
> I'm not sure I've thought out all the paths, but would it make more sense if you are trying to convert EfiConventionalMemory to EfiConventionalMemory that you are trying to free pages that are already freed. That is not very obvious from the above DEBUG print.  Could there be an if in the error path to print a better DEBUG message for a free pages bug? 
> 
> Also to be pedantic the function change names to: CoreConvertPagesEx(). 

(Reacting only to this side question:)

I generally prefer:

  DEBUG ((
    DebugMask,
    "%a: ...",
    __FUNCTION__,
    ...
    ));

This way when the function is renamed, or the DEBUG is moved to another
function, the log will update itself.

Taking it one step further: if the DEBUG is in a widely used library
instance, then I like

  DEBUG ((
    DebugMask,
    "%a:%a: ...",
    gEfiCallerBaseName,
    __FUNCTION__,
    ...
    ));

Because this identifies the calling driver module as well (using the
BASE_NAME from the module's INF file).

("%g" with &gEfiCallerIdGuid would be more robust than "%a" with
gEfiCallerBaseName, but the log should also be readable...)

Thanks
Laszlo