On Wed, Sep 11, 2019 at 6:42 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> Most of these values should never be negative, so convert them to
> unsigned values. Leave caps_max alone however, as it will be compared
> with a counter that we want to leave signed.
>
> Add some sanity checking to the parsed values, and clean up some
> unneeded casts.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/ceph/mds_client.c | 5 +++--
> fs/ceph/super.c | 34 ++++++++++++++++++----------------
> fs/ceph/super.h | 16 ++++++++--------
> 3 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 0d7afabb1f49..da882217d04d 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2031,12 +2031,13 @@ int ceph_alloc_readdir_reply_buffer(struct ceph_mds_request *req,
> struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info;
> struct ceph_mount_options *opt = req->r_mdsc->fsc->mount_options;
> size_t size = sizeof(struct ceph_mds_reply_dir_entry);
> - int order, num_entries;
> + unsigned int num_entries;
> + int order;
>
> spin_lock(&ci->i_ceph_lock);
> num_entries = ci->i_files + ci->i_subdirs;
> spin_unlock(&ci->i_ceph_lock);
> - num_entries = max(num_entries, 1);
> + num_entries = max(num_entries, 1U);
> num_entries = min(num_entries, opt->max_readdir);
>
> order = get_order(size * num_entries);
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 2710f9a4a372..fa95c2faf167 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -170,10 +170,10 @@ static const struct fs_parameter_enum ceph_param_enums[] = {
> static const struct fs_parameter_spec ceph_param_specs[] = {
> fsparam_flag_no ("acl", Opt_acl),
> fsparam_flag_no ("asyncreaddir", Opt_asyncreaddir),
> - fsparam_u32 ("caps_max", Opt_caps_max),
> + fsparam_s32 ("caps_max", Opt_caps_max),
> fsparam_u32 ("caps_wanted_delay_max", Opt_caps_wanted_delay_max),
> fsparam_u32 ("caps_wanted_delay_min", Opt_caps_wanted_delay_min),
> - fsparam_s32 ("write_congestion_kb", Opt_congestion_kb),
> + fsparam_u32 ("write_congestion_kb", Opt_congestion_kb),
> fsparam_flag_no ("copyfrom", Opt_copyfrom),
> fsparam_flag_no ("dcache", Opt_dcache),
> fsparam_flag_no ("dirstat", Opt_dirstat),
> @@ -185,8 +185,8 @@ static const struct fs_parameter_spec ceph_param_specs[] = {
> fsparam_flag_no ("quotadf", Opt_quotadf),
> fsparam_u32 ("rasize", Opt_rasize),
> fsparam_flag_no ("rbytes", Opt_rbytes),
> - fsparam_s32 ("readdir_max_bytes", Opt_readdir_max_bytes),
> - fsparam_s32 ("readdir_max_entries", Opt_readdir_max_entries),
> + fsparam_u32 ("readdir_max_bytes", Opt_readdir_max_bytes),
> + fsparam_u32 ("readdir_max_entries", Opt_readdir_max_entries),
>
> [...]
>
> case Opt_caps_max:
> - fsopt->caps_max = result.uint_32;
> + if (result.int_32 < 0)
> + goto invalid_value;
> + fsopt->caps_max = result.int_32;
I picked on David's patch because it converted everything to unsigned,
but left write_congestion_kb, readdir_max_bytes and readdir_max_entries
signed. None of them can be negative, so it didn't make sense to me.
This patch fixes that, but leaves caps_max signed. caps_max can't be
negative, so again it doesn't make sense to me. If we are going to
tweak the option table as part of this conversion, I think it needs to
be consistent: either signed with a check or unsigned without a check
for all options that can't be negative.
Thanks,
Ilya
