* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.