On 30. Jul 2024, at 20:12, Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx> wrote: > On 30/07/24 11:23, Thorsten Blum wrote: >> Add the __counted_by compiler attribute to the flexible array member >> inodes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and >> CONFIG_FORTIFY_SOURCE. >> Remove the now obsolete comment on the count field. >> Refactor ext4_expand_inode_array() by assigning count before copying any >> data using memcpy(). Copy only the inodes array instead of the whole >> struct because count has been set explicitly. >> Use struct_size() and struct_size_t() instead of offsetof(). >> Change the data type of the local variable count to unsigned int to >> match the struct's count data type. >> Compile-tested only. >> Signed-off-by: Thorsten Blum <thorsten.blum@xxxxxxxxxx> >> --- >> Changes in v2: >> - Adjust ext4_expand_inode_array() as suggested by Gustavo A. R. Silva >> - Use struct_size() and struct_size_t() instead of offsetof() >> - Link to v1: https://lore.kernel.org/linux-kernel/20240729110454.346918-3-thorsten.blum@xxxxxxxxxx/ >> --- >> fs/ext4/xattr.c | 20 +++++++++----------- >> fs/ext4/xattr.h | 4 ++-- >> 2 files changed, 11 insertions(+), 13 deletions(-) >> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c >> index 46ce2f21fef9..b27543587103 100644 >> --- a/fs/ext4/xattr.c >> +++ b/fs/ext4/xattr.c >> @@ -2879,11 +2879,10 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array, >> if (*ea_inode_array == NULL) { >> /* >> * Start with 15 inodes, so it fits into a power-of-two size. >> - * If *ea_inode_array is NULL, this is essentially offsetof() >> */ >> (*ea_inode_array) = >> - kmalloc(offsetof(struct ext4_xattr_inode_array, >> - inodes[EIA_MASK]), >> + kmalloc(struct_size_t(struct ext4_xattr_inode_array, >> + inodes, EIA_MASK), > > As Kees previously commented, you can use struct_size() here. > >> GFP_NOFS); >> if (*ea_inode_array == NULL) >> return -ENOMEM; >> @@ -2891,17 +2890,16 @@ ext4_expand_inode_array(struct ext4_xattr_inode_array **ea_inode_array, >> } else if (((*ea_inode_array)->count & EIA_MASK) == EIA_MASK) { >> /* expand the array once all 15 + n * 16 slots are full */ >> struct ext4_xattr_inode_array *new_array = NULL; >> - int count = (*ea_inode_array)->count; >> + unsigned int count = (*ea_inode_array)->count; > > It seems `count` is not actually needed anymore. > > If you remove it and directly use `(*ea_inode_array)->count` in the following > call to `kmalloc()`, you could use `struct_size()` in the call to `memcpy()` > below, and copy the whole thing in one line. See below. > >> - /* if new_array is NULL, this is essentially offsetof() */ >> - new_array = kmalloc( >> - offsetof(struct ext4_xattr_inode_array, >> - inodes[count + EIA_INCR]), >> - GFP_NOFS); >> + new_array = kmalloc(struct_size(*ea_inode_array, inodes, >> + count + EIA_INCR), >> + GFP_NOFS); >> if (new_array == NULL) >> return -ENOMEM; >> - memcpy(new_array, *ea_inode_array, >> - offsetof(struct ext4_xattr_inode_array, inodes[count])); >> + new_array->count = count; >> + memcpy(new_array->inodes, (*ea_inode_array)->inodes, >> + count * sizeof(struct inode *)); > > memcpy(new_array, *ea_inode_array, struct_size(new_array, inodes, (*ea_inode_array)->count)); > >> kfree(*ea_inode_array); >> *ea_inode_array = new_array; >> } > > Also, you are missing one more like just below this one, where `(*ea_inode_array)->count` > is currently being used to directly index `inodes`: > > (*ea_inode_array)->inodes[(*ea_inode_array)->count++] = inode; Thanks, I missed this one. Thorsten
