On Tue, Jun 26, 2018 at 11:04 AM cgxu519 <cgxu519@xxxxxxx> wrote:
>
>
> On 06/26/2018 04:39 PM, Ilya Dryomov wrote:
> > On Tue, Jun 26, 2018 at 10:17 AM Yan, Zheng <ukernel@xxxxxxxxx> wrote:
> >> On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@xxxxxxx> wrote:
> >>>
> >>> On 06/26/2018 02:32 PM, Yan, Zheng wrote:
> >>>>> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@xxxxxxx> wrote:
> >>>>>
> >>>>> Hi Ilya,
> >>>>>
> >>>>> Any objection for this? Maybe I should fix the overlapping return code as well.
> >>>>> Should I split the patch to libceph and ceph part?
> >>>>>
> >>>>> On 06/24/2018 09:06 PM, Chengguang Xu wrote:
> >>>>>> Using ceph_pagelist_encode_string() instead of combination of
> >>>>>> ceph_pagelist_encode_32() and ceph_pagelist_append() when encoding
> >>>>>> string. Meanwhile add return code check for ceph_pagelist_encode_string().
> >>>>>>
> >>>>>> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
> >>>>>> ---
> >>>>>> v2:
> >>>>>> - Add return code check for ceph_pagelist_encode_string().
> >>>>>>
> >>>>>> fs/ceph/acl.c | 18 ++++++++++++------
> >>>>>> net/ceph/osd_client.c | 16 +++++++++-------
> >>>>>> 2 files changed, 21 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> >>>>>> index 5da752751a2b..6f0210984456 100644
> >>>>>> --- a/fs/ceph/acl.c
> >>>>>> +++ b/fs/ceph/acl.c
> >>>>>> @@ -222,14 +222,17 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode,
> >>>>>> err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8);
> >>>>>> if (err)
> >>>>>> goto out_err;
> >>>>>> - ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS,
> >>>>>> - len);
> >>>>>> + err = ceph_pagelist_encode_string(pagelist,
> >>>>>> + XATTR_NAME_POSIX_ACL_ACCESS, len);
> >>>>>> + if (err)
> >>>>>> + goto out_err;
> >>>>>> err = posix_acl_to_xattr(&init_user_ns, acl,
> >>>>>> tmp_buf, val_size1);
> >>>>>> if (err < 0)
> >>>>>> goto out_err;
> >>>>>> - ceph_pagelist_encode_32(pagelist, val_size1);
> >>>>>> - ceph_pagelist_append(pagelist, tmp_buf, val_size1);
> >>>>>> + err = ceph_pagelist_encode_string(pagelist, tmp_buf, val_size1);
> >>>>>> + if (err)
> >>>>>> + goto out_err;
> >>>>>> }
> >>>> the code first reserve space, then add data to page list. no need to check return
> >>> Hi Yan,
> >>>
> >>> Thanks for your reply, after applying below patch
> >>> ceph_pagelist_encode_string()
> >>> may return error(maybe very rare case?) even we have reserved space
> >>> before calling it.
> >>>
> >>> [PATCH v2 1/2] ceph: check string length in
> >>> ceph_pagelist_encode_string() for safety
> >>>
> >> should make ceph_pagelist_reserve() do the same check. If reservation
> >> succeeds, ceph_pagelist_encode_string() should never fail.
> > If we add it to reserve(), what about append(), etc?
> >
> > I applied that patch yesterday, but on a second thought, I think we
> > should revert it and change ceph_pagelist_encode_string() to take u32,
> > thus pushing the responsibility for that check into the callers.
> >
> > I have already changed ceph_osdc_notify{,ack}() which took size_t to
> > u32 (see testing). Here size_t is mandated, but ceph_pre_init_acls()
> > can do that check before calling ceph_pagelist_reserve().
> >
> > No need to patch pagelist.c.
>
> How about introduce a helper ceph_pagelist_encode_size_t() to deal with
> type size_t.
> It seems a bit easier than checking in the caller.
TBH I'm not sure ceph_pre_init_acls() needs that check at all.
If we want to be defensive, I think
if (val_size1 > U32_MAX || val_size2 > U32_MAX)
...
is just fine.
Thanks,
Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
