linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..'
@ 2025-07-12 18:12 Theodore Ts'o
  2025-07-12 18:12 ` [PATCH 2/3] ext4: use memcpy() instead of strcpy() Theodore Ts'o
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Theodore Ts'o @ 2025-07-12 18:12 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: linux-hardening, ethan, Theodore Ts'o

In a discussion over a proposed patch, "ext4: replace strcpy() with
'.' assignment"[1], I had asserted that directory entries in ext4 were
not NUL terminated, and hence it was safe to replace strcpy() with a
direct assignment.  As it turns out, this was incorrect.  It's true
for all all directory entries *except* for '.' and '..' where the
kernel was using strcmp() and where e2fsck actually checks and offers
to fix things if '.'  and '..' are not NUL terminated.

[1] https://lore.kernel.org/r/202505191316.JJMnPobO-lkp@intel.com

We can't change this without breaking old kernel versions, but in the
spirit of "be liberal in what you receive", use direct comparison of
de->name_len and de->name[0,1] instead of strcmp().  This has the side
benefit of reducing the compiled text size by 96 bytes on x86_64.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/namei.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index a178ac229489..b82f5841c65a 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3082,7 +3082,8 @@ bool ext4_empty_dir(struct inode *inode)
 	de = (struct ext4_dir_entry_2 *) bh->b_data;
 	if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data, bh->b_size,
 				 0) ||
-	    le32_to_cpu(de->inode) != inode->i_ino || strcmp(".", de->name)) {
+	    le32_to_cpu(de->inode) != inode->i_ino || de->name_len != 1 ||
+	    de->name[0] != '.') {
 		ext4_warning_inode(inode, "directory missing '.'");
 		brelse(bh);
 		return false;
@@ -3091,7 +3092,8 @@ bool ext4_empty_dir(struct inode *inode)
 	de = ext4_next_entry(de, sb->s_blocksize);
 	if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data, bh->b_size,
 				 offset) ||
-	    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
+	    le32_to_cpu(de->inode) == 0 || de->name_len != 2 ||
+	    de->name[0] != '.' || de->name[1] != '.') {
 		ext4_warning_inode(inode, "directory missing '..'");
 		brelse(bh);
 		return false;
@@ -3532,7 +3534,7 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
 					 bh->b_size, 0) ||
 		    le32_to_cpu(de->inode) != inode->i_ino ||
-		    strcmp(".", de->name)) {
+		    de->name_len != 1 || de->name[0] != '.') {
 			EXT4_ERROR_INODE(inode, "directory missing '.'");
 			brelse(bh);
 			*retval = -EFSCORRUPTED;
@@ -3543,7 +3545,8 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
 		de = ext4_next_entry(de, inode->i_sb->s_blocksize);
 		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
 					 bh->b_size, offset) ||
-		    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
+		    le32_to_cpu(de->inode) == 0 || de->name_len != 2 ||
+		    de->name[0] != '.' || de->name[1] != '.') {
 			EXT4_ERROR_INODE(inode, "directory missing '..'");
 			brelse(bh);
 			*retval = -EFSCORRUPTED;
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] ext4: use memcpy() instead of strcpy()
  2025-07-12 18:12 [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
@ 2025-07-12 18:12 ` Theodore Ts'o
  2025-07-30 15:02   ` Andy Shevchenko
  2025-07-12 18:12 ` [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths Theodore Ts'o
  2025-07-19 21:45 ` [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
  2 siblings, 1 reply; 8+ messages in thread
From: Theodore Ts'o @ 2025-07-12 18:12 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: linux-hardening, ethan, Theodore Ts'o

The strcpy() function is considered dangerous and eeeevil by people
who are using sophisticated code analysis tools such as "grep".  This
is true even when a quick inspection would show that the source is a
constant string ("." or "..") and the destination is a fixed array
which is guaranteed to have enough space.  Make the "grep" code
analysis tool happy by using memcpy() isstead of strcpy().  :-)

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/inline.c | 4 ++--
 fs/ext4/namei.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index a1bbcdf40824..eeee007251e0 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1315,7 +1315,7 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
 		if (pos == 0) {
 			fake.inode = cpu_to_le32(inode->i_ino);
 			fake.name_len = 1;
-			strcpy(fake.name, ".");
+			memcpy(fake.name, ".", 2);
 			fake.rec_len = ext4_rec_len_to_disk(
 					  ext4_dir_rec_len(fake.name_len, NULL),
 					  inline_size);
@@ -1325,7 +1325,7 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
 		} else if (pos == EXT4_INLINE_DOTDOT_OFFSET) {
 			fake.inode = cpu_to_le32(parent_ino);
 			fake.name_len = 2;
-			strcpy(fake.name, "..");
+			memcpy(fake.name, "..", 3);
 			fake.rec_len = ext4_rec_len_to_disk(
 					  ext4_dir_rec_len(fake.name_len, NULL),
 					  inline_size);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index b82f5841c65a..9913a94b6a6d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2924,7 +2924,7 @@ struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
 	de->name_len = 1;
 	de->rec_len = ext4_rec_len_to_disk(ext4_dir_rec_len(de->name_len, NULL),
 					   blocksize);
-	strcpy(de->name, ".");
+	memcpy(de->name, ".", 2);
 	ext4_set_de_type(inode->i_sb, de, S_IFDIR);
 
 	de = ext4_next_entry(de, blocksize);
@@ -2938,7 +2938,7 @@ struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
 		de->rec_len = ext4_rec_len_to_disk(
 					ext4_dir_rec_len(de->name_len, NULL),
 					blocksize);
-	strcpy(de->name, "..");
+	memcpy(de->name, "..", 3);
 	ext4_set_de_type(inode->i_sb, de, S_IFDIR);
 
 	return ext4_next_entry(de, blocksize);
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths
  2025-07-12 18:12 [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
  2025-07-12 18:12 ` [PATCH 2/3] ext4: use memcpy() instead of strcpy() Theodore Ts'o
@ 2025-07-12 18:12 ` Theodore Ts'o
  2025-07-12 21:12   ` kernel test robot
  2025-07-19 21:45 ` [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
  2 siblings, 1 reply; 8+ messages in thread
From: Theodore Ts'o @ 2025-07-12 18:12 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: linux-hardening, ethan, Theodore Ts'o

There was a lot of common code in the codepaths used to convert an
inline directory and to creaet a new directory.  To address this,
rename ext4_init_dot_dotdot() to ext4_init_dirblock() and then move
common code into that function.

This reduces the lines of code count in fs/ext4/inline.c and
fs/ext4/namei.c, as well as reducing the size of their object files.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/ext4.h   |  9 ++++----
 fs/ext4/inline.c | 60 ++++++++++--------------------------------------
 fs/ext4/namei.c  | 56 ++++++++++++++++++++++++--------------------
 3 files changed, 48 insertions(+), 77 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 18373de980f2..4d1d3eff2418 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3612,6 +3612,7 @@ extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin);
 extern int ext4_get_max_inline_size(struct inode *inode);
 extern int ext4_find_inline_data_nolock(struct inode *inode);
 extern int ext4_destroy_inline_data(handle_t *handle, struct inode *inode);
+extern void ext4_update_final_de(void *de_buf, int old_size, int new_size);
 
 int ext4_readpage_inline(struct inode *inode, struct folio *folio);
 extern int ext4_try_to_write_inline_data(struct address_space *mapping,
@@ -3671,10 +3672,10 @@ static inline int ext4_has_inline_data(struct inode *inode)
 extern const struct inode_operations ext4_dir_inode_operations;
 extern const struct inode_operations ext4_special_inode_operations;
 extern struct dentry *ext4_get_parent(struct dentry *child);
-extern struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
-				 struct ext4_dir_entry_2 *de,
-				 int blocksize, int csum_size,
-				 unsigned int parent_ino, int dotdot_real_len);
+extern int ext4_init_dirblock(handle_t *handle, struct inode *inode,
+			      struct buffer_head *dir_block,
+			      unsigned int parent_ino, void *inline_buf,
+			      int inline_size);
 extern void ext4_initialize_dirent_tail(struct buffer_head *bh,
 					unsigned int blocksize);
 extern int ext4_handle_dirty_dirblock(handle_t *handle, struct inode *inode,
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index eeee007251e0..4ad3255e45cb 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -995,7 +995,7 @@ static void *ext4_get_inline_xattr_pos(struct inode *inode,
 }
 
 /* Set the final de to cover the whole block. */
-static void ext4_update_final_de(void *de_buf, int old_size, int new_size)
+void ext4_update_final_de(void *de_buf, int old_size, int new_size)
 {
 	struct ext4_dir_entry_2 *de, *prev_de;
 	void *limit;
@@ -1059,51 +1059,6 @@ static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,
 	ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
 }
 
-static int ext4_finish_convert_inline_dir(handle_t *handle,
-					  struct inode *inode,
-					  struct buffer_head *dir_block,
-					  void *buf,
-					  int inline_size)
-{
-	int err, csum_size = 0, header_size = 0;
-	struct ext4_dir_entry_2 *de;
-	void *target = dir_block->b_data;
-
-	/*
-	 * First create "." and ".." and then copy the dir information
-	 * back to the block.
-	 */
-	de = target;
-	de = ext4_init_dot_dotdot(inode, de,
-		inode->i_sb->s_blocksize, csum_size,
-		le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode), 1);
-	header_size = (void *)de - target;
-
-	memcpy((void *)de, buf + EXT4_INLINE_DOTDOT_SIZE,
-		inline_size - EXT4_INLINE_DOTDOT_SIZE);
-
-	if (ext4_has_feature_metadata_csum(inode->i_sb))
-		csum_size = sizeof(struct ext4_dir_entry_tail);
-
-	inode->i_size = inode->i_sb->s_blocksize;
-	i_size_write(inode, inode->i_sb->s_blocksize);
-	EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
-	ext4_update_final_de(dir_block->b_data,
-			inline_size - EXT4_INLINE_DOTDOT_SIZE + header_size,
-			inode->i_sb->s_blocksize - csum_size);
-
-	if (csum_size)
-		ext4_initialize_dirent_tail(dir_block,
-					    inode->i_sb->s_blocksize);
-	set_buffer_uptodate(dir_block);
-	unlock_buffer(dir_block);
-	err = ext4_handle_dirty_dirblock(handle, inode, dir_block);
-	if (err)
-		return err;
-	set_buffer_verified(dir_block);
-	return ext4_mark_inode_dirty(handle, inode);
-}
-
 static int ext4_convert_inline_data_nolock(handle_t *handle,
 					   struct inode *inode,
 					   struct ext4_iloc *iloc)
@@ -1175,8 +1130,17 @@ static int ext4_convert_inline_data_nolock(handle_t *handle,
 		error = ext4_handle_dirty_metadata(handle,
 						   inode, data_bh);
 	} else {
-		error = ext4_finish_convert_inline_dir(handle, inode, data_bh,
-						       buf, inline_size);
+		unlock_buffer(data_bh);
+		inode->i_size = inode->i_sb->s_blocksize;
+		i_size_write(inode, inode->i_sb->s_blocksize);
+		EXT4_I(inode)->i_disksize = inode->i_sb->s_blocksize;
+
+		error = ext4_init_dirblock(handle, inode, data_bh,
+			  le32_to_cpu(((struct ext4_dir_entry_2 *)buf)->inode),
+			  buf + EXT4_INLINE_DOTDOT_SIZE,
+			  inline_size - EXT4_INLINE_DOTDOT_SIZE);
+		if (!error)
+			error = ext4_mark_inode_dirty(handle, inode);
 	}
 
 out_restore:
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 9913a94b6a6d..d83f91b62317 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2915,11 +2915,17 @@ static int ext4_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 	return err;
 }
 
-struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
-			  struct ext4_dir_entry_2 *de,
-			  int blocksize, int csum_size,
-			  unsigned int parent_ino, int dotdot_real_len)
+int ext4_init_dirblock(handle_t *handle, struct inode *inode,
+		       struct buffer_head *bh, unsigned int parent_ino,
+		       void *inline_buf, int inline_size)
 {
+	struct ext4_dir_entry_2 *de = (struct ext4_dir_entry_2 *) bh->b_data;
+	size_t			blocksize = bh->b_size;
+	int			csum_size = 0, header_size;
+
+	if (ext4_has_feature_metadata_csum(inode->i_sb))
+		csum_size = sizeof(struct ext4_dir_entry_tail);
+
 	de->inode = cpu_to_le32(inode->i_ino);
 	de->name_len = 1;
 	de->rec_len = ext4_rec_len_to_disk(ext4_dir_rec_len(de->name_len, NULL),
@@ -2930,18 +2936,29 @@ struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
 	de = ext4_next_entry(de, blocksize);
 	de->inode = cpu_to_le32(parent_ino);
 	de->name_len = 2;
-	if (!dotdot_real_len)
-		de->rec_len = ext4_rec_len_to_disk(blocksize -
-					(csum_size + ext4_dir_rec_len(1, NULL)),
-					blocksize);
-	else
+	memcpy(de->name, "..", 3);
+	ext4_set_de_type(inode->i_sb, de, S_IFDIR);
+	if (inline_buf) {
 		de->rec_len = ext4_rec_len_to_disk(
 					ext4_dir_rec_len(de->name_len, NULL),
 					blocksize);
-	memcpy(de->name, "..", 3);
-	ext4_set_de_type(inode->i_sb, de, S_IFDIR);
+		de = ext4_next_entry(de, blocksize);
+		header_size = (char *)de - bh->b_data;
+		memcpy((void *)de, inline_buf, inline_size);
+		ext4_update_final_de(bh->b_data, inline_size + header_size,
+			blocksize - csum_size);
+	} else {
+		de->rec_len = ext4_rec_len_to_disk(blocksize -
+					(csum_size + ext4_dir_rec_len(1, NULL)),
+					blocksize);
+	}
 
-	return ext4_next_entry(de, blocksize);
+	if (csum_size)
+		ext4_initialize_dirent_tail(bh, blocksize);
+	BUFFER_TRACE(dir_block, "call ext4_handle_dirty_metadata");
+	set_buffer_uptodate(bh);
+	set_buffer_verified(bh);
+	return ext4_handle_dirty_dirblock(handle, inode, bh);
 }
 
 int ext4_init_new_dir(handle_t *handle, struct inode *dir,
@@ -2950,13 +2967,8 @@ int ext4_init_new_dir(handle_t *handle, struct inode *dir,
 	struct buffer_head *dir_block = NULL;
 	struct ext4_dir_entry_2 *de;
 	ext4_lblk_t block = 0;
-	unsigned int blocksize = dir->i_sb->s_blocksize;
-	int csum_size = 0;
 	int err;
 
-	if (ext4_has_feature_metadata_csum(dir->i_sb))
-		csum_size = sizeof(struct ext4_dir_entry_tail);
-
 	if (ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
 		err = ext4_try_create_inline_dir(handle, dir, inode);
 		if (err < 0 && err != -ENOSPC)
@@ -2965,21 +2977,15 @@ int ext4_init_new_dir(handle_t *handle, struct inode *dir,
 			goto out;
 	}
 
+	set_nlink(inode, 2);
 	inode->i_size = 0;
 	dir_block = ext4_append(handle, inode, &block);
 	if (IS_ERR(dir_block))
 		return PTR_ERR(dir_block);
 	de = (struct ext4_dir_entry_2 *)dir_block->b_data;
-	ext4_init_dot_dotdot(inode, de, blocksize, csum_size, dir->i_ino, 0);
-	set_nlink(inode, 2);
-	if (csum_size)
-		ext4_initialize_dirent_tail(dir_block, blocksize);
-
-	BUFFER_TRACE(dir_block, "call ext4_handle_dirty_metadata");
-	err = ext4_handle_dirty_dirblock(handle, inode, dir_block);
+	err = ext4_init_dirblock(handle, inode, dir_block, dir->i_ino, NULL, 0);
 	if (err)
 		goto out;
-	set_buffer_verified(dir_block);
 out:
 	brelse(dir_block);
 	return err;
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths
  2025-07-12 18:12 ` [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths Theodore Ts'o
@ 2025-07-12 21:12   ` kernel test robot
  2025-07-21 23:15     ` Eric Biggers
  0 siblings, 1 reply; 8+ messages in thread
From: kernel test robot @ 2025-07-12 21:12 UTC (permalink / raw)
  To: Theodore Ts'o, Ext4 Developers List
  Cc: oe-kbuild-all, linux-hardening, ethan, Theodore Ts'o

Hi Theodore,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v6.16-rc5 next-20250711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Theodore-Ts-o/ext4-use-memcpy-instead-of-strcpy/20250713-021635
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20250712181249.434530-3-tytso%40mit.edu
patch subject: [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths
config: i386-buildonly-randconfig-004-20250713 (https://download.01.org/0day-ci/archive/20250713/202507130429.rPIzofCD-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250713/202507130429.rPIzofCD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507130429.rPIzofCD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/ext4/namei.c: In function 'ext4_init_new_dir':
>> fs/ext4/namei.c:2968:34: warning: variable 'de' set but not used [-Wunused-but-set-variable]
    2968 |         struct ext4_dir_entry_2 *de;
         |                                  ^~


vim +/de +2968 fs/ext4/namei.c

a774f9c20e0864 Tao Ma             2012-12-10  2963  
8016e29f4362e2 Harshad Shirwadkar 2020-10-15  2964  int ext4_init_new_dir(handle_t *handle, struct inode *dir,
a774f9c20e0864 Tao Ma             2012-12-10  2965  			     struct inode *inode)
ac27a0ec112a08 Dave Kleikamp      2006-10-11  2966  {
dabd991f9d8e32 Namhyung Kim       2011-01-10  2967  	struct buffer_head *dir_block = NULL;
617ba13b31fbf5 Mingming Cao       2006-10-11 @2968  	struct ext4_dir_entry_2 *de;
dc6982ff4db1f4 Theodore Ts'o      2013-02-14  2969  	ext4_lblk_t block = 0;
a774f9c20e0864 Tao Ma             2012-12-10  2970  	int err;
ac27a0ec112a08 Dave Kleikamp      2006-10-11  2971  
3c47d54170b6a6 Tao Ma             2012-12-10  2972  	if (ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
3c47d54170b6a6 Tao Ma             2012-12-10  2973  		err = ext4_try_create_inline_dir(handle, dir, inode);
3c47d54170b6a6 Tao Ma             2012-12-10  2974  		if (err < 0 && err != -ENOSPC)
3c47d54170b6a6 Tao Ma             2012-12-10  2975  			goto out;
3c47d54170b6a6 Tao Ma             2012-12-10  2976  		if (!err)
3c47d54170b6a6 Tao Ma             2012-12-10  2977  			goto out;
3c47d54170b6a6 Tao Ma             2012-12-10  2978  	}
3c47d54170b6a6 Tao Ma             2012-12-10  2979  
5eb8361f6b02c3 Theodore Ts'o      2025-07-12  2980  	set_nlink(inode, 2);
dc6982ff4db1f4 Theodore Ts'o      2013-02-14  2981  	inode->i_size = 0;
0f70b40613ee14 Theodore Ts'o      2013-02-15  2982  	dir_block = ext4_append(handle, inode, &block);
0f70b40613ee14 Theodore Ts'o      2013-02-15  2983  	if (IS_ERR(dir_block))
0f70b40613ee14 Theodore Ts'o      2013-02-15  2984  		return PTR_ERR(dir_block);
a774f9c20e0864 Tao Ma             2012-12-10  2985  	de = (struct ext4_dir_entry_2 *)dir_block->b_data;
5eb8361f6b02c3 Theodore Ts'o      2025-07-12  2986  	err = ext4_init_dirblock(handle, inode, dir_block, dir->i_ino, NULL, 0);
a774f9c20e0864 Tao Ma             2012-12-10  2987  	if (err)
a774f9c20e0864 Tao Ma             2012-12-10  2988  		goto out;
a774f9c20e0864 Tao Ma             2012-12-10  2989  out:
a774f9c20e0864 Tao Ma             2012-12-10  2990  	brelse(dir_block);
a774f9c20e0864 Tao Ma             2012-12-10  2991  	return err;
a774f9c20e0864 Tao Ma             2012-12-10  2992  }
a774f9c20e0864 Tao Ma             2012-12-10  2993  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..'
  2025-07-12 18:12 [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
  2025-07-12 18:12 ` [PATCH 2/3] ext4: use memcpy() instead of strcpy() Theodore Ts'o
  2025-07-12 18:12 ` [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths Theodore Ts'o
@ 2025-07-19 21:45 ` Theodore Ts'o
  2 siblings, 0 replies; 8+ messages in thread
From: Theodore Ts'o @ 2025-07-19 21:45 UTC (permalink / raw)
  To: Ext4 Developers List, Theodore Ts'o; +Cc: linux-hardening, ethan


On Sat, 12 Jul 2025 14:12:47 -0400, Theodore Ts'o wrote:
> In a discussion over a proposed patch, "ext4: replace strcpy() with
> '.' assignment"[1], I had asserted that directory entries in ext4 were
> not NUL terminated, and hence it was safe to replace strcpy() with a
> direct assignment.  As it turns out, this was incorrect.  It's true
> for all all directory entries *except* for '.' and '..' where the
> kernel was using strcmp() and where e2fsck actually checks and offers
> to fix things if '.'  and '..' are not NUL terminated.
> 
> [...]

Applied, thanks!

[1/3] ext4: replace strcmp with direct comparison for '.' and '..'
      commit: 3658b8b3398eb2a49ee8d1ac88e5cdc41764f1c9
[2/3] ext4: use memcpy() instead of strcpy()
      commit: a35454ecf8a320c49954fdcdae0e8d3323067632
[3/3] ext4: refactor the inline directory conversion and new directory codepaths
      commit: 90f097b1403f232a202c501bfd49b1b196e840ea

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths
  2025-07-12 21:12   ` kernel test robot
@ 2025-07-21 23:15     ` Eric Biggers
  2025-07-30 15:01       ` Andy Shevchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2025-07-21 23:15 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kernel test robot, Ext4 Developers List, oe-kbuild-all,
	linux-hardening, ethan

Hi Ted,

On Sun, Jul 13, 2025 at 05:12:55AM +0800, kernel test robot wrote:
> All warnings (new ones prefixed by >>):
> 
>    fs/ext4/namei.c: In function 'ext4_init_new_dir':
> >> fs/ext4/namei.c:2968:34: warning: variable 'de' set but not used [-Wunused-but-set-variable]
>     2968 |         struct ext4_dir_entry_2 *de;
>          |                                  ^~

This warning is present in ext4/dev now.

- Eric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths
  2025-07-21 23:15     ` Eric Biggers
@ 2025-07-30 15:01       ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-07-30 15:01 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Ts'o, kernel test robot, Ext4 Developers List,
	oe-kbuild-all, linux-hardening, ethan

On Mon, Jul 21, 2025 at 04:15:59PM -0700, Eric Biggers wrote:
> On Sun, Jul 13, 2025 at 05:12:55AM +0800, kernel test robot wrote:
> > All warnings (new ones prefixed by >>):
> > 
> >    fs/ext4/namei.c: In function 'ext4_init_new_dir':
> > >> fs/ext4/namei.c:2968:34: warning: variable 'de' set but not used [-Wunused-but-set-variable]
> >     2968 |         struct ext4_dir_entry_2 *de;
> >          |                                  ^~
> 
> This warning is present in ext4/dev now.

For somebody (who doesn't alter CONFIG_WERROR) this is a build breakage with both GCC and clang.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] ext4: use memcpy() instead of strcpy()
  2025-07-12 18:12 ` [PATCH 2/3] ext4: use memcpy() instead of strcpy() Theodore Ts'o
@ 2025-07-30 15:02   ` Andy Shevchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-07-30 15:02 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List, linux-hardening, ethan

On Sat, Jul 12, 2025 at 02:12:48PM -0400, Theodore Ts'o wrote:
> The strcpy() function is considered dangerous and eeeevil by people
> who are using sophisticated code analysis tools such as "grep".  This
> is true even when a quick inspection would show that the source is a
> constant string ("." or "..") and the destination is a fixed array
> which is guaranteed to have enough space.  Make the "grep" code
> analysis tool happy by using memcpy() isstead of strcpy().  :-)

Why simple 2-arg strscpy() can't be used?

...

> -			strcpy(fake.name, ".");
> +			memcpy(fake.name, ".", 2);

s/strcpy/strscpy/

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-07-30 14:02 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-12 18:12 [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o
2025-07-12 18:12 ` [PATCH 2/3] ext4: use memcpy() instead of strcpy() Theodore Ts'o
2025-07-30 15:02   ` Andy Shevchenko
2025-07-12 18:12 ` [PATCH 3/3] ext4: refactor the inline directory conversion and new directory codepaths Theodore Ts'o
2025-07-12 21:12   ` kernel test robot
2025-07-21 23:15     ` Eric Biggers
2025-07-30 15:01       ` Andy Shevchenko
2025-07-19 21:45 ` [PATCH 1/3] ext4: replace strcmp with direct comparison for '.' and '..' Theodore Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).