On Tue, Jun 26, 2018 at 4:39 PM Ilya Dryomov <idryomov@xxxxxxxxx> 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.
>
Agree.
Thanks
Yan, Zheng
> 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.
>
> 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
