On mon, 17 Dec 2012 18:34:41 +0100, Goffredo Baroncelli wrote:
> On 12/17/2012 02:30 PM, Jeff Liu wrote:
>> On 12/17/2012 07:57 PM, Miao Xie wrote:
>>> On mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>>
>>>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>>>> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
>>>> Cc: Miao Xie <miaox@xxxxxxxxxxxxxx>
> [...]
>>>> +
>>>> + if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>>> + return -EINVAL;
>>>
>>> I think we should use strnlen()
>> AFAICS, strnlen() is better only if the caller need to get the length of
>> a length-limited string and make use of it proceeding, which means that
>> the procedure would not return an error even if the length is beyond the
>> limit. Or if the caller need to examine if a length-limited string is
>> nul-terminated or not in a manner below,
>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
>> ....
>> }
>>
>> I don't think it really needed here since the logic is clear with
>> strlen(), or Am I miss anything?
>
> I think that Miao fears strlen() searching a zero could go beyond the
> page limit touching an un-mapped page and raising an segmentation fault....
Yes, so I think the following check is better.
if (strnlen(buf, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
return -EINVAL;
Thanks
Miao
> I think that we should change the code as
>
> + label[BTRFS_LABEL_SIZE - 1] = 0;
> +
> + if (strlen(label) > BTRFS_LABEL_SIZE - 1)
> + return -EINVAL;
>
> My 2¢
>
> Ciao
> G.Baroncelli
>>
>>
>> Thanks,
>> -Jeff
>>
>>> Thanks
>>> Miao
>>>
>>>> +
>>>> + ret = mnt_want_write_file(file);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + mutex_lock(&root->fs_info->volume_mutex);
>>>> + trans = btrfs_start_transaction(root, 1);
>>>> + if (IS_ERR(trans)) {
>>>> + ret = PTR_ERR(trans);
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + label[BTRFS_LABEL_SIZE - 1] = '\0';
>>>> + strcpy(super_block->label, label);
>>>> + btrfs_end_transaction(trans, root);
>>>> +
>>>> +out_unlock:
>>>> + mutex_unlock(&root->fs_info->volume_mutex);
>>>> + mnt_drop_write_file(file);
>>>> + return ret;
>>>> +}
>>>> +
>>>> long btrfs_ioctl(struct file *file, unsigned int
>>>> cmd, unsigned long arg)
>>>> {
>>>> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>>> return btrfs_ioctl_qgroup_limit(root, argp);
>>>> case BTRFS_IOC_GET_FSLABEL:
>>>> return btrfs_ioctl_get_fslabel(file, argp);
>>>> + case BTRFS_IOC_SET_FSLABEL:
>>>> + return btrfs_ioctl_set_fslabel(file, argp);
>>>> }
>>>>
>>>> return -ENOTTY;
>>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>>> index 5b2cbef..2abe239 100644
>>>> --- a/fs/btrfs/ioctl.h
>>>> +++ b/fs/btrfs/ioctl.h
>>>> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>>>> struct btrfs_ioctl_qgroup_limit_args)
>>>> #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>>>> char[BTRFS_LABEL_SIZE])
>>>> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>>>> + char[BTRFS_LABEL_SIZE])
>>>> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>>>> struct btrfs_ioctl_get_dev_stats)
>>>> #endif
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
