Messages in this thread | | | Date | Thu, 28 Mar 2024 23:17:33 +0000 | From | Justin Stitt <> | Subject | Re: [PATCH 04/11] orangefs: convert strncpy() to strscpy() |
| |
Hi,
On Thu, Mar 28, 2024 at 03:04:48PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc warns about a truncated string copy with a 255 byte string getting > copied to a buffer of the same length, losing the 0-termination: > > In function 'orangefs_unmount', > inlined from 'orangefs_kill_sb' at arm-soc/fs/orangefs/super.c:619:6: > fs/orangefs/super.c:406:9: error: 'strncpy' output may be truncated copying 255 bytes from a string of length 255 [-Werror=stringop-truncation] > 406 | strncpy(op->upcall.req.fs_umount.orangefs_config_server, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 407 | devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I see that most string copies in orangefs are for the upcalls and use > a buffer that is one short to get the implied termination from the > zero-filled buffer, but some other instances lack the '-1' part. > > Convert from strncpy() to strscpy() to avoids both the warning about > the buffer size and the need for the explicit padding, since strscpy > guarantees a zero-terminated buffer. >
I think I got most of these with my patch sent earlier last week:
https://lore.kernel.org/all/20240322-strncpy-fs-orangefs-dcache-c-v1-1-15d12debbf38@google.com/
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/orangefs/dcache.c | 4 ++-- > fs/orangefs/namei.c | 33 +++++++++++++++------------------ > fs/orangefs/super.c | 16 +++++++--------- > 3 files changed, 24 insertions(+), 29 deletions(-) > > diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c > index 8bbe9486e3a6..96ed9900f7a9 100644 > --- a/fs/orangefs/dcache.c > +++ b/fs/orangefs/dcache.c > @@ -33,9 +33,9 @@ static int orangefs_revalidate_lookup(struct dentry *dentry) > > new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW; > new_op->upcall.req.lookup.parent_refn = parent->refn; > - strncpy(new_op->upcall.req.lookup.d_name, > + strscpy(new_op->upcall.req.lookup.d_name, > dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + ORANGEFS_NAME_MAX); > > gossip_debug(GOSSIP_DCACHE_DEBUG, > "%s:%s:%d interrupt flag [%d]\n", > diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c > index c9dfd5c6a097..5e46d3bdcb05 100644 > --- a/fs/orangefs/namei.c > +++ b/fs/orangefs/namei.c > @@ -41,8 +41,8 @@ static int orangefs_create(struct mnt_idmap *idmap, > fill_default_sys_attrs(new_op->upcall.req.create.attributes, > ORANGEFS_TYPE_METAFILE, mode); > > - strncpy(new_op->upcall.req.create.d_name, > - dentry->d_name.name, ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.create.d_name, > + dentry->d_name.name, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -137,8 +137,8 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry, > &parent->refn.khandle); > new_op->upcall.req.lookup.parent_refn = parent->refn; > > - strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, > + ORANGEFS_NAME_MAX); > > gossip_debug(GOSSIP_NAME_DEBUG, > "%s: doing lookup on %s under %pU,%d\n", > @@ -192,8 +192,8 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry) > return -ENOMEM; > > new_op->upcall.req.remove.parent_refn = parent->refn; > - strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name, > + ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, "orangefs_unlink", > get_interruptible_flag(inode)); > @@ -247,10 +247,9 @@ static int orangefs_symlink(struct mnt_idmap *idmap, > ORANGEFS_TYPE_SYMLINK, > mode); > > - strncpy(new_op->upcall.req.sym.entry_name, > - dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.sym.entry_name, > + dentry->d_name.name, ORANGEFS_NAME_MAX); > + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -324,8 +323,8 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir, > fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes, > ORANGEFS_TYPE_DIRECTORY, mode); > > - strncpy(new_op->upcall.req.mkdir.d_name, > - dentry->d_name.name, ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.mkdir.d_name, > + dentry->d_name.name, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -405,12 +404,10 @@ static int orangefs_rename(struct mnt_idmap *idmap, > new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn; > new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn; > > - strncpy(new_op->upcall.req.rename.d_old_name, > - old_dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > - strncpy(new_op->upcall.req.rename.d_new_name, > - new_dentry->d_name.name, > - ORANGEFS_NAME_MAX - 1); > + strscpy(new_op->upcall.req.rename.d_old_name, > + old_dentry->d_name.name, ORANGEFS_NAME_MAX); > + strscpy(new_op->upcall.req.rename.d_new_name, > + new_dentry->d_name.name, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, > "orangefs_rename", > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > index d990f4356b30..c714380ab38b 100644 > --- a/fs/orangefs/super.c > +++ b/fs/orangefs/super.c > @@ -256,7 +256,7 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb) > new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT); > if (!new_op) > return -ENOMEM; > - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server, > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > orangefs_sb->devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); > > @@ -403,8 +403,8 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname) > return -ENOMEM; > op->upcall.req.fs_umount.id = id; > op->upcall.req.fs_umount.fs_id = fs_id; > - strncpy(op->upcall.req.fs_umount.orangefs_config_server, > - devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > + strscpy(op->upcall.req.fs_umount.orangefs_config_server, > + devname, ORANGEFS_MAX_SERVER_ADDR_LEN); > r = service_operation(op, "orangefs_fs_umount", 0); > /* Not much to do about an error here. */ > if (r) > @@ -497,9 +497,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > if (!new_op) > return ERR_PTR(-ENOMEM); > > - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server, > - devname, > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server, > + devname, ORANGEFS_MAX_SERVER_ADDR_LEN); > > gossip_debug(GOSSIP_SUPER_DEBUG, > "Attempting ORANGEFS Mount via host %s\n", > @@ -546,9 +545,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > * on successful mount, store the devname and data > * used > */ > - strncpy(ORANGEFS_SB(sb)->devname, > - devname, > - ORANGEFS_MAX_SERVER_ADDR_LEN - 1); > + strscpy(ORANGEFS_SB(sb)->devname, devname, > + ORANGEFS_MAX_SERVER_ADDR_LEN); > > /* mount_pending must be cleared */ > ORANGEFS_SB(sb)->mount_pending = 0; > -- > 2.39.2 > Thanks Justin
| |