[PATCH 1/3] orangefs: clean up truncate ctime and mtime setting

[PATCH 1/3] orangefs: clean up truncate ctime and mtime setting

[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

[PATCH 2/3] orangefs: strncpy -> strlcpy

[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

[PATCH 3/3] orangefs: remove unused variable

[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

Re: [PATCH 2/3] orangefs: strncpy -> strlcpy

[Andy Shevchenko]

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

Re: [PATCH 2/3] orangefs: strncpy -> strlcpy

[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

Re: [PATCH 2/3] orangefs: strncpy -> strlcpy

[martin]

> 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

Re: [PATCH 2/3] orangefs: strncpy -> strlcpy

[Andy Shevchenko]

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

Re: [PATCH 2/3] orangefs: strncpy -> strlcpy

[martin]

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
> 

[PATCH] orangefs: strncpy -> strscpy

[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