From: Martin Brandenburg <[email protected]>
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 <[email protected]>
---
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
From: Martin Brandenburg <[email protected]>
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 <[email protected]>
---
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
From: Martin Brandenburg <[email protected]>
---
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
On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <[email protected]> wrote:
> From: Martin Brandenburg <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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
<[email protected]> wrote:
> On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <[email protected]> wrote:
>> From: Martin Brandenburg <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
> On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <[email protected]> wrote:
> >> From: Martin Brandenburg <[email protected]>
> >>
> >> 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
On Fri, Apr 8, 2016 at 6:34 PM, <[email protected]> wrote:
>> On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko
>> <[email protected]> wrote:
>> > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <[email protected]> wrote:
>> >> From: Martin Brandenburg <[email protected]>
>> >>
>> >> 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
On Fri, 8 Apr 2016, Andy Shevchenko wrote:
> On Fri, Apr 8, 2016 at 6:34 PM, <[email protected]> wrote:
> >> On Thu, Apr 7, 2016 at 2:39 PM, Andy Shevchenko
> >> <[email protected]> wrote:
> >> > On Mon, Apr 4, 2016 at 11:26 PM, Martin Brandenburg <[email protected]> wrote:
> >> >> From: Martin Brandenburg <[email protected]>
> >> >>
> >> >> 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
>
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 <[email protected]>
---
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