[PATCH 0/3] Ext4Pkg: Fix bugs triggered by MdePkg and ShellPkg.

[PATCH 0/3] Ext4Pkg: Fix bugs triggered by MdePkg and ShellPkg.

[Pedro Falcato]

This patch-series addresses bugs found when testing the filesystem with
more complex usage of the file protocol, particularly through the shell
itself.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>


Pedro Falcato (3):
  Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
  Ext4Pkg: Hide "." and ".." entries from Read() callers.
  Ext4Pkg: Add a directory entry tree.

 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 253 ++++++++++++++++++++++----
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  71 +++++++-
 Features/Ext4Pkg/Ext4Dxe/File.c       |  47 +++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |   2 +-
 Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
 Features/Ext4Pkg/Ext4Dxe/Superblock.c |  24 ++-
 7 files changed, 345 insertions(+), 62 deletions(-)

-- 
2.33.0

[PATCH 1/3] Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.

[Pedro Falcato]

Fixes bug triggered by ShellPkg code, in usage of EFI_FILE_PROTOCOL's
Open().

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/File.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index 0f5fa6f73f..a3eff2b48a 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -207,6 +207,11 @@ Ext4Open (
 
     FileName += Length;
 
+    if (StrCmp(PathSegment, L".") == 0) {
+      // Opens of "." are a no-op
+      continue;
+    }
+
     DEBUG ((DEBUG_FS, "[ext4] Opening %s\n", PathSegment));
 
     if (!Ext4FileIsDir (Current)) {
@@ -512,12 +517,20 @@ Ext4GetFileInfo (
   IN EXT4_FILE *File, OUT EFI_FILE_INFO *Info, IN OUT UINTN *BufferSize
   )
 {
-  UINTN  FileNameLen;
-  UINTN  FileNameSize;
-  UINTN  NeededLength;
+  UINTN         FileNameLen;
+  UINTN         FileNameSize;
+  UINTN         NeededLength;
+  CONST CHAR16  *FileName;
+
+  if (File->InodeNum == 2) {
+    // Root inode gets a filename of "", regardless of how it was opened.
+    FileName = L"";
+  } else {
+    FileName = File->FileName;
+  }
 
-  FileNameLen  = StrLen (File->FileName);
-  FileNameSize = StrSize (File->FileName);
+  FileNameLen  = StrLen (FileName);
+  FileNameSize = StrSize (FileName);
 
   NeededLength = SIZE_OF_EFI_FILE_INFO + FileNameSize;
 
@@ -540,7 +553,7 @@ Ext4GetFileInfo (
 
   *BufferSize = NeededLength;
 
-  return StrCpyS (Info->FileName, FileNameLen + 1, File->FileName);
+  return StrCpyS (Info->FileName, FileNameLen + 1, FileName);
 }
 
 /**
@@ -687,6 +700,7 @@ Ext4DuplicateFile (
 {
   EXT4_PARTITION  *Partition;
   EXT4_FILE       *File;
+  EFI_STATUS      Status;
 
   Partition = Original->Partition;
   File = AllocateZeroPool (sizeof (EXT4_FILE));
@@ -717,7 +731,8 @@ Ext4DuplicateFile (
   File->InodeNum = Original->InodeNum;
   File->OpenMode = 0; // Will be filled by other code
 
-  if (!Ext4InitExtentsMap (File)) {
+  Status = Ext4InitExtentsMap (File);
+  if (EFI_ERROR (Status)) {
     FreePool (File->FileName);
     FreePool (File->Inode);
     FreePool (File);
-- 
2.33.0

[PATCH 2/3] Ext4Pkg: Hide "." and ".." entries from Read() callers.

[Pedro Falcato]

This makes it so callers that may expect FAT32 filesystems (most do)
have more normal looking ReadDir() results.
This commit also presents a better filename for files opened through
Open(".").

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c | 45 +++++++++++++++++++++++-----
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h   |  4 ++-
 Features/Ext4Pkg/Ext4Dxe/Inode.c     |  2 +-
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index 081c6bf0f4..c85c4df6d5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -180,6 +180,7 @@ Ext4RetrieveDirent (
    @param[in]      OpenMode    Mode in which the file is supposed to be open.
    @param[out]     OutFile     Pointer to the newly opened file.
    @param[in]      Entry       Directory entry to be used.
+   @param[in]      Directory   Pointer to the opened directory.
 
    @retval EFI_STATUS          Result of the operation
 **/
@@ -188,12 +189,18 @@ Ext4OpenDirent (
   IN  EXT4_PARTITION  *Partition,
   IN  UINT64          OpenMode,
   OUT EXT4_FILE       **OutFile,
-  IN  EXT4_DIR_ENTRY  *Entry
+  IN  EXT4_DIR_ENTRY  *Entry,
+  IN  EXT4_FILE       *Directory
   )
 {
   EFI_STATUS  Status;
-  CHAR16      FileName[EXT4_NAME_MAX + 1];
+  CHAR16      FileNameBuf[EXT4_NAME_MAX + 1];
   EXT4_FILE   *File;
+  CHAR16      *FileName;
+  UINTN       DestMax;
+
+  FileName = FileNameBuf;
+  DestMax  = ARRAY_SIZE (FileNameBuf);
 
   File = AllocateZeroPool (sizeof (EXT4_FILE));
 
@@ -202,12 +209,18 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  Status = Ext4GetUcs2DirentName (Entry, FileName);
+  Status = Ext4GetUcs2DirentName (Entry, FileNameBuf);
 
   if (EFI_ERROR (Status)) {
     goto Error;
   }
 
+  if (StrCmp (FileNameBuf, L".") == 0) {
+    // We're using the parent directory's name
+    FileName = Directory->FileName;
+    DestMax  = StrLen (FileName) + 1;
+  }
+
   File->FileName = AllocateZeroPool (StrSize (FileName));
 
   if (!File->FileName) {
@@ -222,7 +235,7 @@ Ext4OpenDirent (
   }
 
   // This should not fail.
-  StrCpyS (File->FileName, ARRAY_SIZE (FileName), FileName);
+  StrCpyS (File->FileName, DestMax, FileName);
 
   File->InodeNum = Entry->inode;
 
@@ -290,7 +303,7 @@ Ext4OpenFile (
     return EFI_NOT_FOUND;
   }
 
-  return Ext4OpenDirent (Partition, OpenMode, OutFile, &Entry);
+  return Ext4OpenDirent (Partition, OpenMode, OutFile, &Entry, Directory);
 }
 
 /**
@@ -429,6 +442,8 @@ Ext4ReadDir (
   UINT32          BlockRemainder;
   EXT4_DIR_ENTRY  Entry;
   EXT4_FILE       *TempFile;
+  BOOLEAN         ShouldSkip;
+  BOOLEAN         IsDotOrDotDot;
 
   DirIno     = File->Inode;
   Status     = EFI_SUCCESS;
@@ -470,13 +485,27 @@ Ext4ReadDir (
       goto Out;
     }
 
-    if (Entry.inode == 0) {
-      // When inode = 0, it's unused
+    // We don't care about passing . or .. entries to the caller of ReadDir(),
+    // since they're generally useless entries *and* may break things if too
+    // many callers assume FAT32.
+
+    // Entry.name_len may be 0 if it's a nameless entry, like an unused entry
+    // or a checksum at the end of the directory block.
+    // memcmp (and CompareMem) return 0 when the passed length is 0.
+
+    IsDotOrDotDot = Entry.name_len != 0 &&
+                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
+                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
+
+    // When inode = 0, it's unused.
+    ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
+
+    if (ShouldSkip) {
       Offset += Entry.rec_len;
       continue;
     }
 
-    Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry);
+    Status = Ext4OpenDirent (Partition, EFI_FILE_MODE_READ, &TempFile, &Entry, File);
 
     if (EFI_ERROR (Status)) {
       goto Out;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 93f0a8a04a..1aafc60ab5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -353,6 +353,7 @@ Ext4OpenFile (
    @param[in]      OpenMode    Mode in which the file is supposed to be open.
    @param[out]     OutFile     Pointer to the newly opened file.
    @param[in]      Entry       Directory entry to be used.
+   @param[in]      Directory   Pointer to the opened directory.
 
    @retval EFI_STATUS          Result of the operation
 **/
@@ -361,7 +362,8 @@ Ext4OpenDirent (
   IN  EXT4_PARTITION  *Partition,
   IN  UINT64          OpenMode,
   OUT EXT4_FILE       **OutFile,
-  IN  EXT4_DIR_ENTRY  *Entry
+  IN  EXT4_DIR_ENTRY  *Entry,
+  IN  EXT4_FILE       *Directory
   );
 
 /**
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 1bbff9e69f..982b19c763 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -89,7 +89,7 @@ Ext4Read (
   IN OUT UINTN           *Length
   )
 {
-  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(Offset %lu, Length %lu)\n", Offset, *Length));
+  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->FileName, Offset, *Length));
   EXT4_INODE   *Inode;
   UINT64       InodeSize;
   UINT64       CurrentSeek;
-- 
2.33.0

[PATCH 3/3] Ext4Pkg: Add a directory entry tree.

[Pedro Falcato]

This helps us track directories and directory entries,
which helps us getting a general idea of how the filesystem
looks. In the future, it might serve as a directory cache.
Right now, it only lets us know which name "." and ".." may refer
to, which fixes a EFI_FILE_PROTOCOL::GetInfo() bug that got triggered
by ShellPkg, in some MdePkg code.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 Features/Ext4Pkg/Ext4Dxe/Directory.c  | 238 +++++++++++++++++++++-----
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  67 +++++++-
 Features/Ext4Pkg/Ext4Dxe/File.c       |  24 +--
 Features/Ext4Pkg/Ext4Dxe/Inode.c      |   2 +-
 Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
 Features/Ext4Pkg/Ext4Dxe/Superblock.c |  24 ++-
 7 files changed, 301 insertions(+), 64 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index c85c4df6d5..7d1b2dcfe5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -194,13 +194,8 @@ Ext4OpenDirent (
   )
 {
   EFI_STATUS  Status;
-  CHAR16      FileNameBuf[EXT4_NAME_MAX + 1];
+  CHAR16      FileName[EXT4_NAME_MAX + 1];
   EXT4_FILE   *File;
-  CHAR16      *FileName;
-  UINTN       DestMax;
-
-  FileName = FileNameBuf;
-  DestMax  = ARRAY_SIZE (FileNameBuf);
 
   File = AllocateZeroPool (sizeof (EXT4_FILE));
 
@@ -209,23 +204,32 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  Status = Ext4GetUcs2DirentName (Entry, FileNameBuf);
+  Status = Ext4GetUcs2DirentName (Entry, FileName);
 
   if (EFI_ERROR (Status)) {
     goto Error;
   }
 
-  if (StrCmp (FileNameBuf, L".") == 0) {
-    // We're using the parent directory's name
-    FileName = Directory->FileName;
-    DestMax  = StrLen (FileName) + 1;
-  }
+  if (StrCmp (FileName, L".") == 0) {
+    // We're using the parent directory's dentry
+    File->Dentry = Directory->Dentry;
 
-  File->FileName = AllocateZeroPool (StrSize (FileName));
+    ASSERT (File->Dentry != NULL);
 
-  if (!File->FileName) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Error;
+    Ext4RefDentry (File->Dentry);
+  } else if (StrCmp (FileName, L"..") == 0) {
+    // Using the parent's parent's dentry
+    File->Dentry = Directory->Dentry->Parent;
+
+    ASSERT (File->Dentry != NULL);
+
+    Ext4RefDentry (File->Dentry);
+  } else {
+    File->Dentry = Ext4CreateDentry (FileName, Directory->Dentry);
+
+    if (!File->Dentry) {
+      goto Error;
+    }
   }
 
   Status = Ext4InitExtentsMap (File);
@@ -234,9 +238,6 @@ Ext4OpenDirent (
     goto Error;
   }
 
-  // This should not fail.
-  StrCpyS (File->FileName, DestMax, FileName);
-
   File->InodeNum = Entry->inode;
 
   Ext4SetupFile (File, Partition);
@@ -255,8 +256,8 @@ Ext4OpenDirent (
 
 Error:
   if (File != NULL) {
-    if (File->FileName != NULL) {
-      FreePool (File->FileName);
+    if (File->Dentry != NULL) {
+      Ext4UnrefDentry (File->Dentry);
     }
 
     if (File->ExtentsMap != NULL) {
@@ -333,52 +334,47 @@ Ext4OpenVolume (
   OUT EFI_FILE_PROTOCOL               **Root
   )
 {
-  EXT4_INODE  *RootInode;
-  EFI_STATUS  Status;
-  EXT4_FILE   *RootDir;
+  EXT4_INODE      *RootInode;
+  EFI_STATUS      Status;
+  EXT4_FILE       *RootDir;
+  EXT4_PARTITION  *Partition;
+
+  Partition = (EXT4_PARTITION *)This;
 
-  // 2 is always the root inode number in ext4
-  Status = Ext4ReadInode ((EXT4_PARTITION *)This, 2, &RootInode);
+  Status = Ext4ReadInode (Partition, EXT4_ROOT_INODE_NR, &RootInode);
 
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "[ext4] Could not open root inode - status %x\n", Status));
+    DEBUG ((DEBUG_ERROR, "[ext4] Could not open root inode - error %r\n", Status));
     return Status;
   }
 
   RootDir = AllocateZeroPool (sizeof (EXT4_FILE));
 
-  if (!RootDir) {
+  if (RootDir == NULL) {
     FreePool (RootInode);
     return EFI_OUT_OF_RESOURCES;
   }
 
-  // The filename will be "\"(null terminated of course)
-  RootDir->FileName = AllocateZeroPool (2 * sizeof (CHAR16));
-
-  if (!RootDir->FileName) {
-    FreePool (RootDir);
-    FreePool (RootInode);
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  RootDir->FileName[0] = L'\\';
-
   RootDir->Inode    = RootInode;
-  RootDir->InodeNum = 2;
+  RootDir->InodeNum = EXT4_ROOT_INODE_NR;
 
   Status = Ext4InitExtentsMap (RootDir);
 
   if (EFI_ERROR (Status)) {
-    FreePool (RootDir->FileName);
     FreePool (RootInode);
     FreePool (RootDir);
     return EFI_OUT_OF_RESOURCES;
   }
 
-  Ext4SetupFile (RootDir, (EXT4_PARTITION *)This);
+  Ext4SetupFile (RootDir, Partition);
   *Root = &RootDir->Protocol;
 
-  InsertTailList (&((EXT4_PARTITION *)This)->OpenFiles, &RootDir->OpenFilesListNode);
+  InsertTailList (&Partition->OpenFiles, &RootDir->OpenFilesListNode);
+
+  ASSERT (Partition->RootDentry != NULL);
+  RootDir->Dentry = Partition->RootDentry;
+
+  Ext4RefDentry (RootDir->Dentry);
 
   return EFI_SUCCESS;
 }
@@ -525,3 +521,159 @@ Ext4ReadDir (
 Out:
   return Status;
 }
+
+/**
+   Removes a dentry from the other's list.
+
+   @param[in out]            Parent       Pointer to the parent EXT4_DENTRY.
+   @param[in out]            ToBeRemoved  Pointer to the child EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4RemoveDentry (
+  IN OUT EXT4_DENTRY  *Parent,
+  IN OUT EXT4_DENTRY  *ToBeRemoved
+  )
+{
+  EXT4_DENTRY  *D;
+  LIST_ENTRY   *Entry;
+  LIST_ENTRY   *NextEntry;
+
+  BASE_LIST_FOR_EACH_SAFE (Entry, NextEntry, &Parent->Children) {
+    D = EXT4_DENTRY_FROM_DENTRY_LIST (Entry);
+
+    if (D == ToBeRemoved) {
+      RemoveEntryList (Entry);
+      return;
+    }
+  }
+
+  DEBUG ((DEBUG_ERROR, "[ext4] Ext4RemoveDentry did not find the asked-for dentry\n"));
+}
+
+/**
+   Adds a dentry to the other's list.
+
+   The dentry that is added to the other one's list gets ->Parent set to Parent,
+   and the parent gets its reference count incremented.
+
+   @param[in out]            Parent       Pointer to the parent EXT4_DENTRY.
+   @param[in out]            ToBeAdded    Pointer to the child EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4AddDentry (
+  IN OUT EXT4_DENTRY  *Parent,
+  IN OUT EXT4_DENTRY  *ToBeAdded
+  )
+{
+  ToBeAdded->Parent = Parent;
+  InsertTailList (&Parent->Children, &ToBeAdded->ListNode);
+  Ext4RefDentry (Parent);
+}
+
+/**
+   Creates a new dentry object.
+
+   @param[in]              Name        Name of the dentry.
+   @param[in out opt]      Parent      Parent dentry, if it's not NULL.
+
+   @return The new allocated and initialised dentry.
+           The ref count will be set to 1.
+**/
+EXT4_DENTRY *
+Ext4CreateDentry (
+  IN CONST CHAR16              *Name,
+  IN OUT EXT4_DENTRY  *Parent  OPTIONAL
+  )
+{
+  EXT4_DENTRY  *Dentry;
+  EFI_STATUS   Status;
+
+  Dentry = AllocateZeroPool (sizeof (EXT4_DENTRY));
+
+  if (Dentry == NULL) {
+    return NULL;
+  }
+
+  Dentry->RefCount = 1;
+
+  // This StrCpyS should not fail.
+  Status = StrCpyS (Dentry->Name, ARRAY_SIZE (Dentry->Name), Name);
+
+  ASSERT_EFI_ERROR (Status);
+
+  InitializeListHead (&Dentry->Children);
+
+  if (Parent != NULL) {
+    Ext4AddDentry (Parent, Dentry);
+  }
+
+  DEBUG ((DEBUG_FS, "[ext4] Created dentry %s\n", Name));
+
+  return Dentry;
+}
+
+/**
+   Increments the ref count of the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+VOID
+Ext4RefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  UINTN  OldRef;
+
+  OldRef = Dentry->RefCount;
+
+  Dentry->RefCount++;
+
+  // I'm not sure if this (Refcount overflow) is a valid concern,
+  // but it's better to be safe than sorry.
+  ASSERT (OldRef < Dentry->RefCount);
+}
+
+/**
+   Deletes the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+STATIC
+VOID
+Ext4DeleteDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  if (Dentry->Parent) {
+    Ext4RemoveDentry (Dentry->Parent, Dentry);
+    Ext4UnrefDentry (Dentry->Parent);
+  }
+
+  DEBUG ((DEBUG_FS, "[ext4] Deleted dentry %s\n", Dentry->Name));
+  FreePool (Dentry);
+}
+
+/**
+   Decrements the ref count of the dentry.
+   If the ref count is 0, it's destroyed.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+
+   @retval True if it was destroyed, false if it's alive.
+**/
+BOOLEAN
+Ext4UnrefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  )
+{
+  Dentry->RefCount--;
+
+  if (Dentry->RefCount == 0) {
+    Ext4DeleteDentry (Dentry);
+    return TRUE;
+  }
+
+  return FALSE;
+}
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index b387ebcd36..070eb5a9c8 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -451,4 +451,7 @@ typedef struct {
 typedef UINT64  EXT4_BLOCK_NR;
 typedef UINT32  EXT4_INO_NR;
 
+// 2 is always the root inode number in ext4
+#define EXT4_ROOT_INODE_NR  2
+
 #endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 1aafc60ab5..db938c2524 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -55,6 +55,7 @@ Ext4OpenPartition (
   );
 
 typedef struct _Ext4File EXT4_FILE;
+typedef struct _Ext4_Dentry EXT4_DENTRY;
 
 typedef struct _Ext4_PARTITION {
   EFI_SIMPLE_FILE_SYSTEM_PROTOCOL    Interface;
@@ -81,8 +82,68 @@ typedef struct _Ext4_PARTITION {
   UINT32                             InitialSeed;
 
   LIST_ENTRY                         OpenFiles;
+
+  EXT4_DENTRY                        *RootDentry;
 } EXT4_PARTITION;
 
+/**
+   This structure represents a directory entry inside our directory entry tree.
+   For now, it will be used as a way to track file names inside our opening code,
+   but it may very well be used as a directory cache in the future.
+   Because it's not being used as a directory cache right now,
+   an EXT4_DENTRY structure is not necessarily unique name-wise in the list of
+   children. Therefore, the dentry tree does not accurately reflect the filesystem
+   structure.
+ */
+struct _Ext4_Dentry {
+  UINTN                  RefCount;
+  CHAR16                 Name[EXT4_NAME_MAX + 1];
+  EXT4_INO_NR            Inode;
+  struct _Ext4_Dentry    *Parent;
+  LIST_ENTRY             Children;
+  LIST_ENTRY             ListNode;
+};
+
+#define EXT4_DENTRY_FROM_DENTRY_LIST(Node)  BASE_CR (Node, EXT4_DENTRY, ListNode)
+
+/**
+   Creates a new dentry object.
+
+   @param[in]              Name        Name of the dentry.
+   @param[in out opt]      Parent      Parent dentry, if it's not NULL.
+
+   @return The new allocated and initialised dentry.
+           The ref count will be set to 1.
+**/
+EXT4_DENTRY *
+Ext4CreateDentry (
+  IN CONST CHAR16     *Name,
+  IN OUT EXT4_DENTRY  *Parent  OPTIONAL
+  );
+
+/**
+   Increments the ref count of the dentry.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+**/
+VOID
+Ext4RefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  );
+
+/**
+   Decrements the ref count of the dentry.
+   If the ref count is 0, it's destroyed.
+
+   @param[in out]            Dentry    Pointer to a valid EXT4_DENTRY.
+
+   @retval True if it was destroyed, false if it's alive.
+**/
+BOOLEAN
+Ext4UnrefDentry (
+  IN OUT EXT4_DENTRY  *Dentry
+  );
+
 /**
    Opens and parses the superblock.
 
@@ -126,7 +187,7 @@ Ext4OpenSuperblock (
    @param[in]     Partition  Pointer to the opened ext4 partition.
    @return The media ID associated with the partition.
 **/
-#define EXT4_MEDIA_ID(Partition) Partition->BlockIo->Media->MediaId
+#define EXT4_MEDIA_ID(Partition)  Partition->BlockIo->Media->MediaId
 
 /**
    Reads from the partition's disk using the DISK_IO protocol.
@@ -299,11 +360,13 @@ struct _Ext4File {
   UINT64                Position;
 
   EXT4_PARTITION        *Partition;
-  CHAR16                *FileName;
 
   ORDERED_COLLECTION    *ExtentsMap;
 
   LIST_ENTRY            OpenFilesListNode;
+
+  // Owning reference to this file's directory entry.
+  EXT4_DENTRY           *Dentry;
 };
 
 #define EXT4_FILE_FROM_OPEN_FILES_NODE(Node)  BASE_CR (Node, EXT4_FILE, OpenFilesListNode)
diff --git a/Features/Ext4Pkg/Ext4Dxe/File.c b/Features/Ext4Pkg/Ext4Dxe/File.c
index a3eff2b48a..021d10b1ed 100644
--- a/Features/Ext4Pkg/Ext4Dxe/File.c
+++ b/Features/Ext4Pkg/Ext4Dxe/File.c
@@ -207,7 +207,7 @@ Ext4Open (
 
     FileName += Length;
 
-    if (StrCmp(PathSegment, L".") == 0) {
+    if (StrCmp (PathSegment, L".") == 0) {
       // Opens of "." are a no-op
       continue;
     }
@@ -272,7 +272,7 @@ Ext4Open (
 
   *NewHandle = &Current->Protocol;
 
-  DEBUG ((DEBUG_FS, "Opened filename %s\n", Current->FileName));
+  DEBUG ((DEBUG_FS, "[ext4] Opened filename %s\n", Current->Dentry->Name));
   return EFI_SUCCESS;
 }
 
@@ -312,9 +312,9 @@ Ext4CloseInternal (
 
   DEBUG ((DEBUG_FS, "[ext4] Closed file %p (inode %lu)\n", File, File->InodeNum));
   RemoveEntryList (&File->OpenFilesListNode);
-  FreePool (File->FileName);
   FreePool (File->Inode);
   Ext4FreeExtentsMap (File);
+  Ext4UnrefDentry (File->Dentry);
   FreePool (File);
   return EFI_SUCCESS;
 }
@@ -522,11 +522,11 @@ Ext4GetFileInfo (
   UINTN         NeededLength;
   CONST CHAR16  *FileName;
 
-  if (File->InodeNum == 2) {
+  if (File->InodeNum == EXT4_ROOT_INODE_NR) {
     // Root inode gets a filename of "", regardless of how it was opened.
     FileName = L"";
   } else {
-    FileName = File->FileName;
+    FileName = File->Dentry->Name;
   }
 
   FileNameLen  = StrLen (FileName);
@@ -717,15 +717,6 @@ Ext4DuplicateFile (
 
   CopyMem (File->Inode, Original->Inode, Partition->InodeSize);
 
-  File->FileName = AllocateZeroPool (StrSize (Original->FileName));
-  if (File->FileName == NULL) {
-    FreePool (File->Inode);
-    FreePool (File);
-    return NULL;
-  }
-
-  StrCpyS (File->FileName, StrLen (Original->FileName) + 1, Original->FileName);
-
   File->Position = 0;
   Ext4SetupFile (File, Partition);
   File->InodeNum = Original->InodeNum;
@@ -733,12 +724,15 @@ Ext4DuplicateFile (
 
   Status = Ext4InitExtentsMap (File);
   if (EFI_ERROR (Status)) {
-    FreePool (File->FileName);
     FreePool (File->Inode);
     FreePool (File);
     return NULL;
   }
 
+  File->Dentry = Original->Dentry;
+
+  Ext4RefDentry (File->Dentry);
+
   InsertTailList (&Partition->OpenFiles, &File->OpenFilesListNode);
 
   return File;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 982b19c763..a2f8b4135f 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -89,7 +89,7 @@ Ext4Read (
   IN OUT UINTN           *Length
   )
 {
-  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->FileName, Offset, *Length));
+  DEBUG ((DEBUG_FS, "[ext4] Ext4Read(%s, Offset %lu, Length %lu)\n", File->Dentry->Name, Offset, *Length));
   EXT4_INODE   *Inode;
   UINT64       InodeSize;
   UINT64       CurrentSeek;
diff --git a/Features/Ext4Pkg/Ext4Dxe/Partition.c b/Features/Ext4Pkg/Ext4Dxe/Partition.c
index 2258bac76a..afa0392024 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Partition.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Partition.c
@@ -108,6 +108,7 @@ Ext4UnmountAndFreePartition (
   LIST_ENTRY  *Entry;
   LIST_ENTRY  *NextEntry;
   EXT4_FILE   *File;
+  BOOLEAN     DeletedRootDentry;
 
   Partition->Unmounting = TRUE;
   Ext4CloseInternal (Partition->Root);
@@ -118,6 +119,12 @@ Ext4UnmountAndFreePartition (
     Ext4CloseInternal (File);
   }
 
+  DeletedRootDentry = Ext4UnrefDentry (Partition->RootDentry);
+
+  if (!DeletedRootDentry) {
+    DEBUG ((DEBUG_ERROR, "[ext4] Failed to delete root dentry - resource leak present.\n"));
+  }
+
   FreePool (Partition->BlockGroups);
   FreePool (Partition);
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 8231831115..3efb047dc5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -6,6 +6,7 @@
 **/
 
 #include "Ext4Dxe.h"
+#include "Uefi/UefiBaseType.h"
 
 STATIC CONST UINT32  gSupportedCompatFeat = EXT4_FEATURE_COMPAT_EXT_ATTR;
 
@@ -167,8 +168,11 @@ Ext4OpenSuperblock (
   // accidentally opening an ext2/3/4 filesystem we don't understand, which would be disasterous.
 
   if (Partition->FeaturesIncompat & ~gSupportedIncompatFeat) {
-    DEBUG ((DEBUG_ERROR, "[ext4] Unsupported features %lx\n",
-            Partition->FeaturesIncompat & ~gSupportedIncompatFeat));
+    DEBUG ((
+      DEBUG_ERROR,
+      "[ext4] Unsupported features %lx\n",
+      Partition->FeaturesIncompat & ~gSupportedIncompatFeat
+      ));
     return EFI_UNSUPPORTED;
   }
 
@@ -247,7 +251,7 @@ Ext4OpenSuperblock (
 
   Partition->BlockGroups = Ext4AllocAndReadBlocks (Partition, NrBlocks, Partition->BlockSize == 1024 ? 2 : 1);
 
-  if (!Partition->BlockGroups) {
+  if (Partition->BlockGroups == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -255,13 +259,27 @@ Ext4OpenSuperblock (
     Desc = Ext4GetBlockGroupDesc (Partition, Index);
     if (!Ext4VerifyBlockGroupDescChecksum (Partition, Desc, Index)) {
       DEBUG ((DEBUG_ERROR, "[ext4] Block group descriptor %u has an invalid checksum\n", Index));
+      FreePool (Partition->BlockGroups);
       return EFI_VOLUME_CORRUPTED;
     }
   }
 
+  // RootDentry will serve as the basis of our directory entry tree.
+  Partition->RootDentry = Ext4CreateDentry (L"\\", NULL);
+
+  if (Partition->RootDentry == NULL) {
+    FreePool (Partition->BlockGroups);
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   // Note that the cast below is completely safe, because EXT4_FILE is a specialisation of EFI_FILE_PROTOCOL
   Status = Ext4OpenVolume (&Partition->Interface, (EFI_FILE_PROTOCOL **)&Partition->Root);
 
+  if (EFI_ERROR (Status)) {
+    Ext4UnrefDentry (Partition->RootDentry);
+    FreePool (Partition->BlockGroups);
+  }
+
   return Status;
 }
 
-- 
2.33.0

Re: [edk2-devel] [PATCH 0/3] Ext4Pkg: Fix bugs triggered by MdePkg and ShellPkg.

[Michael D Kinney]

Hi Pedro,

1) SuperBlock.c: The following #include was added, but should not be required:

    #include "Uefi/UefiBaseType.h"

2) Inode.c - Ext4Read() - Has DEBUG() macro before local variable declarations.


Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> Sent: Thursday, August 19, 2021 2:29 PM
> To: devel@edk2.groups.io
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: [edk2-devel] [PATCH 0/3] Ext4Pkg: Fix bugs triggered by MdePkg and ShellPkg.
> 
> This patch-series addresses bugs found when testing the filesystem with
> more complex usage of the file protocol, particularly through the shell
> itself.
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> 
> Pedro Falcato (3):
>   Ext4Pkg: Fix incorrect usage of Ext4InitExtentsMap.
>   Ext4Pkg: Hide "." and ".." entries from Read() callers.
>   Ext4Pkg: Add a directory entry tree.
> 
>  Features/Ext4Pkg/Ext4Dxe/Directory.c  | 253 ++++++++++++++++++++++----
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h   |   3 +
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |  71 +++++++-
>  Features/Ext4Pkg/Ext4Dxe/File.c       |  47 +++--
>  Features/Ext4Pkg/Ext4Dxe/Inode.c      |   2 +-
>  Features/Ext4Pkg/Ext4Dxe/Partition.c  |   7 +
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c |  24 ++-
>  7 files changed, 345 insertions(+), 62 deletions(-)
> 
> --
> 2.33.0
> 
> 
> 
> 
>