[PATCH v2 0/6] This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().

[PATCH v2 0/6] This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().

[Gabriel Dalimonte]

This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().
One of the use cases for renaming in EFI is to facilitate boot loader
boot counting.

No existing filesystems in U-Boot currently include file renaming,
resulting in support for renaming at the filesystem level and a
concrete implementation for the FAT filesystem.

Changes in v2:
 - update create_link() docstring
 - remove ATTR_ARCH being implicitly set in create_link()
 - cleanup log message and verbosity in fs_rename()
 - update fs_close() docstring with reference to fs_rename()
 - clarify fs_rename() behaviour in docstring
 - change fatrename to mv (updating semantics to match mv)
 - add mv command docs
 - add FAT_RENAME Kconfig symbol
 - update rename tests for mv command semantics
 - update rename test docs on whether mv should succeed or fail
 - remove all (fat) prefixes from commands used in rename tests
 - clarify docs in check_path_prefix()
 - stack allocate fat_itr in check_path_prefix()
 - log on observed filesystem corruption in check_path_prefix()
 - change 'break's to 'goto' in check_path_prefix()
 - normalize added logging to use log_debug()
 - correct error code returned when destination directory is not empty
 - remove setting ATTR_ARCH on parent dirs during renames
 - skip updating timestamps if no RTC present
 - make fat_itr stack allocated in update_parent_dir_props
 - update parent dir props in create_link and delete_dentry_link
 - simplify freeing of new_file_name and new_path

Gabriel Dalimonte (6):
  fs: fat: factor out dentry link create/delete
  fs: add rename infrastructure
  fs: fat: add rename
  fs: fat: update parent dirs metadata on dentry create/delete
  efi_loader: move path out of file_handle
  efi_loader: support file rename in SetInfo()

 cmd/fs.c                                |  14 +
 doc/usage/cmd/mv.rst                    |  61 +++
 fs/fat/Kconfig                          |   7 +
 fs/fat/fat_write.c                      | 476 +++++++++++++++++++++---
 fs/fs.c                                 |  95 +++++
 include/fat.h                           |   1 +
 include/fs.h                            |  16 +-
 lib/efi_loader/Kconfig                  |   1 +
 lib/efi_loader/efi_file.c               |  58 ++-
 test/py/tests/test_fs/conftest.py       | 121 ++++++
 test/py/tests/test_fs/fstest_helpers.py |   2 +
 test/py/tests/test_fs/test_rename.py    | 372 ++++++++++++++++++
 12 files changed, 1157 insertions(+), 67 deletions(-)
 create mode 100644 doc/usage/cmd/mv.rst
 create mode 100644 test/py/tests/test_fs/test_rename.py

-- 
2.34.1

[PATCH v2 1/6] fs: fat: factor out dentry link create/delete

[Gabriel Dalimonte]

The create_link() code was previously duplicated in two existing functions.
The two functions will be used in a future commit to achieve renaming.

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>

---

Changes in v2:
 - update create_link() docstring
 - remove ATTR_ARCH being implicitly set in create_link()


---
 fs/fat/fat_write.c | 121 ++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 56 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index ea877ee9171..86366d03853 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1215,6 +1215,43 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
 	memcpy(&dentptr->nameext, shortname, SHORT_NAME_SIZE);
 }
 
+/**
+ * create_link() - inserts a directory entry for a file or directory
+ *
+ * @itr:	directory iterator
+ * @basename:	file name
+ * @clust:	cluster number the new directory entry should point to. Use 0
+ * if no cluster is assigned yet
+ * @size:	file size
+ * @attr:	file attributes
+ * Return:	0 for success
+ */
+static int create_link(fat_itr *itr, char *basename, __u32 clust, __u32 size,
+		       __u8 attr)
+{
+	char shortname[SHORT_NAME_SIZE];
+	int ndent;
+	int ret;
+
+	/* Check if long name is needed */
+	ndent = set_name(itr, basename, shortname);
+	if (ndent < 0)
+		return ndent;
+	ret = fat_find_empty_dentries(itr, ndent);
+	if (ret)
+		return ret;
+	if (ndent > 1) {
+		/* Set long name entries */
+		ret = fill_dir_slot(itr, basename, shortname);
+		if (ret)
+			return ret;
+	}
+
+	fill_dentry(itr->fsdata, itr->dent, shortname, clust, size, attr);
+
+	return 0;
+}
+
 /**
  * find_directory_entry() - find a directory entry by filename
  *
@@ -1420,35 +1457,15 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
 		/* Update change date */
 		dentry_set_time(retdent);
 	} else {
-		/* Create a new file */
-		char shortname[SHORT_NAME_SIZE];
-		int ndent;
-
 		if (pos) {
 			/* No hole allowed */
 			ret = -EINVAL;
 			goto exit;
 		}
 
-		/* Check if long name is needed */
-		ndent = set_name(itr, basename, shortname);
-		if (ndent < 0) {
-			ret = ndent;
-			goto exit;
-		}
-		ret = fat_find_empty_dentries(itr, ndent);
+		ret = create_link(itr, basename, 0, size, ATTR_ARCH);
 		if (ret)
 			goto exit;
-		if (ndent > 1) {
-			/* Set long name entries */
-			ret = fill_dir_slot(itr, basename, shortname);
-			if (ret)
-				goto exit;
-		}
-
-		/* Set short name entry */
-		fill_dentry(itr->fsdata, itr->dent, shortname, 0, size,
-			    ATTR_ARCH);
 
 		retdent = itr->dent;
 	}
@@ -1564,6 +1581,31 @@ static int delete_long_name(fat_itr *itr)
 	return 0;
 }
 
+/**
+ * delete_dentry_link() - deletes a directory entry, but not the cluster chain
+ * it points to
+ *
+ * @itr:	the first directory entry (if a longname) to remove
+ * Return:	0 for success
+ */
+static int delete_dentry_link(fat_itr *itr)
+{
+	itr->dent = itr->dent_start;
+	itr->remaining = itr->dent_rem;
+	/* Delete long name */
+	if ((itr->dent->attr & ATTR_VFAT) == ATTR_VFAT &&
+	    (itr->dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
+		int ret;
+
+		ret = delete_long_name(itr);
+		if (ret)
+			return ret;
+	}
+	/* Delete short name */
+	delete_single_dentry(itr);
+	return flush_dir(itr);
+}
+
 /**
  * delete_dentry_long() - remove directory entry
  *
@@ -1589,21 +1631,7 @@ static int delete_dentry_long(fat_itr *itr)
 		if (ret)
 			return ret;
 	}
-	itr->dent = itr->dent_start;
-	itr->remaining = itr->dent_rem;
-	dent = itr->dent_start;
-	/* Delete long name */
-	if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&
-	    (dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
-		int ret;
-
-		ret = delete_long_name(itr);
-		if (ret)
-			return ret;
-	}
-	/* Delete short name */
-	delete_single_dentry(itr);
-	return flush_dir(itr);
+	return delete_dentry_link(itr);
 }
 
 int fat_unlink(const char *filename)
@@ -1725,9 +1753,6 @@ int fat_mkdir(const char *dirname)
 		ret = -EEXIST;
 		goto exit;
 	} else {
-		char shortname[SHORT_NAME_SIZE];
-		int ndent;
-
 		if (itr->is_root) {
 			/* root dir cannot have "." or ".." */
 			if (!strcmp(l_dirname, ".") ||
@@ -1737,25 +1762,9 @@ int fat_mkdir(const char *dirname)
 			}
 		}
 
-		/* Check if long name is needed */
-		ndent = set_name(itr, basename, shortname);
-		if (ndent < 0) {
-			ret = ndent;
-			goto exit;
-		}
-		ret = fat_find_empty_dentries(itr, ndent);
+		ret = create_link(itr, basename, 0, 0, ATTR_DIR | ATTR_ARCH);
 		if (ret)
 			goto exit;
-		if (ndent > 1) {
-			/* Set long name entries */
-			ret = fill_dir_slot(itr, basename, shortname);
-			if (ret)
-				goto exit;
-		}
-
-		/* Set attribute as archive for regular file */
-		fill_dentry(itr->fsdata, itr->dent, shortname, 0, 0,
-			    ATTR_DIR | ATTR_ARCH);
 
 		retdent = itr->dent;
 	}
-- 
2.34.1

[PATCH v2 2/6] fs: add rename infrastructure

[Gabriel Dalimonte]

The selection for *rename as the name for the rename/move operation
derives from the POSIX specification where they name the function
rename/renameat. [1] This aligns with Linux where the syscalls for
renaming/moving also use the rename/renameat naming.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>

---

Changes in v2:
 - cleanup log message and verbosity in fs_rename()
 - update fs_close() docstring with reference to fs_rename()
 - clarify fs_rename() behaviour in docstring


---
 fs/fs.c      | 32 ++++++++++++++++++++++++++++++++
 include/fs.h | 14 +++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/fs/fs.c b/fs/fs.c
index 99ddcc5e37b..fdff83719b1 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -143,6 +143,12 @@ static inline int fs_mkdir_unsupported(const char *dirname)
 	return -1;
 }
 
+static inline int fs_rename_unsupported(const char *old_path,
+					const char *new_path)
+{
+	return -1;
+}
+
 struct fstype_info {
 	int fstype;
 	char *name;
@@ -183,6 +189,7 @@ struct fstype_info {
 	int (*unlink)(const char *filename);
 	int (*mkdir)(const char *dirname);
 	int (*ln)(const char *filename, const char *target);
+	int (*rename)(const char *old_path, const char *new_path);
 };
 
 static struct fstype_info fstypes[] = {
@@ -206,6 +213,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 #endif
+		.rename = fs_rename_unsupported,
 		.uuid = fat_uuid,
 		.opendir = fat_opendir,
 		.readdir = fat_readdir,
@@ -238,6 +246,7 @@ static struct fstype_info fstypes[] = {
 		.closedir = ext4fs_closedir,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #if IS_ENABLED(CONFIG_SANDBOX) && !IS_ENABLED(CONFIG_XPL_BUILD)
@@ -257,6 +266,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #if CONFIG_IS_ENABLED(SEMIHOSTING)
@@ -276,6 +286,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #ifndef CONFIG_XPL_BUILD
@@ -296,6 +307,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #endif
@@ -317,6 +329,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #endif
@@ -339,6 +352,7 @@ static struct fstype_info fstypes[] = {
 		.ln = fs_ln_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 #if IS_ENABLED(CONFIG_FS_EROFS)
@@ -360,6 +374,7 @@ static struct fstype_info fstypes[] = {
 		.ln = fs_ln_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 #endif
 	{
@@ -378,6 +393,7 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
+		.rename = fs_rename_unsupported,
 	},
 };
 
@@ -713,6 +729,22 @@ int fs_ln(const char *fname, const char *target)
 	return ret;
 }
 
+int fs_rename(const char *old_path, const char *new_path)
+{
+	struct fstype_info *info = fs_get_info(fs_type);
+	int ret;
+
+	ret = info->rename(old_path, new_path);
+
+	if (ret < 0) {
+		log_debug("Unable to rename %s -> %s\n", old_path, new_path);
+		ret = -1;
+	}
+	fs_close();
+
+	return ret;
+}
+
 int do_size(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
 	    int fstype)
 {
diff --git a/include/fs.h b/include/fs.h
index 2474880385d..5b272eb9f5e 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -86,7 +86,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part);
  *
  * Many file functions implicitly call fs_close(), e.g. fs_closedir(),
  * fs_exist(), fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write(),
- * fs_unlink().
+ * fs_unlink(), fs_rename().
  */
 void fs_close(void);
 
@@ -270,6 +270,18 @@ int fs_unlink(const char *filename);
  */
 int fs_mkdir(const char *filename);
 
+/**
+ * fs_rename - rename/move a file or directory
+ *
+ * @old_path: existing path of the file/directory to rename
+ * @new_path: new path of the file/directory. If this points to an existing
+ * file or empty directory, the existing file/directory will be unlinked.
+ * If this points to a non-empty directory, the rename will fail.
+ *
+ * Return: 0 on success, -1 on error conditions
+ */
+int fs_rename(const char *old_path, const char *new_path);
+
 /*
  * Common implementation for various filesystem commands, optionally limited
  * to a specific filesystem type via the fstype parameter.
-- 
2.34.1

Re: [PATCH v2 2/6] fs: add rename infrastructure

[Ilias Apalodimas]

Hi Gabriel,

On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
<gabriel.dalimonte@gmail.com> wrote:
>
> The selection for *rename as the name for the rename/move operation
> derives from the POSIX specification where they name the function
> rename/renameat. [1] This aligns with Linux where the syscalls for
> renaming/moving also use the rename/renameat naming.
>
> [1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html
>
> Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
>
> ---
>
> Changes in v2:
>  - cleanup log message and verbosity in fs_rename()
>  - update fs_close() docstring with reference to fs_rename()
>  - clarify fs_rename() behaviour in docstring
>
>
> ---
>  fs/fs.c      | 32 ++++++++++++++++++++++++++++++++
>  include/fs.h | 14 +++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 99ddcc5e37b..fdff83719b1 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -143,6 +143,12 @@ static inline int fs_mkdir_unsupported(const char *dirname)
>         return -1;
>  }
>
> +static inline int fs_rename_unsupported(const char *old_path,
> +                                       const char *new_path)
> +{
> +       return -1;
> +}
> +
>  struct fstype_info {
>         int fstype;
>         char *name;
> @@ -183,6 +189,7 @@ struct fstype_info {
>         int (*unlink)(const char *filename);
>         int (*mkdir)(const char *dirname);
>         int (*ln)(const char *filename, const char *target);
> +       int (*rename)(const char *old_path, const char *new_path);
>  };
>
>  static struct fstype_info fstypes[] = {
> @@ -206,6 +213,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>  #endif
> +               .rename = fs_rename_unsupported,
>                 .uuid = fat_uuid,
>                 .opendir = fat_opendir,
>                 .readdir = fat_readdir,
> @@ -238,6 +246,7 @@ static struct fstype_info fstypes[] = {
>                 .closedir = ext4fs_closedir,
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #if IS_ENABLED(CONFIG_SANDBOX) && !IS_ENABLED(CONFIG_XPL_BUILD)
> @@ -257,6 +266,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>                 .ln = fs_ln_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #if CONFIG_IS_ENABLED(SEMIHOSTING)
> @@ -276,6 +286,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>                 .ln = fs_ln_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #ifndef CONFIG_XPL_BUILD
> @@ -296,6 +307,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>                 .ln = fs_ln_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #endif
> @@ -317,6 +329,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>                 .ln = fs_ln_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #endif
> @@ -339,6 +352,7 @@ static struct fstype_info fstypes[] = {
>                 .ln = fs_ln_unsupported,
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>  #if IS_ENABLED(CONFIG_FS_EROFS)
> @@ -360,6 +374,7 @@ static struct fstype_info fstypes[] = {
>                 .ln = fs_ln_unsupported,
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  #endif
>         {
> @@ -378,6 +393,7 @@ static struct fstype_info fstypes[] = {
>                 .unlink = fs_unlink_unsupported,
>                 .mkdir = fs_mkdir_unsupported,
>                 .ln = fs_ln_unsupported,
> +               .rename = fs_rename_unsupported,
>         },
>  };
>
> @@ -713,6 +729,22 @@ int fs_ln(const char *fname, const char *target)
>         return ret;
>  }
>
> +int fs_rename(const char *old_path, const char *new_path)
> +{
> +       struct fstype_info *info = fs_get_info(fs_type);
> +       int ret;
> +
> +       ret = info->rename(old_path, new_path);
> +
> +       if (ret < 0) {
> +               log_debug("Unable to rename %s -> %s\n", old_path, new_path);
> +               ret = -1;
> +       }
> +       fs_close();
> +
> +       return ret;
> +}
> +
>  int do_size(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
>             int fstype)
>  {
> diff --git a/include/fs.h b/include/fs.h
> index 2474880385d..5b272eb9f5e 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -86,7 +86,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part);
>   *
>   * Many file functions implicitly call fs_close(), e.g. fs_closedir(),
>   * fs_exist(), fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write(),
> - * fs_unlink().
> + * fs_unlink(), fs_rename().
>   */
>  void fs_close(void);
>
> @@ -270,6 +270,18 @@ int fs_unlink(const char *filename);
>   */
>  int fs_mkdir(const char *filename);
>
> +/**
> + * fs_rename - rename/move a file or directory
> + *
> + * @old_path: existing path of the file/directory to rename
> + * @new_path: new path of the file/directory. If this points to an existing
> + * file or empty directory, the existing file/directory will be unlinked.
> + * If this points to a non-empty directory, the rename will fail.
> + *
> + * Return: 0 on success, -1 on error conditions
> + */
> +int fs_rename(const char *old_path, const char *new_path);
> +
>  /*
>   * Common implementation for various filesystem commands, optionally limited
>   * to a specific filesystem type via the fstype parameter.
> --
> 2.34.1
>

I am by no means an expert on our FS support but this looks reasonable
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v2 3/6] fs: fat: add rename

[Gabriel Dalimonte]

The implementation roughly follows the POSIX specification for
rename() [1]. The ordering of operations attempting to minimize the chance
for data loss in unexpected circumstances.

The 'mv' command was implemented as a front end for the rename operation
as that is what most users are likely familiar with in terms of behavior.

The 'FAT_RENAME' Kconfig option was added to prevent code size increase on
size-oriented builds like SPL.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>

---

Changes in v2:
 - change fatrename to mv (updating semantics to match mv)
 - add mv command docs
 - add FAT_RENAME Kconfig symbol
 - update rename tests for mv command semantics
 - update rename test docs on whether mv should succeed or fail
 - remove all (fat) prefixes from commands used in rename tests
 - clarify docs in check_path_prefix()
 - stack allocate fat_itr in check_path_prefix()
 - log on observed filesystem corruption in check_path_prefix()
 - change 'break's to 'goto' in check_path_prefix()
 - normalize added logging to use log_debug()
 - correct error code returned when destination directory is not empty


---
 cmd/fs.c                                |  14 +
 doc/usage/cmd/mv.rst                    |  61 ++++
 fs/fat/Kconfig                          |   7 +
 fs/fat/fat_write.c                      | 285 ++++++++++++++++++
 fs/fs.c                                 |  65 ++++-
 include/fat.h                           |   1 +
 include/fs.h                            |   2 +
 lib/efi_loader/Kconfig                  |   1 +
 test/py/tests/test_fs/conftest.py       | 121 ++++++++
 test/py/tests/test_fs/fstest_helpers.py |   2 +
 test/py/tests/test_fs/test_rename.py    | 372 ++++++++++++++++++++++++
 11 files changed, 930 insertions(+), 1 deletion(-)
 create mode 100644 doc/usage/cmd/mv.rst
 create mode 100644 test/py/tests/test_fs/test_rename.py

diff --git a/cmd/fs.c b/cmd/fs.c
index 3d7e06d6f1e..3faf7627447 100644
--- a/cmd/fs.c
+++ b/cmd/fs.c
@@ -110,3 +110,17 @@ U_BOOT_CMD(
 	fstypes, 1, 1, do_fstypes_wrapper,
 	"List supported filesystem types", ""
 );
+
+static int do_mv_wrapper(struct cmd_tbl *cmdtp, int flag, int argc,
+			 char *const argv[])
+{
+	return do_mv(cmdtp, flag, argc, argv, FS_TYPE_ANY);
+}
+
+U_BOOT_CMD(
+	mv,	5,	1,	do_mv_wrapper,
+	"rename/move a file/directory",
+	"<interface> [<dev[:part]>] <old_path> <new_path>\n"
+	"    - renames/moves a file/directory in 'dev' on 'interface' from\n"
+	"      'old_path' to 'new_path'"
+);
diff --git a/doc/usage/cmd/mv.rst b/doc/usage/cmd/mv.rst
new file mode 100644
index 00000000000..99864371038
--- /dev/null
+++ b/doc/usage/cmd/mv.rst
@@ -0,0 +1,61 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+.. index::
+   single: mv (command)
+
+mv command
+==========
+
+Synopsis
+--------
+
+::
+
+    mv <interface> [<dev[:part]>] <old_path> <new_path>
+
+Description
+-----------
+
+The mv command renames/moves a file or directory within a filesystem.
+
+interface
+    interface for accessing the block device (mmc, sata, scsi, usb, ....)
+
+dev
+    device number
+
+part
+    partition number, defaults to 0 (whole device)
+
+old_path
+   existing path to file/directory
+
+new_path
+   new path/name for the rename/move
+
+
+Example
+-------
+
+    # Rename file 'foo' in directory 'dir' to 'bar'
+    mv mmc 0:0 dir/foo dir/bar
+
+    # Move file 'f' from directory 'foo' to existing directory 'bar' renaming
+    # 'f' to 'g'
+    mv mmc 0:0 foo/f bar/g
+
+    # Move directory 'abc' in directory 'dir1' into existing directory 'dir2'
+    mv mmc 0:0 dir1/abc dir2
+
+Configuration
+-------------
+
+The mv command is only available if CONFIG_CMD_FS_GENERIC=y.
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the file was successfully
+renamed/moved.
+
+If an error occurs, the return value $? is set to 1 (false).
diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig
index 9bb11eac9f7..19d52238713 100644
--- a/fs/fat/Kconfig
+++ b/fs/fat/Kconfig
@@ -13,6 +13,13 @@ config FAT_WRITE
 	  This provides support for creating and writing new files to an
 	  existing FAT filesystem partition.
 
+config FAT_RENAME
+	bool "Enable filesystem rename support"
+	depends on FAT_WRITE
+	help
+	  This provides support for renaming and moving files within a
+	  FAT filesystem partition.
+
 config FS_FAT_MAX_CLUSTSIZE
 	int "Set maximum possible clustersize"
 	default 65536
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 86366d03853..d4952e259ff 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1215,6 +1215,28 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
 	memcpy(&dentptr->nameext, shortname, SHORT_NAME_SIZE);
 }
 
+/**
+ * fat_itr_parent() - modifies the iterator to the parent directory of the
+ * current iterator.
+ *
+ * @itr:	iterator positioned anywhere in a directory
+ * @Return:	0 if the iterator is in the parent directory, -errno otherwise
+ */
+static int fat_itr_parent(fat_itr *itr)
+{
+	int ret;
+
+	if (itr->is_root)
+		return -EIO;
+
+	/* ensure iterator is at the first directory entry */
+	ret = fat_move_to_cluster(itr, itr->start_clust);
+	if (ret)
+		return ret;
+
+	return fat_itr_resolve(itr, "..", TYPE_DIR);
+}
+
 /**
  * create_link() - inserts a directory entry for a file or directory
  *
@@ -1822,3 +1844,266 @@ exit:
 	free(dotdent);
 	return ret;
 }
+
+/**
+ * check_path_prefix() - ensures one path does not contains another path as a
+ * prefix.
+ *
+ * for example: path foo/bar/baz/qux contains the path prefix foo/bar/baz
+ *
+ * note: the iterator may be pointing to any directory entry in the directory
+ *
+ * @prefix_clust:	start cluster of the final directory in the prefix path
+ * (the start cluster of 'baz' in the above example)
+ * @path_itr:	iterator of the path to check (an iterator pointing to any
+ * direntry in 'qux' in the above example)
+ * Return:	-errno on error, 0 if path_itr does not have the directory
+ * at prefix_clust as an ancestor.
+ */
+static int check_path_prefix(loff_t prefix_clust, fat_itr *path_itr)
+{
+	fat_itr itr;
+	fsdata fsdata = { .fatbuf = NULL, }, *mydata = &fsdata;
+	int ret;
+
+	/* duplicate fsdata */
+	itr = *path_itr;
+	fsdata = *itr.fsdata;
+
+	/* allocate local fat buffer */
+	fsdata.fatbuf = malloc_cache_aligned(FATBUFSIZE);
+	if (!fsdata.fatbuf) {
+		log_debug("Error: allocating memory\n");
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	fsdata.fatbufnum = -1;
+	itr.fsdata = &fsdata;
+
+	/* ensure iterator is at the first directory entry */
+	ret = fat_move_to_cluster(&itr, itr.start_clust);
+	if (ret)
+		goto exit;
+
+	while (1) {
+		if (prefix_clust == itr.start_clust) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		if (itr.is_root) {
+			ret = 0;
+			goto exit;
+		}
+
+		/* Should not occur in a well-formed FAT filesystem besides the root */
+		if (fat_itr_parent(&itr)) {
+			log_debug("FAT filesystem corrupt!\n");
+			log_debug("dir @ clust %u has no parent direntry\n",
+				  itr.start_clust);
+			ret = -EIO;
+			goto exit;
+		}
+	}
+
+exit:
+	free(fsdata.fatbuf);
+	return ret;
+}
+
+/**
+ * fat_rename - rename/move a file or directory
+ *
+ * @old_path:	path to the existing file/directory
+ * @new_path:	new path/name for the rename/move
+ * Return:	0 on success, -errno otherwise
+ */
+int fat_rename(const char *old_path, const char *new_path)
+{
+	fat_itr *old_itr = NULL, *new_itr = NULL;
+	fsdata old_datablock = { .fatbuf = NULL, };
+	fsdata new_datablock = { .fatbuf = NULL, };
+	/* used for START macro */
+	fsdata *mydata = &old_datablock;
+	int ret = -EIO, is_old_dir;
+	char *old_path_copy, *old_dirname, *old_basename;
+	char *new_path_copy, *new_dirname, *new_basename;
+	char l_new_basename[VFAT_MAXLEN_BYTES];
+	__u32 old_clust;
+	dir_entry *found_existing;
+	/* only set if found_existing != NULL */
+	__u32 new_clust;
+
+	old_path_copy = strdup(old_path);
+	new_path_copy = strdup(new_path);
+	old_itr = malloc_cache_aligned(sizeof(fat_itr));
+	new_itr = malloc_cache_aligned(sizeof(fat_itr));
+	if (!old_path_copy || !new_path_copy || !old_itr || !new_itr) {
+		log_debug("Error: out of memory\n");
+		ret = -ENOMEM;
+		goto exit;
+	}
+	split_filename(old_path_copy, &old_dirname, &old_basename);
+	split_filename(new_path_copy, &new_dirname, &new_basename);
+
+	if (normalize_longname(l_new_basename, new_basename)) {
+		log_debug("FAT: illegal filename (%s)\n", new_basename);
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	if (!strcmp(old_basename, ".") || !strcmp(old_basename, "..") ||
+	    !strcmp(old_basename, "") || !strcmp(l_new_basename, ".") ||
+	    !strcmp(l_new_basename, "..") || !strcmp(l_new_basename, "")) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	/* checking for old_path == new_path is deferred until they're resolved */
+
+	/* resolve old_path */
+	ret = fat_itr_root(old_itr, &old_datablock);
+	if (ret)
+		goto exit;
+
+	ret = fat_itr_resolve(old_itr, old_dirname, TYPE_DIR);
+	if (ret) {
+		log_debug("%s doesn't exist (%d)\n", old_dirname, ret);
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	if (!find_directory_entry(old_itr, old_basename)) {
+		log_debug("%s doesn't exist (%d)\n", old_basename, -ENOENT);
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	/* store clust old_path points to, to relink later */
+	total_sector = old_datablock.total_sect;
+	old_clust = START(old_itr->dent);
+	is_old_dir = fat_itr_isdir(old_itr);
+
+	/* resolve new_path*/
+	ret = fat_itr_root(new_itr, &new_datablock);
+	if (ret)
+		goto exit;
+
+	ret = fat_itr_resolve(new_itr, new_dirname, TYPE_DIR);
+	if (ret) {
+		log_debug("%s doesn't exist (%d)\n", new_dirname, ret);
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	found_existing = find_directory_entry(new_itr, l_new_basename);
+
+	if (found_existing) {
+		/* store cluster of new_path since it may need to be deleted */
+		new_clust = START(new_itr->dent);
+
+		/* old_path is new_path, noop */
+		if (old_clust == new_clust) {
+			ret = 0;
+			goto exit;
+		}
+
+		if (fat_itr_isdir(new_itr) != is_old_dir) {
+			if (is_old_dir)
+				ret = -ENOTDIR;
+			else
+				ret = -EISDIR;
+			goto exit;
+		}
+	}
+
+	if (is_old_dir) {
+		ret = check_path_prefix(old_clust, new_itr);
+		if (ret)
+			goto exit;
+	}
+
+	/* create/update dentry to point to old_path's data cluster */
+	if (found_existing) {
+		struct nameext new_name = new_itr->dent->nameext;
+		__u8 lcase = new_itr->dent->lcase;
+
+		if (is_old_dir) {
+			int n_entries = fat_dir_entries(new_itr);
+
+			if (n_entries < 0) {
+				ret = n_entries;
+				goto exit;
+			}
+			if (n_entries > 2) {
+				log_debug("Error: directory is not empty: %d\n",
+					  n_entries);
+				ret = -ENOTEMPTY;
+				goto exit;
+			}
+		}
+
+		*new_itr->dent = *old_itr->dent;
+		new_itr->dent->nameext = new_name;
+		new_itr->dent->lcase = lcase;
+	} else {
+		/* reset iterator to the start of the directory */
+		ret = fat_move_to_cluster(new_itr, new_itr->start_clust);
+		if (ret)
+			goto exit;
+
+		ret = create_link(new_itr, l_new_basename, old_clust,
+				  old_itr->dent->size,
+				  old_itr->dent->attr | ATTR_ARCH);
+		if (ret)
+			goto exit;
+	}
+
+	ret = flush_dir(new_itr);
+	if (ret)
+		goto exit;
+
+	/* with new_path data cluster unreferenced, clear it */
+	if (found_existing) {
+		ret = clear_fatent(&new_datablock, new_clust);
+		if (ret)
+			goto exit;
+	}
+
+	/* update moved directory so the parent is new_path */
+	if (is_old_dir) {
+		__u32 clust = new_itr->start_clust;
+		dir_entry *dent;
+
+		fat_itr_child(new_itr, new_itr);
+		dent = find_directory_entry(new_itr, "..");
+		if (!dent) {
+			log_debug("FAT filesystem corrupt!\n");
+			log_debug("dir %s has no parent direntry\n",
+				  l_new_basename);
+			ret = -EIO;
+			goto exit;
+		}
+		set_start_cluster(&new_datablock, dent, clust);
+		ret = flush_dir(new_itr);
+		if (ret)
+			goto exit;
+	}
+
+	/* refresh old in case write happened to the same block. */
+	ret = fat_move_to_cluster(old_itr, old_itr->dent_clust);
+	if (ret)
+		goto exit;
+
+	ret = delete_dentry_link(old_itr);
+exit:
+	free(new_datablock.fatbuf);
+	free(old_datablock.fatbuf);
+	free(new_itr);
+	free(old_itr);
+	free(new_path_copy);
+	free(old_path_copy);
+
+	return ret;
+}
diff --git a/fs/fs.c b/fs/fs.c
index fdff83719b1..30a8e5010f2 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -213,12 +213,16 @@ static struct fstype_info fstypes[] = {
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
 #endif
-		.rename = fs_rename_unsupported,
 		.uuid = fat_uuid,
 		.opendir = fat_opendir,
 		.readdir = fat_readdir,
 		.closedir = fat_closedir,
 		.ln = fs_ln_unsupported,
+#if CONFIG_IS_ENABLED(FAT_RENAME) && !IS_ENABLED(CONFIG_XPL_BUILD)
+		.rename = fat_rename,
+#else
+		.rename = fs_rename_unsupported,
+#endif
 	},
 #endif
 
@@ -1007,6 +1011,65 @@ int do_ln(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
 	return 0;
 }
 
+int do_mv(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
+	  int fstype)
+{
+	struct fs_dir_stream *dirs;
+	char *src = argv[3];
+	char *dst = argv[4];
+	char *new_dst = NULL;
+	int ret = 1;
+
+	if (argc != 5) {
+		ret = CMD_RET_USAGE;
+		goto exit;
+	}
+
+	if (fs_set_blk_dev(argv[1], argv[2], fstype))
+		goto exit;
+
+	dirs = fs_opendir(dst);
+	/* dirs being valid means dst points to an existing directory.
+	 * mv should copy the file/dir (keeping the same name) into the
+	 * directory
+	 */
+	if (dirs) {
+		char *src_name = strrchr(src, '/');
+		int dst_len;
+
+		if (src_name)
+			src_name += 1;
+		else
+			src_name = src;
+
+		dst_len = strlen(dst);
+		new_dst = calloc(1, dst_len + strlen(src_name) + 2);
+		strcpy(new_dst, dst);
+
+		/* If there is already a trailing slash, don't add another */
+		if (new_dst[dst_len - 1] != '/') {
+			new_dst[dst_len] = '/';
+			dst_len += 1;
+		}
+
+		strcpy(new_dst + dst_len, src_name);
+		dst = new_dst;
+	}
+	fs_closedir(dirs);
+
+	if (fs_set_blk_dev(argv[1], argv[2], fstype))
+		goto exit;
+
+	if (fs_rename(src, dst))
+		goto exit;
+
+	ret = 0;
+
+exit:
+	free(new_dst);
+	return ret;
+}
+
 int do_fs_types(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
 {
 	struct fstype_info *drv = fstypes;
diff --git a/include/fat.h b/include/fat.h
index 3dce99a23cf..ca97880de12 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -206,6 +206,7 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
 int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
 void fat_closedir(struct fs_dir_stream *dirs);
 int fat_unlink(const char *filename);
+int fat_rename(const char *old_path, const char *new_path);
 int fat_mkdir(const char *dirname);
 void fat_close(void);
 void *fat_next_cluster(fat_itr *itr, unsigned int *nbytes);
diff --git a/include/fs.h b/include/fs.h
index 5b272eb9f5e..54449faf2e5 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -302,6 +302,8 @@ int do_mkdir(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
 	     int fstype);
 int do_ln(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
 	  int fstype);
+int do_mv(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
+	  int fstype);
 
 /*
  * Determine the UUID of the specified filesystem and print it. Optionally it is
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d4f6b56afaa..6130af14337 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -27,6 +27,7 @@ config EFI_LOADER
 	select REGEX
 	imply FAT
 	imply FAT_WRITE
+	imply FAT_RENAME
 	imply USB_KEYBOARD_FN_KEYS
 	imply VIDEO_ANSI
 	help
diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
index af2adaf1645..7bfcf41ed6f 100644
--- a/test/py/tests/test_fs/conftest.py
+++ b/test/py/tests/test_fs/conftest.py
@@ -18,6 +18,7 @@ supported_fs_fat = ['fat12', 'fat16']
 supported_fs_mkdir = ['fat12', 'fat16', 'fat32']
 supported_fs_unlink = ['fat12', 'fat16', 'fat32']
 supported_fs_symlink = ['ext4']
+supported_fs_rename = ['fat12', 'fat16', 'fat32']
 
 #
 # Filesystem test specific setup
@@ -55,6 +56,7 @@ def pytest_configure(config):
     global supported_fs_mkdir
     global supported_fs_unlink
     global supported_fs_symlink
+    global supported_fs_rename
 
     def intersect(listA, listB):
         return  [x for x in listA if x in listB]
@@ -68,6 +70,7 @@ def pytest_configure(config):
         supported_fs_mkdir =  intersect(supported_fs, supported_fs_mkdir)
         supported_fs_unlink =  intersect(supported_fs, supported_fs_unlink)
         supported_fs_symlink =  intersect(supported_fs, supported_fs_symlink)
+        supported_fs_rename =  intersect(supported_fs, supported_fs_rename)
 
 def pytest_generate_tests(metafunc):
     """Parametrize fixtures, fs_obj_xxx
@@ -99,6 +102,9 @@ def pytest_generate_tests(metafunc):
     if 'fs_obj_symlink' in metafunc.fixturenames:
         metafunc.parametrize('fs_obj_symlink', supported_fs_symlink,
             indirect=True, scope='module')
+    if 'fs_obj_rename' in metafunc.fixturenames:
+        metafunc.parametrize('fs_obj_rename', supported_fs_rename,
+            indirect=True, scope='module')
 
 #
 # Helper functions
@@ -527,6 +533,121 @@ def fs_obj_symlink(request, u_boot_config):
         call('rm -rf %s' % scratch_dir, shell=True)
         call('rm -f %s' % fs_img, shell=True)
 
+#
+# Fixture for rename test
+#
+@pytest.fixture()
+def fs_obj_rename(request, u_boot_config):
+    """Set up a file system to be used in rename tests.
+
+    Args:
+        request: Pytest request object.
+        u_boot_config: U-Boot configuration.
+
+    Return:
+        A fixture for rename tests, i.e. a triplet of file system type,
+        volume file name, and dictionary of test identifier and md5val.
+    """
+    def new_rand_file(path):
+        check_call('dd if=/dev/urandom of=%s bs=1K count=1' % path, shell=True)
+
+    def file_hash(path):
+        out = check_output(
+            'dd if=%s bs=1K skip=0 count=1 2> /dev/null | md5sum' % path,
+            shell=True
+        )
+        return out.decode().split()[0]
+
+    fs_type = request.param
+    fs_img = ''
+
+    fs_ubtype = fstype_to_ubname(fs_type)
+    check_ubconfig(u_boot_config, fs_ubtype)
+
+    mount_dir = u_boot_config.persistent_data_dir + '/scratch'
+
+    try:
+        check_call('mkdir -p %s' % mount_dir, shell=True)
+    except CalledProcessError as err:
+        pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
+        call('rm -f %s' % fs_img, shell=True)
+        return
+
+    try:
+        md5val = {}
+        # Test Case 1
+        check_call('mkdir %s/test1' % mount_dir, shell=True)
+        new_rand_file('%s/test1/file1' % mount_dir)
+        md5val['test1'] = file_hash('%s/test1/file1' % mount_dir)
+
+        # Test Case 2
+        check_call('mkdir %s/test2' % mount_dir, shell=True)
+        new_rand_file('%s/test2/file1' % mount_dir)
+        new_rand_file('%s/test2/file_exist' % mount_dir)
+        md5val['test2'] = file_hash('%s/test2/file1' % mount_dir)
+
+        # Test Case 3
+        check_call('mkdir -p %s/test3/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test3/dir1/file1' % mount_dir)
+        md5val['test3'] = file_hash('%s/test3/dir1/file1' % mount_dir)
+
+        # Test Case 4
+        check_call('mkdir -p %s/test4/dir1' % mount_dir, shell=True)
+        check_call('mkdir -p %s/test4/dir2/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test4/dir1/file1' % mount_dir)
+        md5val['test4'] = file_hash('%s/test4/dir1/file1' % mount_dir)
+
+        # Test Case 5
+        check_call('mkdir -p %s/test5/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test5/file2' % mount_dir)
+        md5val['test5'] = file_hash('%s/test5/file2' % mount_dir)
+
+        # Test Case 6
+        check_call('mkdir -p %s/test6/dir2/existing' % mount_dir, shell=True)
+        new_rand_file('%s/test6/existing' % mount_dir)
+        md5val['test6'] = file_hash('%s/test6/existing' % mount_dir)
+
+        # Test Case 7
+        check_call('mkdir -p %s/test7/dir1' % mount_dir, shell=True)
+        check_call('mkdir -p %s/test7/dir2/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test7/dir2/dir1/file1' % mount_dir)
+        md5val['test7'] = file_hash('%s/test7/dir2/dir1/file1' % mount_dir)
+
+        # Test Case 8
+        check_call('mkdir -p %s/test8/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test8/dir1/file1' % mount_dir)
+        md5val['test8'] = file_hash('%s/test8/dir1/file1' % mount_dir)
+
+        # Test Case 9
+        check_call('mkdir -p %s/test9/dir1/nested/inner' % mount_dir, shell=True)
+        new_rand_file('%s/test9/dir1/nested/inner/file1' % mount_dir)
+
+        # Test Case 10
+        check_call('mkdir -p %s/test10' % mount_dir, shell=True)
+        new_rand_file('%s/test10/file1' % mount_dir)
+        md5val['test10'] = file_hash('%s/test10/file1' % mount_dir)
+
+        # Test Case 11
+        check_call('mkdir -p %s/test11/dir1' % mount_dir, shell=True)
+        new_rand_file('%s/test11/dir1/file1' % mount_dir)
+        md5val['test11'] = file_hash('%s/test11/dir1/file1' % mount_dir)
+
+        try:
+            # 128MiB volume
+            fs_img = fs_helper.mk_fs(u_boot_config, fs_type, 0x8000000, '128MB', mount_dir)
+        except CalledProcessError as err:
+            pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
+            return
+
+    except CalledProcessError:
+        pytest.skip('Setup failed for filesystem: ' + fs_type)
+        return
+    else:
+        yield [fs_ubtype, fs_img, md5val]
+    finally:
+        call('rm -rf %s' % mount_dir, shell=True)
+        call('rm -f %s' % fs_img, shell=True)
+
 #
 # Fixture for fat test
 #
diff --git a/test/py/tests/test_fs/fstest_helpers.py b/test/py/tests/test_fs/fstest_helpers.py
index faec2982489..c1447b4d43e 100644
--- a/test/py/tests/test_fs/fstest_helpers.py
+++ b/test/py/tests/test_fs/fstest_helpers.py
@@ -9,5 +9,7 @@ def assert_fs_integrity(fs_type, fs_img):
     try:
         if fs_type == 'ext4':
             check_call('fsck.ext4 -n -f %s' % fs_img, shell=True)
+        elif fs_type in ['fat12', 'fat16', 'fat32']:
+            check_call('fsck.fat -n %s' % fs_img, shell=True)
     except CalledProcessError:
         raise
diff --git a/test/py/tests/test_fs/test_rename.py b/test/py/tests/test_fs/test_rename.py
new file mode 100644
index 00000000000..df2b2fd2945
--- /dev/null
+++ b/test/py/tests/test_fs/test_rename.py
@@ -0,0 +1,372 @@
+# SPDX-License-Identifier:      GPL-2.0+
+# Copyright 2025 Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
+#
+# U-Boot File System:rename Test
+
+
+import pytest
+
+from fstest_defs import *
+from fstest_helpers import assert_fs_integrity
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.slow
+class TestRename(object):
+    def test_rename1(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 1 - rename a file (successful mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 1 - rename a file'):
+            d = 'test1'
+            src = '%s/file1' % d
+            dst = '%s/file2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('file1' not in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test1'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename2(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 2 - rename a file to an existing file (successful mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 2 - rename a file to an existing file'):
+            d = 'test2'
+            src = '%s/file1' % d
+            dst = '%s/file_exist' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('file1' not in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test2'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename3(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 3 - rename a directory (successful mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 3 - rename a directory'):
+            d = 'test3'
+            src = '%s/dir1' % d
+            dst = '%s/dir2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s/file1' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' not in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test3'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename4(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 4 - rename a directory to an existing directory (successful
+        mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 4 - rename a directory to an existing directory'):
+            d = 'test4'
+            src = '%s/dir1' % d
+            dst = '%s/dir2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s/dir1/file1' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' not in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test4'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename5(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 5 - rename a directory to an existing file (failed mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 5 - rename a directory to an existing file'):
+            d = 'test5'
+            src = '%s/dir1' % d
+            dst = '%s/file2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' in ''.join(output))
+            assert('file2' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test5'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename6(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 6 - rename a file to an existing empty directory (failed mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 6 - rename a file to an existing empty directory'):
+            d = 'test6'
+            src = '%s/existing' % d
+            dst = '%s/dir2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s' % (ADDR, src),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir2' in ''.join(output))
+            assert('existing' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test6'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename7(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 7 - rename a directory to a non-empty directory (failed mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 7 - rename a directory to a non-empty directory'):
+            d = 'test7'
+            src = '%s/dir1' % d
+            dst = '%s/dir2' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s/dir1/file1' % (ADDR, dst),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' in ''.join(output))
+            assert('dir2' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test7'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename8(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 8 - rename a directory inside itself (failed mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 8 - rename a directory inside itself'):
+            d = 'test8'
+            src = '%s/dir1' % d
+            dst = '%s/dir1/dir1' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s/file1' % (ADDR, src),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (src),
+            ])
+            assert('file1' in ''.join(output))
+            assert('dir1' not in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test8'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename9(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 9 - rename a directory inside itself with backtracks (failed
+        mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 9 - rename a directory inside itself with backtracks'):
+            d = 'test9'
+            src = '%s/dir1/nested' % d
+            dst = '%s/dir1/nested/inner/./../../../dir1/nested/inner/another' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, dst),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s/dir1' % (d),
+            ])
+            assert('nested' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (src),
+            ])
+            assert('inner' in ''.join(output))
+            assert('nested' not in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename10(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 10 - rename a file to itself (successful mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 10 - rename a file to itself'):
+            d = 'test10'
+            src = '%s/file1' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, src),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s' % (ADDR, src),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('file1' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test10'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
+
+    def test_rename11(self, u_boot_console, fs_obj_rename):
+        """
+        Test Case 11 - rename a directory to itself (successful mv)
+        """
+        fs_type, fs_img, md5val = fs_obj_rename
+        with u_boot_console.log.section('Test Case 11 - rename a directory to itself'):
+            # / at the end here is intentional. Ensures trailing / doesn't
+            # affect mv producing an updated dst path for fs_rename
+            d = 'test11/'
+            src = '%sdir1' % d
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'mv host 0:0 %s %s' % (src, d),
+            ])
+            assert('' == ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'load host 0:0 %x /%s/file1' % (ADDR, src),
+                'printenv filesize'])
+            assert('filesize=400' in output)
+
+            output = u_boot_console.run_command_list([
+                'ls host 0:0 %s' % (d),
+            ])
+            assert('dir1' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val['test11'] in ''.join(output))
+            assert_fs_integrity(fs_type, fs_img)
-- 
2.34.1

[PATCH v2 4/6] fs: fat: update parent dirs metadata on dentry create/delete

[Gabriel Dalimonte]

POSIX filesystem functions that create or remove directory entries contain
text along the lines of "[function] shall mark for update the last data
modification and last file status change timestamps of the parent
directory of each file." [1][2][3] The common theme is these timestamp
updates occur when a directory entry is added or removed. The
create_link() and delete_dentry_link() functions have been changed to
update the modification timestamp on the directory where the direntry
change occurs. This differs slightly from Linux in the case of rename(),
where Linux will not update `new_path`'s parent directory's timestamp if
it is replacing an existing file. (via `vfat_add_entry` [4])

The timestamps are not updated if the build configuration does not support
RTCs. This is an effort to minimize introducing erratic timestamps where
they would go from [current date] -> 2000-01-01 (error timestamp in the
FAT driver). I would assume an unchanged timestamp would be more valuable
than a default timestamp in these cases.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html
[2] https://pubs.opengroup.org/onlinepubs/9799919799/functions/unlink.html
[3] https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html
[4] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L682

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>

---

Changes in v2:
 - remove setting ATTR_ARCH on parent dirs during renames
 - skip updating timestamps if no RTC present
 - make fat_itr stack allocated in update_parent_dir_props
 - update parent dir props in create_link and delete_dentry_link


---
 fs/fat/fat_write.c | 78 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index d4952e259ff..0b924541187 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1237,6 +1237,64 @@ static int fat_itr_parent(fat_itr *itr)
 	return fat_itr_resolve(itr, "..", TYPE_DIR);
 }
 
+/**
+ * update_parent_dir_props - updates the modified time for the parent directory
+ *
+ * @dir_itr:	iterator positioned anywhere in a directory whose parent
+ * should be updated
+ * @Return:	0 for success, -errno otherwise
+ */
+static int update_parent_dir_props(fat_itr *dir_itr)
+{
+	int ret = 0;
+
+	fat_itr itr;
+	fsdata fsdata = { .fatbuf = NULL, }, *mydata = &fsdata;
+	__u32 target_clust = dir_itr->start_clust;
+
+	/* Short circuit if no RTC because it only updates timestamps */
+	if (!CONFIG_IS_ENABLED(DM_RTC))
+		return ret;
+
+	/* duplicate fsdata */
+	itr = *dir_itr;
+	fsdata = *itr.fsdata;
+
+	/* allocate local fat buffer */
+	fsdata.fatbuf = malloc_cache_aligned(FATBUFSIZE);
+	if (!fsdata.fatbuf) {
+		log_debug("Error: allocating memory\n");
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	fsdata.fatbufnum = -1;
+	itr.fsdata = &fsdata;
+
+	if (!itr.is_root) {
+		ret = fat_itr_parent(&itr);
+		if (ret)
+			goto exit;
+
+		while (fat_itr_next(&itr)) {
+			if (START(itr.dent) == target_clust)
+				goto update;
+		}
+
+		/* dent not found */
+		ret = -EIO;
+		goto exit;
+update:
+		dentry_set_time(itr.dent);
+		ret = flush_dir(&itr);
+	}
+
+exit:
+	free(fsdata.fatbuf);
+
+	return ret;
+}
+
 /**
  * create_link() - inserts a directory entry for a file or directory
  *
@@ -1270,8 +1328,9 @@ static int create_link(fat_itr *itr, char *basename, __u32 clust, __u32 size,
 	}
 
 	fill_dentry(itr->fsdata, itr->dent, shortname, clust, size, attr);
+	ret = update_parent_dir_props(itr);
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -1612,20 +1671,25 @@ static int delete_long_name(fat_itr *itr)
  */
 static int delete_dentry_link(fat_itr *itr)
 {
+	int ret;
+
 	itr->dent = itr->dent_start;
 	itr->remaining = itr->dent_rem;
 	/* Delete long name */
 	if ((itr->dent->attr & ATTR_VFAT) == ATTR_VFAT &&
 	    (itr->dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
-		int ret;
-
 		ret = delete_long_name(itr);
 		if (ret)
 			return ret;
 	}
 	/* Delete short name */
 	delete_single_dentry(itr);
-	return flush_dir(itr);
+
+	ret = flush_dir(itr);
+	if (ret)
+		return ret;
+
+	return update_parent_dir_props(itr);
 }
 
 /**
@@ -2047,6 +2111,10 @@ int fat_rename(const char *old_path, const char *new_path)
 		*new_itr->dent = *old_itr->dent;
 		new_itr->dent->nameext = new_name;
 		new_itr->dent->lcase = lcase;
+
+		ret = update_parent_dir_props(new_itr);
+		if (ret)
+			goto exit;
 	} else {
 		/* reset iterator to the start of the directory */
 		ret = fat_move_to_cluster(new_itr, new_itr->start_clust);
@@ -2089,6 +2157,8 @@ int fat_rename(const char *old_path, const char *new_path)
 		ret = flush_dir(new_itr);
 		if (ret)
 			goto exit;
+		/* restore directory location to update parent props below */
+		fat_itr_child(new_itr, new_itr);
 	}
 
 	/* refresh old in case write happened to the same block. */
-- 
2.34.1

[PATCH v2 5/6] efi_loader: move path out of file_handle

[Gabriel Dalimonte]

In order to support renaming via SetInfo(), path must allow for longer
values than what was originally present when file_handle was allocated.

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
---

(no changes since v1)

 lib/efi_loader/efi_file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 201fa5f8f3c..6b15c1f3d27 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -40,7 +40,7 @@ struct file_handle {
 	struct fs_dir_stream *dirs;
 	struct fs_dirent *dent;
 
-	char path[0];
+	char *path;
 };
 #define to_fh(x) container_of(x, struct file_handle, base)
 
@@ -178,6 +178,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 		u64 attributes)
 {
 	struct file_handle *fh;
+	char *path;
 	char f0[MAX_UTF8_PER_UTF16] = {0};
 	int plen = 0;
 	int flen = 0;
@@ -194,11 +195,13 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 		plen = strlen(parent->path) + 1;
 	}
 
+	fh = calloc(1, sizeof(*fh));
 	/* +2 is for null and '/' */
-	fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) + 2);
-	if (!fh)
-		return NULL;
+	path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);
+	if (!fh || !path)
+		goto error;
 
+	fh->path = path;
 	fh->open_mode = open_mode;
 	fh->base = efi_file_handle_protocol;
 	fh->fs = fs;
@@ -245,6 +248,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 	return &fh->base;
 
 error:
+	free(fh->path);
 	free(fh);
 	return NULL;
 }
@@ -368,6 +372,7 @@ out:
 static efi_status_t file_close(struct file_handle *fh)
 {
 	fs_closedir(fh->dirs);
+	free(fh->path);
 	free(fh);
 	return EFI_SUCCESS;
 }
-- 
2.34.1

Re: [PATCH v2 5/6] efi_loader: move path out of file_handle

[Ilias Apalodimas]

Hi Gabriel,

On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
<gabriel.dalimonte@gmail.com> wrote:
>
> In order to support renaming via SetInfo(), path must allow for longer
> values than what was originally present when file_handle was allocated.
>
> Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
> ---
>
> (no changes since v1)
>
>  lib/efi_loader/efi_file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 201fa5f8f3c..6b15c1f3d27 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -40,7 +40,7 @@ struct file_handle {
>         struct fs_dir_stream *dirs;
>         struct fs_dirent *dent;
>
> -       char path[0];
> +       char *path;
>  };
>  #define to_fh(x) container_of(x, struct file_handle, base)
>
> @@ -178,6 +178,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
>                 u64 attributes)
>  {
>         struct file_handle *fh;
> +       char *path;
>         char f0[MAX_UTF8_PER_UTF16] = {0};
>         int plen = 0;
>         int flen = 0;
> @@ -194,11 +195,13 @@ static struct efi_file_handle *file_open(struct file_system *fs,
>                 plen = strlen(parent->path) + 1;
>         }
>
> +       fh = calloc(1, sizeof(*fh));
>         /* +2 is for null and '/' */
> -       fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) + 2);
> -       if (!fh)
> -               return NULL;
> +       path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);

I assume the new allocation gets freed in patch #6 right?
Is there an easy way to include this change in the current patch?
Otherwise, we'll end up not freeing memory on a specific sha commit.
I can live with it if the change is too tricky though.

> +       if (!fh || !path)
> +               goto error;
>
> +       fh->path = path;
>         fh->open_mode = open_mode;
>         fh->base = efi_file_handle_protocol;
>         fh->fs = fs;
> @@ -245,6 +248,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
>         return &fh->base;
>
>  error:
> +       free(fh->path);
>         free(fh);
>         return NULL;
>  }
> @@ -368,6 +372,7 @@ out:
>  static efi_status_t file_close(struct file_handle *fh)
>  {
>         fs_closedir(fh->dirs);
> +       free(fh->path);
>         free(fh);
>         return EFI_SUCCESS;
>  }
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v2 5/6] efi_loader: move path out of file_handle

[Gabriel D'Alimonte]

Hi Ilias,

Thanks for the review!

On Thu, Feb 20, 2025 at 2:55 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Gabriel,
>
> On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
> <gabriel.dalimonte@gmail.com> wrote:
> >
> > In order to support renaming via SetInfo(), path must allow for longer
> > values than what was originally present when file_handle was allocated.
> >
> > Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> >  lib/efi_loader/efi_file.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 201fa5f8f3c..6b15c1f3d27 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -40,7 +40,7 @@ struct file_handle {
> >         struct fs_dir_stream *dirs;
> >         struct fs_dirent *dent;
> >
> > -       char path[0];
> > +       char *path;
> >  };
> >  #define to_fh(x) container_of(x, struct file_handle, base)
> >
> > @@ -178,6 +178,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >                 u64 attributes)
> >  {
> >         struct file_handle *fh;
> > +       char *path;
> >         char f0[MAX_UTF8_PER_UTF16] = {0};
> >         int plen = 0;
> >         int flen = 0;
> > @@ -194,11 +195,13 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >                 plen = strlen(parent->path) + 1;
> >         }
> >
> > +       fh = calloc(1, sizeof(*fh));
> >         /* +2 is for null and '/' */
> > -       fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) + 2);
> > -       if (!fh)
> > -               return NULL;
> > +       path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);
>
> I assume the new allocation gets freed in patch #6 right?
> Is there an easy way to include this change in the current patch?
> Otherwise, we'll end up not freeing memory on a specific sha commit.
> I can live with it if the change is too tricky though.

The `path` variable is assigned to `fh->path`

>
> > +       if (!fh || !path)
> > +               goto error;
> >
> > +       fh->path = path;

here. I used the intermediate `path` variable to collapse calloc
failure handling
into a single conditional. If it is preferable to split the
conditional and eliminate
`path`, I can make that change.

> >         fh->open_mode = open_mode;
> >         fh->base = efi_file_handle_protocol;
> >         fh->fs = fs;
> > @@ -245,6 +248,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >         return &fh->base;
> >
> >  error:
> > +       free(fh->path);
> >         free(fh);
> >         return NULL;
> >  }
> > @@ -368,6 +372,7 @@ out:
> >  static efi_status_t file_close(struct file_handle *fh)
> >  {
> >         fs_closedir(fh->dirs);
> > +       free(fh->path);
> >         free(fh);
> >         return EFI_SUCCESS;
> >  }
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Thanks,
Gabriel

[PATCH v2 6/6] efi_loader: support file rename in SetInfo()

[Gabriel Dalimonte]

Following the UEFI specification. The specification did not seem to
delineate if file_name was explicitly a file name only, or could
include paths to move the file to a different directory. The more
generous interpretation of supporting paths was selected.

Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>

---

Changes in v2:
 - simplify freeing of new_file_name and new_path


---
 lib/efi_loader/efi_file.c | 45 +++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 6b15c1f3d27..7d81da8f2d8 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -954,6 +954,7 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
 {
 	struct file_handle *fh = to_fh(file);
 	efi_status_t ret = EFI_UNSUPPORTED;
+	char *new_file_name = NULL, *new_path = NULL;
 
 	EFI_ENTRY("%p, %pUs, %zu, %p", file, info_type, buffer_size, buffer);
 
@@ -983,13 +984,43 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
 		pos = new_file_name;
 		utf16_utf8_strcpy(&pos, info->file_name);
 		if (strcmp(new_file_name, filename)) {
-			/* TODO: we do not support renaming */
-			EFI_PRINT("Renaming not supported\n");
-			free(new_file_name);
-			ret = EFI_ACCESS_DENIED;
-			goto out;
+			int dlen;
+			int rv;
+
+			if (set_blk_dev(fh)) {
+				ret = EFI_DEVICE_ERROR;
+				goto out;
+			}
+			dlen = filename - fh->path;
+			new_path = calloc(1, dlen + strlen(new_file_name) + 1);
+			if (!new_path) {
+				ret = EFI_OUT_OF_RESOURCES;
+				goto out;
+			}
+			memcpy(new_path, fh->path, dlen);
+			strcpy(new_path + dlen, new_file_name);
+			sanitize_path(new_path);
+			rv = fs_exists(new_path);
+			if (rv) {
+				ret = EFI_ACCESS_DENIED;
+				goto out;
+			}
+			/* fs_exists() calls fs_close(), so open file system again */
+			if (set_blk_dev(fh)) {
+				ret = EFI_DEVICE_ERROR;
+				goto out;
+			}
+			rv = fs_rename(fh->path, new_path);
+			if (rv) {
+				ret = EFI_ACCESS_DENIED;
+				goto out;
+			}
+			free(fh->path);
+			fh->path = new_path;
+			/* Prevent new_path from being freed on out */
+			new_path = NULL;
+			ret = EFI_SUCCESS;
 		}
-		free(new_file_name);
 		/* Check for truncation */
 		if (!fh->isdir) {
 			ret = efi_get_file_size(fh, &file_size);
@@ -1012,6 +1043,8 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
 		ret = EFI_UNSUPPORTED;
 	}
 out:
+	free(new_path);
+	free(new_file_name);
 	return EFI_EXIT(ret);
 }
 
-- 
2.34.1

Re: [PATCH v2 6/6] efi_loader: support file rename in SetInfo()

[Ilias Apalodimas]

Hi Gabriel,

On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
<gabriel.dalimonte@gmail.com> wrote:
>
> Following the UEFI specification. The specification did not seem to
> delineate if file_name was explicitly a file name only, or could
> include paths to move the file to a different directory. The more
> generous interpretation of supporting paths was selected.
>
> Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
>
> ---
>
> Changes in v2:
>  - simplify freeing of new_file_name and new_path
>
>
> ---
>  lib/efi_loader/efi_file.c | 45 +++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 6b15c1f3d27..7d81da8f2d8 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -954,6 +954,7 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
>  {
>         struct file_handle *fh = to_fh(file);
>         efi_status_t ret = EFI_UNSUPPORTED;
> +       char *new_file_name = NULL, *new_path = NULL;
>
>         EFI_ENTRY("%p, %pUs, %zu, %p", file, info_type, buffer_size, buffer);
>
> @@ -983,13 +984,43 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
>                 pos = new_file_name;
>                 utf16_utf8_strcpy(&pos, info->file_name);
>                 if (strcmp(new_file_name, filename)) {
> -                       /* TODO: we do not support renaming */
> -                       EFI_PRINT("Renaming not supported\n");
> -                       free(new_file_name);
> -                       ret = EFI_ACCESS_DENIED;
> -                       goto out;
> +                       int dlen;
> +                       int rv;
> +
> +                       if (set_blk_dev(fh)) {
> +                               ret = EFI_DEVICE_ERROR;
> +                               goto out;
> +                       }
> +                       dlen = filename - fh->path;
> +                       new_path = calloc(1, dlen + strlen(new_file_name) + 1);
> +                       if (!new_path) {
> +                               ret = EFI_OUT_OF_RESOURCES;
> +                               goto out;
> +                       }
> +                       memcpy(new_path, fh->path, dlen);
> +                       strcpy(new_path + dlen, new_file_name);
> +                       sanitize_path(new_path);
> +                       rv = fs_exists(new_path);
> +                       if (rv) {
> +                               ret = EFI_ACCESS_DENIED;
> +                               goto out;
> +                       }
> +                       /* fs_exists() calls fs_close(), so open file system again */
> +                       if (set_blk_dev(fh)) {
> +                               ret = EFI_DEVICE_ERROR;
> +                               goto out;
> +                       }
> +                       rv = fs_rename(fh->path, new_path);
> +                       if (rv) {
> +                               ret = EFI_ACCESS_DENIED;
> +                               goto out;
> +                       }
> +                       free(fh->path);
> +                       fh->path = new_path;
> +                       /* Prevent new_path from being freed on out */
> +                       new_path = NULL;

fh->path is also NULL now. I am not sure where these are used but this
seems wrong.

> +                       ret = EFI_SUCCESS;
>                 }
> -               free(new_file_name);
>                 /* Check for truncation */
>                 if (!fh->isdir) {
>                         ret = efi_get_file_size(fh, &file_size);
> @@ -1012,6 +1043,8 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
>                 ret = EFI_UNSUPPORTED;
>         }
>  out:
> +       free(new_path);
> +       free(new_file_name);
>         return EFI_EXIT(ret);
>  }
>
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v2 6/6] efi_loader: support file rename in SetInfo()

[Ilias Apalodimas]

On Thu, 20 Feb 2025 at 10:21, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Gabriel,
>
> On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
> <gabriel.dalimonte@gmail.com> wrote:
> >
> > Following the UEFI specification. The specification did not seem to
> > delineate if file_name was explicitly a file name only, or could
> > include paths to move the file to a different directory. The more
> > generous interpretation of supporting paths was selected.
> >
> > Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
> >
> > ---
> >
> > Changes in v2:
> >  - simplify freeing of new_file_name and new_path
> >
> >
> > ---
> >  lib/efi_loader/efi_file.c | 45 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 6b15c1f3d27..7d81da8f2d8 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -954,6 +954,7 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
> >  {
> >         struct file_handle *fh = to_fh(file);
> >         efi_status_t ret = EFI_UNSUPPORTED;
> > +       char *new_file_name = NULL, *new_path = NULL;
> >
> >         EFI_ENTRY("%p, %pUs, %zu, %p", file, info_type, buffer_size, buffer);
> >
> > @@ -983,13 +984,43 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
> >                 pos = new_file_name;
> >                 utf16_utf8_strcpy(&pos, info->file_name);
> >                 if (strcmp(new_file_name, filename)) {
> > -                       /* TODO: we do not support renaming */
> > -                       EFI_PRINT("Renaming not supported\n");
> > -                       free(new_file_name);
> > -                       ret = EFI_ACCESS_DENIED;
> > -                       goto out;
> > +                       int dlen;
> > +                       int rv;
> > +
> > +                       if (set_blk_dev(fh)) {
> > +                               ret = EFI_DEVICE_ERROR;
> > +                               goto out;
> > +                       }
> > +                       dlen = filename - fh->path;
> > +                       new_path = calloc(1, dlen + strlen(new_file_name) + 1);
> > +                       if (!new_path) {
> > +                               ret = EFI_OUT_OF_RESOURCES;
> > +                               goto out;
> > +                       }
> > +                       memcpy(new_path, fh->path, dlen);
> > +                       strcpy(new_path + dlen, new_file_name);
> > +                       sanitize_path(new_path);
> > +                       rv = fs_exists(new_path);
> > +                       if (rv) {
> > +                               ret = EFI_ACCESS_DENIED;
> > +                               goto out;
> > +                       }
> > +                       /* fs_exists() calls fs_close(), so open file system again */
> > +                       if (set_blk_dev(fh)) {
> > +                               ret = EFI_DEVICE_ERROR;
> > +                               goto out;
> > +                       }
> > +                       rv = fs_rename(fh->path, new_path);
> > +                       if (rv) {
> > +                               ret = EFI_ACCESS_DENIED;
> > +                               goto out;
> > +                       }
> > +                       free(fh->path);
> > +                       fh->path = new_path;
> > +                       /* Prevent new_path from being freed on out */
> > +                       new_path = NULL;
>
> fh->path is also NULL now. I am not sure where these are used but this
> seems wrong.

Right ignore me, this is comment is obviously wrong and the patch is fine

>
> > +                       ret = EFI_SUCCESS;
> >                 }
> > -               free(new_file_name);
> >                 /* Check for truncation */
> >                 if (!fh->isdir) {
> >                         ret = efi_get_file_size(fh, &file_size);
> > @@ -1012,6 +1043,8 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
> >                 ret = EFI_UNSUPPORTED;
> >         }
> >  out:
> > +       free(new_path);
> > +       free(new_file_name);
> >         return EFI_EXIT(ret);
> >  }
> >
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 0/6] This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().

[Tom Rini]

On Mon, 17 Feb 2025 13:26:41 -0500, Gabriel Dalimonte wrote:

> This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().
> One of the use cases for renaming in EFI is to facilitate boot loader
> boot counting.
> 
> No existing filesystems in U-Boot currently include file renaming,
> resulting in support for renaming at the filesystem level and a
> concrete implementation for the FAT filesystem.
> 
> [...]

Applied to u-boot/next, thanks!

[1/6] fs: fat: factor out dentry link create/delete
      commit: 1742b8484e797c56c720c24fa7b923a6d7275cc6
[2/6] fs: add rename infrastructure
      commit: d9c149664fa7a0c2eabfc046dcf89637f655364b
[3/6] fs: fat: add rename
      commit: 06159a1465fc97d8d7b72b9bea39a396f6e7057c
[4/6] fs: fat: update parent dirs metadata on dentry create/delete
      commit: 879eee641b6ddd4cd8299cb14803766935536b3e
[5/6] efi_loader: move path out of file_handle
      commit: 8465ee528b84f242e403a3c6e67dab5b244bacc8
[6/6] efi_loader: support file rename in SetInfo()
      commit: 0165e1a8bd80ee91216e901064bfd4b0ca7f623a
-- 
Tom