Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler

["Laszlo Ersek" <lersek@redhat.com>]

On 06/25/20 02:21, Ni, Ray wrote:
> Laszlo,
> You are surprised because the issue is related to Redhat, or because
> the word "Redhat" should not be mentioned in the Bugzilla?

It's totally fine to mention Red Hat (with correct spelling) in the
issue description. The more details, the better.

I'm surprised that the issue *seems* specific to Red Hat. Earlier I had
looked on-and-off on the scripts that generate (for example) the Fedora
ISO images (so that they be both BIOS-bootable and UEFI-bootable), and I
don't recall any particular tricks that would exploit gray areas in
specs or so.

It's possible that the *tools* the scripts use for formatting the ISOs
have some bugs or quirks, of course. (But then I'm again a bit surprised
for those issues or quirks to be RH-specific -- I just don't see a
reason for us to diverge from the respective upstream code bases of
those tools.)

> http://manpages.ubuntu.com/manpages/bionic/man8/mkudffs.8.html
> is a tool that creates UDF file system with a compatible MBR table in the
> top 32KB specially for Windows.
> 
> Search " WHOLE DISK VS PARTITION" in the web page.

Thanks -- and, indeed, this confirms my point. The issue (or unexpected
behavior at least) is not specific to Red Hat, it is part of the (most
likely: upstream) mkudffs tool. (Case in point, the manual page you're
quoting is from an Ubuntu distro.)

I think a minimal reproducer using the mkudffs command (and maybe more
commands) would be nice in the TianoCore BZ.

Anyway, looking at the manual you've identified, it does seem like a
gray area. It's a conflict between two specifications, apparently. Or
perhaps more precisely, a bug in the UDF spec. (It seems really strange
that a filesystem insists on being installed on a complete block device,
and not on a partition described in a standard partition table.)

The workaround described in the manual is presented as a workaround "for
Windows' sake". I don't think that's fair; in my (uneducated?) opinion
*any* file system should be happy to live inside a partition. I wouldn't
"blame" Windows for making an assumption like this.

Furthermore, the technical details of the workaround are really annoying:

    So  the  whole  disk device  and  also first partition on disk
    points to same sectors.

This means that the first partition recursively includes itself -- in
other words, it includes the partition *table* that describes itself. :/

The manual then says,

    mkudffs  option --bootarea=mbr put such MBR table for compatibility
    with Microsoft Windows systems into disk when formatting.

So maybe there *is* a bug in the Red Hat ISO generator scripts; I'm not
sure. I don't see a reason why "--bootarea=mbr" should ever be used with
*removable* media (such as an ISO image). The mkudffs manual states that
this trickery is useful only for non-removable media even on Windows:

    Microsoft Windows systems are unable to detect non-removable hard
    disks without MBR or GPT partition table.  Removable disks do not
    have this restriction.

(Personally I've been using an UDF-formatted USB pendrive for a long
time now, because it supports 4G+ files, and is compatible with both
Linux and Windows. There's no partition table on it; I formatted the
pendrive myself, and I used neither a partition table nor the
"--bootarea=mbr' flag.)

Yet further, I also don't understand why this issue is being reported
only now, and not before. TianoCore#2823 does not explain what it is
exactly that fails, with RH ISOs. I've booted a whole lot of such ISOs
over time (using OVMF).

So is this -- that is, the assumed use of "--bootarea=mbr" -- a recent
issue?

Is it a regression with the ISOs?

What is the bad symptom *precisely*, when the ISO image is formatted
with "--bootarea=mbr"?

The problem description is very lacking.

Thanks,
Laszlo


> I think maybe edk2 Mbr logic should (but doesn't) skip MBR whose
> partition starts from LBA #0 (includes MBR itself).
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, June 24, 2020 7:58 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler
>>
>> On 06/24/20 10:32, Ni, Ray wrote:
>>> Zhichao,
>>> Will two BlockIo child devices cover the same range in the ISO after your patch?
>>>
>>> I don't think that's a valid behavior.
>>
>> Right; I'm surprised to find the Red Hat reference in both the commit
>> message and the BZ.
>>
>> I can't say I understand the issue at hand, but are we sure this isn't a
>> bug -- a standards conformance problem -- with the ISO in question?
>>
>> Zhichao, do you have a link to an ISO image that triggers the problem,
>> and can you provide a hexdump of the part of the image that is either
>> buggy, or triggers the problem in edk2? With a more specific issue
>> description, I might be able to find someone at Red Hat who could
>> comment with more insight than myself.
>>
>> Thanks!
>> Laszlo
>>
>>>
>>> Thanks,
>>> Ray
>>>
>>>> -----Original Message-----
>>>> From: Gao, Zhichao <zhichao.gao@intel.com>
>>>> Sent: Wednesday, June 24, 2020 1:56 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
>>>> <liming.gao@intel.com>
>>>> Subject: [PATCH] MdeModulePkg/PartitionDxe: Seperate the Udf handler
>>>>
>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2823
>>>>
>>>> Some linux iso like Redhat would contain both MBR info in the first
>>>> 512 bytes and volume info at the beginning of 32KB offset.
>>>> But the partition driver would only choose one of the GPT, MBR, and
>>>> UDF(el torito compatible) to install the partition. That would lose
>>>> one info for such linux ISO during one connect. And UDF(el torito
>>>> compatible) is not conflicted with MBR/GPT. So partition driver should
>>>> check UDF and MBR/GPT separately.
>>>>
>>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>>>> ---
>>>>  .../Universal/Disk/PartitionDxe/Partition.c   | 52 +++++++++++++------
>>>>  1 file changed, 37 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
>>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
>>>> index d1c878ad2e..562490db4f 100644
>>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
>>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c
>>>> @@ -5,7 +5,7 @@
>>>>    MBR, and GPT partition schemes are supported.
>>>>
>>>>  Copyright (c) 2018 Qualcomm Datacenter Technologies, Inc.
>>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>>> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
>>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>
>>>>  **/
>>>> @@ -39,7 +39,6 @@ EFI_DRIVER_BINDING_PROTOCOL gPartitionDriverBinding = {
>>>>  PARTITION_DETECT_ROUTINE mPartitionDetectRoutineTable[] = {
>>>>    PartitionInstallGptChildHandles,
>>>>    PartitionInstallMbrChildHandles,
>>>> -  PartitionInstallUdfChildHandles,
>>>>    NULL
>>>>  };
>>>>
>>>> @@ -189,6 +188,8 @@ PartitionDriverBindingStart (
>>>>  {
>>>>    EFI_STATUS                Status;
>>>>    EFI_STATUS                OpenStatus;
>>>> +  EFI_STATUS                UdfStatus;
>>>> +  EFI_STATUS                GptMbrStatus;
>>>>    EFI_BLOCK_IO_PROTOCOL     *BlockIo;
>>>>    EFI_BLOCK_IO2_PROTOCOL    *BlockIo2;
>>>>    EFI_DISK_IO_PROTOCOL      *DiskIo;
>>>> @@ -300,26 +301,47 @@ PartitionDriverBindingStart (
>>>>    if (BlockIo->Media->MediaPresent ||
>>>>        (BlockIo->Media->RemovableMedia && !BlockIo->Media->LogicalPartition)) {
>>>>      //
>>>> -    // Try for GPT, then legacy MBR partition types, and then UDF and El Torito.
>>>> -    // If the media supports a given partition type install child handles to
>>>> -    // represent the partitions described by the media.
>>>> +    // Try for UDF (El TOrito compatible) and GPT/MBR partition types.
>>>> +    // If the media supports a given partition type, it would install child handles
>>>> +    // to represent the partitions described by the media.
>>>> +    // Notes: GPT is conflicted with MBR. It would only install one of them to describe
>>>> +    // the partition. UDF (or El Torito) can exist with MBR, so need to check it along with
>>>> +    // GPT/MBR.
>>>>      //
>>>> +    UdfStatus = PartitionInstallUdfChildHandles (
>>>> +                  This,
>>>> +                  ControllerHandle,
>>>> +                  DiskIo,
>>>> +                  DiskIo2,
>>>> +                  BlockIo,
>>>> +                  BlockIo2,
>>>> +                  ParentDevicePath
>>>> +                  );
>>>> +
>>>>      Routine = &mPartitionDetectRoutineTable[0];
>>>>      while (*Routine != NULL) {
>>>> -      Status = (*Routine) (
>>>> -                   This,
>>>> -                   ControllerHandle,
>>>> -                   DiskIo,
>>>> -                   DiskIo2,
>>>> -                   BlockIo,
>>>> -                   BlockIo2,
>>>> -                   ParentDevicePath
>>>> -                   );
>>>> -      if (!EFI_ERROR (Status) || Status == EFI_MEDIA_CHANGED || Status == EFI_NO_MEDIA) {
>>>> +      GptMbrStatus = (*Routine) (
>>>> +                        This,
>>>> +                        ControllerHandle,
>>>> +                        DiskIo,
>>>> +                        DiskIo2,
>>>> +                        BlockIo,
>>>> +                        BlockIo2,
>>>> +                        ParentDevicePath
>>>> +                        );
>>>> +      if (!EFI_ERROR (GptMbrStatus) || GptMbrStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_NO_MEDIA) {
>>>>          break;
>>>>        }
>>>>        Routine++;
>>>>      }
>>>> +
>>>> +    if (!EFI_ERROR (UdfStatus) || !EFI_ERROR (GptMbrStatus)) {
>>>> +      Status = EFI_SUCCESS;
>>>> +    } else if (UdfStatus == EFI_MEDIA_CHANGED || GptMbrStatus == EFI_MEDIA_CHANGED) {
>>>> +      Status = EFI_MEDIA_CHANGED;
>>>> +    } else if (UdfStatus == EFI_NO_MEDIA || GptMbrStatus == EFI_NO_MEDIA) {
>>>> +      Status = EFI_NO_MEDIA;
>>>> +    }
>>>>    }
>>>>    //
>>>>    // In the case that the driver is already started (OpenStatus == EFI_ALREADY_STARTED),
>>>> --
>>>> 2.21.0.windows.1
>>>
>>>
>>> 
>>>
>