* [PATCH 1/3] orangefs: clean up truncate ctime and mtime setting @ 2016-04-04 20:26 Martin Brandenburg 2016-04-04 20:26 ` [PATCH 2/3] orangefs: strncpy -> strlcpy Martin Brandenburg 2016-04-04 20:26 ` [PATCH 3/3] orangefs: remove unused variable Martin Brandenburg 0 siblings, 2 replies; 9+ messages in thread From: Martin Brandenburg @ 2016-04-04 20:26 UTC (permalink / raw) To: hubcap; +Cc: linux-kernel, linux-fsdevel, Martin Brandenburg From: Martin Brandenburg <martin@omnibond.com> The ctime and mtime are always updated on a successful ftruncate and only updated on a successful truncate where the size changed. We handle the ``if the size changed'' bit. This matches FUSE's behavior. Signed-off-by: Martin Brandenburg <martin@omnibond.com> --- fs/orangefs/inode.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c index 2382e26..975a796 100644 --- a/fs/orangefs/inode.c +++ b/fs/orangefs/inode.c @@ -204,22 +204,8 @@ static int orangefs_setattr_size(struct inode *inode, struct iattr *iattr) if (ret != 0) return ret; - /* - * Only change the c/mtime if we are changing the size or we are - * explicitly asked to change it. This handles the semantic difference - * between truncate() and ftruncate() as implemented in the VFS. - * - * The regular truncate() case without ATTR_CTIME and ATTR_MTIME is a - * special case where we need to update the times despite not having - * these flags set. For all other operations the VFS set these flags - * explicitly if it wants a timestamp update. - */ - if (orig_size != i_size_read(inode) && - !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) { - iattr->ia_ctime = iattr->ia_mtime = - current_fs_time(inode->i_sb); + if (orig_size != i_size_read(inode)) iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME; - } return ret; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-04 20:26 [PATCH 1/3] orangefs: clean up truncate ctime and mtime setting Martin Brandenburg @ 2016-04-04 20:26 ` Martin Brandenburg 2016-04-07 18:39 ` Andy Shevchenko 2016-04-04 20:26 ` [PATCH 3/3] orangefs: remove unused variable Martin Brandenburg 1 sibling, 1 reply; 9+ messages in thread From: Martin Brandenburg @ 2016-04-04 20:26 UTC (permalink / raw) To: hubcap; +Cc: linux-kernel, linux-fsdevel, Martin Brandenburg From: Martin Brandenburg <martin@omnibond.com> Almost everywhere we use strncpy we should use strlcpy. This affects path names (d_name mostly), symlink targets, and server names. Leave debugfs code as is for now, though it could use a review as well. Signed-off-by: Martin Brandenburg <martin@omnibond.com> --- fs/orangefs/dcache.c | 2 +- fs/orangefs/namei.c | 16 ++++++++-------- fs/orangefs/orangefs-utils.c | 2 +- fs/orangefs/super.c | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c index 5dfc4f3..0710869 100644 --- a/fs/orangefs/dcache.c +++ b/fs/orangefs/dcache.c @@ -30,7 +30,7 @@ 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, + strlcpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, ORANGEFS_NAME_MAX); diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c index 5a60c50..fc7e948 100644 --- a/fs/orangefs/namei.c +++ b/fs/orangefs/namei.c @@ -37,7 +37,7 @@ static int orangefs_create(struct inode *dir, fill_default_sys_attrs(new_op->upcall.req.create.attributes, ORANGEFS_TYPE_METAFILE, mode); - strncpy(new_op->upcall.req.create.d_name, + strlcpy(new_op->upcall.req.create.d_name, dentry->d_name.name, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -132,7 +132,7 @@ 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, + strlcpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, ORANGEFS_NAME_MAX); gossip_debug(GOSSIP_NAME_DEBUG, @@ -231,7 +231,7 @@ 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, + strlcpy(new_op->upcall.req.remove.d_name, dentry->d_name.name, ORANGEFS_NAME_MAX); ret = service_operation(new_op, "orangefs_unlink", @@ -282,10 +282,10 @@ static int orangefs_symlink(struct inode *dir, ORANGEFS_TYPE_SYMLINK, mode); - strncpy(new_op->upcall.req.sym.entry_name, + strlcpy(new_op->upcall.req.sym.entry_name, dentry->d_name.name, ORANGEFS_NAME_MAX); - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); + strlcpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -347,7 +347,7 @@ static int orangefs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes, ORANGEFS_TYPE_DIRECTORY, mode); - strncpy(new_op->upcall.req.mkdir.d_name, + strlcpy(new_op->upcall.req.mkdir.d_name, dentry->d_name.name, ORANGEFS_NAME_MAX); ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); @@ -419,10 +419,10 @@ static int orangefs_rename(struct inode *old_dir, 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, + strlcpy(new_op->upcall.req.rename.d_old_name, old_dentry->d_name.name, ORANGEFS_NAME_MAX); - strncpy(new_op->upcall.req.rename.d_new_name, + strlcpy(new_op->upcall.req.rename.d_new_name, new_dentry->d_name.name, ORANGEFS_NAME_MAX); diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c index 40f5163..d72f3fc 100644 --- a/fs/orangefs/orangefs-utils.c +++ b/fs/orangefs/orangefs-utils.c @@ -505,7 +505,7 @@ int orangefs_unmount_sb(struct super_block *sb) return -ENOMEM; new_op->upcall.req.fs_umount.id = ORANGEFS_SB(sb)->id; new_op->upcall.req.fs_umount.fs_id = ORANGEFS_SB(sb)->fs_id; - strncpy(new_op->upcall.req.fs_umount.orangefs_config_server, + strlcpy(new_op->upcall.req.fs_umount.orangefs_config_server, ORANGEFS_SB(sb)->devname, ORANGEFS_MAX_SERVER_ADDR_LEN); diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c index b9da9a0..5f9a4ff 100644 --- a/fs/orangefs/super.c +++ b/fs/orangefs/super.c @@ -220,7 +220,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, + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, orangefs_sb->devname, ORANGEFS_MAX_SERVER_ADDR_LEN); @@ -434,7 +434,7 @@ 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, + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, devname, ORANGEFS_MAX_SERVER_ADDR_LEN); @@ -474,7 +474,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst, * on successful mount, store the devname and data * used */ - strncpy(ORANGEFS_SB(sb)->devname, + strlcpy(ORANGEFS_SB(sb)->devname, devname, ORANGEFS_MAX_SERVER_ADDR_LEN); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-04 20:26 ` [PATCH 2/3] orangefs: strncpy -> strlcpy Martin Brandenburg @ 2016-04-07 18:39 ` Andy Shevchenko 2016-04-07 19:43 ` Mike Marshall 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2016-04-07 18:39 UTC (permalink / raw) To: Martin Brandenburg; +Cc: hubcap, linux-kernel, Linux FS Devel On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <martin@omnibond.com> wrote: > From: Martin Brandenburg <martin@omnibond.com> > > Almost everywhere we use strncpy we should use strlcpy. This affects > path names (d_name mostly), symlink targets, and server names. > > Leave debugfs code as is for now, though it could use a review as well. > |Why not strscpy() as most robust one? > Signed-off-by: Martin Brandenburg <martin@omnibond.com> > --- > fs/orangefs/dcache.c | 2 +- > fs/orangefs/namei.c | 16 ++++++++-------- > fs/orangefs/orangefs-utils.c | 2 +- > fs/orangefs/super.c | 6 +++--- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c > index 5dfc4f3..0710869 100644 > --- a/fs/orangefs/dcache.c > +++ b/fs/orangefs/dcache.c > @@ -30,7 +30,7 @@ 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, > + strlcpy(new_op->upcall.req.lookup.d_name, > dentry->d_name.name, > ORANGEFS_NAME_MAX); > > diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c > index 5a60c50..fc7e948 100644 > --- a/fs/orangefs/namei.c > +++ b/fs/orangefs/namei.c > @@ -37,7 +37,7 @@ static int orangefs_create(struct inode *dir, > fill_default_sys_attrs(new_op->upcall.req.create.attributes, > ORANGEFS_TYPE_METAFILE, mode); > > - strncpy(new_op->upcall.req.create.d_name, > + strlcpy(new_op->upcall.req.create.d_name, > dentry->d_name.name, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > @@ -132,7 +132,7 @@ 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, > + strlcpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, > ORANGEFS_NAME_MAX); > > gossip_debug(GOSSIP_NAME_DEBUG, > @@ -231,7 +231,7 @@ 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, > + strlcpy(new_op->upcall.req.remove.d_name, dentry->d_name.name, > ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, "orangefs_unlink", > @@ -282,10 +282,10 @@ static int orangefs_symlink(struct inode *dir, > ORANGEFS_TYPE_SYMLINK, > mode); > > - strncpy(new_op->upcall.req.sym.entry_name, > + strlcpy(new_op->upcall.req.sym.entry_name, > dentry->d_name.name, > ORANGEFS_NAME_MAX); > - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > + strlcpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > > @@ -347,7 +347,7 @@ static int orangefs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode > fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes, > ORANGEFS_TYPE_DIRECTORY, mode); > > - strncpy(new_op->upcall.req.mkdir.d_name, > + strlcpy(new_op->upcall.req.mkdir.d_name, > dentry->d_name.name, ORANGEFS_NAME_MAX); > > ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); > @@ -419,10 +419,10 @@ static int orangefs_rename(struct inode *old_dir, > 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, > + strlcpy(new_op->upcall.req.rename.d_old_name, > old_dentry->d_name.name, > ORANGEFS_NAME_MAX); > - strncpy(new_op->upcall.req.rename.d_new_name, > + strlcpy(new_op->upcall.req.rename.d_new_name, > new_dentry->d_name.name, > ORANGEFS_NAME_MAX); > > diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c > index 40f5163..d72f3fc 100644 > --- a/fs/orangefs/orangefs-utils.c > +++ b/fs/orangefs/orangefs-utils.c > @@ -505,7 +505,7 @@ int orangefs_unmount_sb(struct super_block *sb) > return -ENOMEM; > new_op->upcall.req.fs_umount.id = ORANGEFS_SB(sb)->id; > new_op->upcall.req.fs_umount.fs_id = ORANGEFS_SB(sb)->fs_id; > - strncpy(new_op->upcall.req.fs_umount.orangefs_config_server, > + strlcpy(new_op->upcall.req.fs_umount.orangefs_config_server, > ORANGEFS_SB(sb)->devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); > > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > index b9da9a0..5f9a4ff 100644 > --- a/fs/orangefs/super.c > +++ b/fs/orangefs/super.c > @@ -220,7 +220,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, > + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, > orangefs_sb->devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); > > @@ -434,7 +434,7 @@ 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, > + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, > devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); > > @@ -474,7 +474,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst, > * on successful mount, store the devname and data > * used > */ > - strncpy(ORANGEFS_SB(sb)->devname, > + strlcpy(ORANGEFS_SB(sb)->devname, > devname, > ORANGEFS_MAX_SERVER_ADDR_LEN); > > -- > 2.7.4 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-07 18:39 ` Andy Shevchenko @ 2016-04-07 19:43 ` Mike Marshall 2016-04-08 15:34 ` martin 0 siblings, 1 reply; 9+ messages in thread From: Mike Marshall @ 2016-04-07 19:43 UTC (permalink / raw) To: Andy Shevchenko Cc: Martin Brandenburg, linux-kernel, Linux FS Devel, Mike Marshall It looks like strscpy went in last October... there are no users of it yet. I was just about to send in a pull request that includes Martin's strncpy->strlcpy patch when I saw Andy's comment. Linus said when he pulled strscpy: > So I'm pulling the strscpy() support because it *is* a better interface. > But I will refuse to pull mindless conversion patches. Use this in > places where it makes sense, but don't do trivial patches to fix things > that aren't actually known to be broken. Maybe it makes sense for our strncpy->strlcpy patch to be a strscpy patch instead? Maybe our strncpy->strlcpy patch is itself a "mindless conversion patch"? (I don't think so)... I'll wait until tomorrow, and then send my pull request as it is, unless everyone chimes in and says "use strscpy!"... -Mike On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <martin@omnibond.com> wrote: >> From: Martin Brandenburg <martin@omnibond.com> >> >> Almost everywhere we use strncpy we should use strlcpy. This affects >> path names (d_name mostly), symlink targets, and server names. >> >> Leave debugfs code as is for now, though it could use a review as well. >> > > |Why not strscpy() as most robust one? > >> Signed-off-by: Martin Brandenburg <martin@omnibond.com> >> --- >> fs/orangefs/dcache.c | 2 +- >> fs/orangefs/namei.c | 16 ++++++++-------- >> fs/orangefs/orangefs-utils.c | 2 +- >> fs/orangefs/super.c | 6 +++--- >> 4 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c >> index 5dfc4f3..0710869 100644 >> --- a/fs/orangefs/dcache.c >> +++ b/fs/orangefs/dcache.c >> @@ -30,7 +30,7 @@ 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, >> + strlcpy(new_op->upcall.req.lookup.d_name, >> dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> >> diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c >> index 5a60c50..fc7e948 100644 >> --- a/fs/orangefs/namei.c >> +++ b/fs/orangefs/namei.c >> @@ -37,7 +37,7 @@ static int orangefs_create(struct inode *dir, >> fill_default_sys_attrs(new_op->upcall.req.create.attributes, >> ORANGEFS_TYPE_METAFILE, mode); >> >> - strncpy(new_op->upcall.req.create.d_name, >> + strlcpy(new_op->upcall.req.create.d_name, >> dentry->d_name.name, ORANGEFS_NAME_MAX); >> >> ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); >> @@ -132,7 +132,7 @@ 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, >> + strlcpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> >> gossip_debug(GOSSIP_NAME_DEBUG, >> @@ -231,7 +231,7 @@ 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, >> + strlcpy(new_op->upcall.req.remove.d_name, dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> >> ret = service_operation(new_op, "orangefs_unlink", >> @@ -282,10 +282,10 @@ static int orangefs_symlink(struct inode *dir, >> ORANGEFS_TYPE_SYMLINK, >> mode); >> >> - strncpy(new_op->upcall.req.sym.entry_name, >> + strlcpy(new_op->upcall.req.sym.entry_name, >> dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); >> + strlcpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX); >> >> ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); >> >> @@ -347,7 +347,7 @@ static int orangefs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode >> fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes, >> ORANGEFS_TYPE_DIRECTORY, mode); >> >> - strncpy(new_op->upcall.req.mkdir.d_name, >> + strlcpy(new_op->upcall.req.mkdir.d_name, >> dentry->d_name.name, ORANGEFS_NAME_MAX); >> >> ret = service_operation(new_op, __func__, get_interruptible_flag(dir)); >> @@ -419,10 +419,10 @@ static int orangefs_rename(struct inode *old_dir, >> 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, >> + strlcpy(new_op->upcall.req.rename.d_old_name, >> old_dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> - strncpy(new_op->upcall.req.rename.d_new_name, >> + strlcpy(new_op->upcall.req.rename.d_new_name, >> new_dentry->d_name.name, >> ORANGEFS_NAME_MAX); >> >> diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c >> index 40f5163..d72f3fc 100644 >> --- a/fs/orangefs/orangefs-utils.c >> +++ b/fs/orangefs/orangefs-utils.c >> @@ -505,7 +505,7 @@ int orangefs_unmount_sb(struct super_block *sb) >> return -ENOMEM; >> new_op->upcall.req.fs_umount.id = ORANGEFS_SB(sb)->id; >> new_op->upcall.req.fs_umount.fs_id = ORANGEFS_SB(sb)->fs_id; >> - strncpy(new_op->upcall.req.fs_umount.orangefs_config_server, >> + strlcpy(new_op->upcall.req.fs_umount.orangefs_config_server, >> ORANGEFS_SB(sb)->devname, >> ORANGEFS_MAX_SERVER_ADDR_LEN); >> >> diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c >> index b9da9a0..5f9a4ff 100644 >> --- a/fs/orangefs/super.c >> +++ b/fs/orangefs/super.c >> @@ -220,7 +220,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, >> + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, >> orangefs_sb->devname, >> ORANGEFS_MAX_SERVER_ADDR_LEN); >> >> @@ -434,7 +434,7 @@ 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, >> + strlcpy(new_op->upcall.req.fs_mount.orangefs_config_server, >> devname, >> ORANGEFS_MAX_SERVER_ADDR_LEN); >> >> @@ -474,7 +474,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst, >> * on successful mount, store the devname and data >> * used >> */ >> - strncpy(ORANGEFS_SB(sb)->devname, >> + strlcpy(ORANGEFS_SB(sb)->devname, >> devname, >> ORANGEFS_MAX_SERVER_ADDR_LEN); >> >> -- >> 2.7.4 >> > > > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-07 19:43 ` Mike Marshall @ 2016-04-08 15:34 ` martin 2016-04-08 16:02 ` Andy Shevchenko 0 siblings, 1 reply; 9+ messages in thread From: martin @ 2016-04-08 15:34 UTC (permalink / raw) To: Mike Marshall; +Cc: Andy Shevchenko, linux-kernel, Linux FS Devel > On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <martin@omnibond.com> wrote: > >> From: Martin Brandenburg <martin@omnibond.com> > >> > >> Almost everywhere we use strncpy we should use strlcpy. This affects > >> path names (d_name mostly), symlink targets, and server names. > >> > >> Leave debugfs code as is for now, though it could use a review as well. > >> > > > > |Why not strscpy() as most robust one? Mostly because I hadn't heard about strscpy. On Thu, 7 Apr 2016, Mike Marshall wrote: > It looks like strscpy went in last October... there are > no users of it yet. I was just about to send in a pull request > that includes Martin's strncpy->strlcpy patch when I saw > Andy's comment. > > Linus said when he pulled strscpy: > > > So I'm pulling the strscpy() support because it *is* a better interface. > > But I will refuse to pull mindless conversion patches. Use this in > > places where it makes sense, but don't do trivial patches to fix things > > that aren't actually known to be broken. > > Maybe it makes sense for our strncpy->strlcpy patch to be a strscpy > patch instead? Maybe our strncpy->strlcpy patch is itself a > "mindless conversion patch"? (I don't think so)... There is something broken! If the client-core sends in a string with no NUL terminator, we would blindly copy it into the d_name with strncpy. > > I'll wait until tomorrow, and then send my pull request as it is, unless > everyone chimes in and says "use strscpy!"... > > -Mike After looking over strscpy I don't see a compelling reason not to go ahead and use it while we're fixing up this code. -- Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-08 15:34 ` martin @ 2016-04-08 16:02 ` Andy Shevchenko 2016-04-08 17:16 ` martin 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2016-04-08 16:02 UTC (permalink / raw) To: Martin Brandenburg; +Cc: Mike Marshall, linux-kernel, Linux FS Devel On Fri, Apr 8, 2016 at 6:34 PM, <martin@omnibond.com> wrote: >> On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >> > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <martin@omnibond.com> wrote: >> >> From: Martin Brandenburg <martin@omnibond.com> >> >> >> >> Almost everywhere we use strncpy we should use strlcpy. This affects >> >> path names (d_name mostly), symlink targets, and server names. >> >> >> >> Leave debugfs code as is for now, though it could use a review as well. >> >> >> > >> > |Why not strscpy() as most robust one? > > Mostly because I hadn't heard about strscpy. It was nice story how he applied it to the tree. >> It looks like strscpy went in last October... there are >> no users of it yet. I was just about to send in a pull request >> that includes Martin's strncpy->strlcpy patch when I saw >> Andy's comment. >> >> Linus said when he pulled strscpy: > After looking over strscpy I don't see a compelling > reason not to go ahead and use it while we're fixing up > this code. I recommend to mention that this is a fix explicitly in the commit message, currently it sounds like a meaningless patch of trainee. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] orangefs: strncpy -> strlcpy 2016-04-08 16:02 ` Andy Shevchenko @ 2016-04-08 17:16 ` martin 2016-04-08 17:33 ` [PATCH] orangefs: strncpy -> strscpy Martin Brandenburg 0 siblings, 1 reply; 9+ messages in thread From: martin @ 2016-04-08 17:16 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Mike Marshall, linux-kernel, Linux FS Devel On Fri, 8 Apr 2016, Andy Shevchenko wrote: > On Fri, Apr 8, 2016 at 6:34 PM, <martin@omnibond.com> wrote: > >> On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko > >> <andy.shevchenko@gmail.com> wrote: > >> > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <martin@omnibond.com> wrote: > >> >> From: Martin Brandenburg <martin@omnibond.com> > >> >> > >> >> Almost everywhere we use strncpy we should use strlcpy. This affects > >> >> path names (d_name mostly), symlink targets, and server names. > >> >> > >> >> Leave debugfs code as is for now, though it could use a review as well. > >> >> > >> > > >> > |Why not strscpy() as most robust one? > > > > Mostly because I hadn't heard about strscpy. > > It was nice story how he applied it to the tree. Just read it.. > > >> It looks like strscpy went in last October... there are > >> no users of it yet. I was just about to send in a pull request > >> that includes Martin's strncpy->strlcpy patch when I saw > >> Andy's comment. > >> > >> Linus said when he pulled strscpy: > > > After looking over strscpy I don't see a compelling > > reason not to go ahead and use it while we're fixing up > > this code. > > I recommend to mention that this is a fix explicitly in the commit > message, currently it sounds like a meaningless patch of trainee. I've decided to scrap most of this, but one change is important. Most of it is a no-op because the client-core buffer is larger than NAME_MAX and there is always room. Replying with patch in a minute. Thanks for the review! Mike, I think we can delay this one for later so we can look at the debugfs and superblock code in more detail. -- Martin > > -- > With Best Regards, > Andy Shevchenko > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] orangefs: strncpy -> strscpy 2016-04-08 17:16 ` martin @ 2016-04-08 17:33 ` Martin Brandenburg 0 siblings, 0 replies; 9+ messages in thread From: Martin Brandenburg @ 2016-04-08 17:33 UTC (permalink / raw) To: andy.shevchenko, hubcap, linux-kernel, linux-fsdevel; +Cc: Martin Brandenburg It would have been possible for a rogue client-core to send in a symlink target which is not NUL terminated. This returns EIO if the client-core gives us corrupt data. Leave debugfs and superblock code as is for now. Other dcache.c and namei.c strncpy instances are safe because ORANGEFS_NAME_MAX = NAME_MAX + 1; there is always enough space for a name plus a NUL byte. Signed-off-by: Martin Brandenburg <martin@omnibond.com> --- fs/orangefs/orangefs-utils.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c index 40f5163..f392a6a 100644 --- a/fs/orangefs/orangefs-utils.c +++ b/fs/orangefs/orangefs-utils.c @@ -315,9 +315,13 @@ int orangefs_inode_getattr(struct inode *inode, int new, int size) inode->i_size = (loff_t)strlen(new_op-> downcall.resp.getattr.link_target); orangefs_inode->blksize = (1 << inode->i_blkbits); - strlcpy(orangefs_inode->link_target, + ret = strscpy(orangefs_inode->link_target, new_op->downcall.resp.getattr.link_target, ORANGEFS_NAME_MAX); + if (ret == -E2BIG) { + ret = -EIO; + goto out; + } inode->i_link = orangefs_inode->link_target; } break; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] orangefs: remove unused variable 2016-04-04 20:26 [PATCH 1/3] orangefs: clean up truncate ctime and mtime setting Martin Brandenburg 2016-04-04 20:26 ` [PATCH 2/3] orangefs: strncpy -> strlcpy Martin Brandenburg @ 2016-04-04 20:26 ` Martin Brandenburg 1 sibling, 0 replies; 9+ messages in thread From: Martin Brandenburg @ 2016-04-04 20:26 UTC (permalink / raw) To: hubcap; +Cc: linux-kernel, linux-fsdevel, Martin Brandenburg From: Martin Brandenburg <martin@omnibond.com> --- fs/orangefs/dir.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/orangefs/dir.c b/fs/orangefs/dir.c index ba7dec4..324f0af 100644 --- a/fs/orangefs/dir.c +++ b/fs/orangefs/dir.c @@ -153,7 +153,6 @@ static int orangefs_readdir(struct file *file, struct dir_context *ctx) struct dentry *dentry = file->f_path.dentry; struct orangefs_kernel_op_s *new_op = NULL; struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(dentry->d_inode); - int buffer_full = 0; struct orangefs_readdir_response_s readdir_response; void *dents_buf; int i = 0; @@ -350,8 +349,7 @@ get_new_buffer_index: /* * Did we hit the end of the directory? */ - if (readdir_response.token == ORANGEFS_READDIR_END && - !buffer_full) { + if (readdir_response.token == ORANGEFS_READDIR_END) { gossip_debug(GOSSIP_DIR_DEBUG, "End of dir detected; setting ctx->pos to ORANGEFS_READDIR_END.\n"); ctx->pos = ORANGEFS_READDIR_END; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-04-08 17:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-04 20:26 [PATCH 1/3] orangefs: clean up truncate ctime and mtime setting Martin Brandenburg 2016-04-04 20:26 ` [PATCH 2/3] orangefs: strncpy -> strlcpy Martin Brandenburg 2016-04-07 18:39 ` Andy Shevchenko 2016-04-07 19:43 ` Mike Marshall 2016-04-08 15:34 ` martin 2016-04-08 16:02 ` Andy Shevchenko 2016-04-08 17:16 ` martin 2016-04-08 17:33 ` [PATCH] orangefs: strncpy -> strscpy Martin Brandenburg 2016-04-04 20:26 ` [PATCH 3/3] orangefs: remove unused variable Martin Brandenburg
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).