| Message ID | 20180624130603.16782-2-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@gmx.com> > --- > 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; > } > if (default_acl) { > size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); > @@ -238,12 +241,15 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > goto out_err; > err = ceph_pagelist_encode_string(pagelist, > XATTR_NAME_POSIX_ACL_DEFAULT, len); > + if (err) > + goto out_err; > err = posix_acl_to_xattr(&init_user_ns, default_acl, > tmp_buf, val_size2); > if (err < 0) > goto out_err; > - ceph_pagelist_encode_32(pagelist, val_size2); > - ceph_pagelist_append(pagelist, tmp_buf, val_size2); > + err = ceph_pagelist_encode_string(pagelist, tmp_buf, val_size2); > + if (err) > + goto out_err; > } > > kfree(tmp_buf); > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index f2584fe1246f..720ac564d392 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -4607,14 +4607,15 @@ static int osd_req_op_notify_ack_init(struct ceph_osd_request *req, int which, > ceph_pagelist_init(pl); > ret = ceph_pagelist_encode_64(pl, notify_id); > ret |= ceph_pagelist_encode_64(pl, cookie); > - if (payload) { > - ret |= ceph_pagelist_encode_32(pl, payload_len); > - ret |= ceph_pagelist_append(pl, payload, payload_len); > - } else { > + if (payload) > + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); > + else > ret |= ceph_pagelist_encode_32(pl, 0); > - } > + > if (ret) { > ceph_pagelist_release(pl); > + if (ret & -ERANGE) > + return -ERANGE; > return -ENOMEM; > } > > @@ -4678,10 +4679,11 @@ static int osd_req_op_notify_init(struct ceph_osd_request *req, int which, > ceph_pagelist_init(pl); > ret = ceph_pagelist_encode_32(pl, 1); /* prot_ver */ > ret |= ceph_pagelist_encode_32(pl, timeout); > - ret |= ceph_pagelist_encode_32(pl, payload_len); > - ret |= ceph_pagelist_append(pl, payload, payload_len); > + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); > if (ret) { > ceph_pagelist_release(pl); > + if (ret & -ERANGE) > + return -ERANGE; > return -ENOMEM; > } > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> >> --- >> 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 >> if (default_acl) { >> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); >> @@ -238,12 +241,15 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >> goto out_err; >> err = ceph_pagelist_encode_string(pagelist, >> XATTR_NAME_POSIX_ACL_DEFAULT, len); >> + if (err) >> + goto out_err; >> err = posix_acl_to_xattr(&init_user_ns, default_acl, >> tmp_buf, val_size2); >> if (err < 0) >> goto out_err; >> - ceph_pagelist_encode_32(pagelist, val_size2); >> - ceph_pagelist_append(pagelist, tmp_buf, val_size2); >> + err = ceph_pagelist_encode_string(pagelist, tmp_buf, val_size2); >> + if (err) >> + goto out_err; >> } >> kfree(tmp_buf); >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> index f2584fe1246f..720ac564d392 100644 >> --- a/net/ceph/osd_client.c >> +++ b/net/ceph/osd_client.c >> @@ -4607,14 +4607,15 @@ static int osd_req_op_notify_ack_init(struct ceph_osd_request *req, int which, >> ceph_pagelist_init(pl); >> ret = ceph_pagelist_encode_64(pl, notify_id); >> ret |= ceph_pagelist_encode_64(pl, cookie); >> - if (payload) { >> - ret |= ceph_pagelist_encode_32(pl, payload_len); >> - ret |= ceph_pagelist_append(pl, payload, payload_len); >> - } else { >> + if (payload) >> + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); >> + else >> ret |= ceph_pagelist_encode_32(pl, 0); >> - } >> + >> if (ret) { >> ceph_pagelist_release(pl); >> + if (ret & -ERANGE) >> + return -ERANGE; >> return -ENOMEM; >> } >> @@ -4678,10 +4679,11 @@ static int osd_req_op_notify_init(struct ceph_osd_request *req, int which, >> ceph_pagelist_init(pl); >> ret = ceph_pagelist_encode_32(pl, 1); /* prot_ver */ >> ret |= ceph_pagelist_encode_32(pl, timeout); >> - ret |= ceph_pagelist_encode_32(pl, payload_len); >> - ret |= ceph_pagelist_append(pl, payload, payload_len); >> + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); >> if (ret) { >> ceph_pagelist_release(pl); >> + if (ret & -ERANGE) >> + return -ERANGE; >> return -ENOMEM; >> } >> > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/2018 02:32 PM, Yan, Zheng wrote: > >> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> >>> --- >>> 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 Thanks, Chengguang. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@gmx.com> wrote: > > > On 06/26/2018 02:32 PM, Yan, Zheng wrote: > > > >> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> > >>> --- > >>> 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. Regards Yan, Zheng > Thanks, > Chengguang. > > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 10:17 AM Yan, Zheng <ukernel@gmail.com> wrote: > > On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@gmx.com> wrote: > > > > > > On 06/26/2018 02:32 PM, Yan, Zheng wrote: > > > > > >> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> > > >>> --- > > >>> 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. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 4:39 PM Ilya Dryomov <idryomov@gmail.com> wrote: > > On Tue, Jun 26, 2018 at 10:17 AM Yan, Zheng <ukernel@gmail.com> wrote: > > > > On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@gmx.com> wrote: > > > > > > > > > On 06/26/2018 02:32 PM, Yan, Zheng wrote: > > > > > > > >> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> > > > >>> --- > > > >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/2018 04:39 PM, Ilya Dryomov wrote: > On Tue, Jun 26, 2018 at 10:17 AM Yan, Zheng <ukernel@gmail.com> wrote: >> On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@gmx.com> wrote: >>> >>> On 06/26/2018 02:32 PM, Yan, Zheng wrote: >>>>> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> >>>>>> --- >>>>>> 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. Thanks, Chengguang. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 11:04 AM cgxu519 <cgxu519@gmx.com> wrote: > > > On 06/26/2018 04:39 PM, Ilya Dryomov wrote: > > On Tue, Jun 26, 2018 at 10:17 AM Yan, Zheng <ukernel@gmail.com> wrote: > >> On Tue, Jun 26, 2018 at 2:44 PM cgxu519 <cgxu519@gmx.com> wrote: > >>> > >>> On 06/26/2018 02:32 PM, Yan, Zheng wrote: > >>>>> On Jun 26, 2018, at 13:04, cgxu519 <cgxu519@gmx.com> 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@gmx.com> > >>>>>> --- > >>>>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } if (default_acl) { size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); @@ -238,12 +241,15 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, goto out_err; err = ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_DEFAULT, len); + if (err) + goto out_err; err = posix_acl_to_xattr(&init_user_ns, default_acl, tmp_buf, val_size2); if (err < 0) goto out_err; - ceph_pagelist_encode_32(pagelist, val_size2); - ceph_pagelist_append(pagelist, tmp_buf, val_size2); + err = ceph_pagelist_encode_string(pagelist, tmp_buf, val_size2); + if (err) + goto out_err; } kfree(tmp_buf); diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index f2584fe1246f..720ac564d392 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -4607,14 +4607,15 @@ static int osd_req_op_notify_ack_init(struct ceph_osd_request *req, int which, ceph_pagelist_init(pl); ret = ceph_pagelist_encode_64(pl, notify_id); ret |= ceph_pagelist_encode_64(pl, cookie); - if (payload) { - ret |= ceph_pagelist_encode_32(pl, payload_len); - ret |= ceph_pagelist_append(pl, payload, payload_len); - } else { + if (payload) + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); + else ret |= ceph_pagelist_encode_32(pl, 0); - } + if (ret) { ceph_pagelist_release(pl); + if (ret & -ERANGE) + return -ERANGE; return -ENOMEM; } @@ -4678,10 +4679,11 @@ static int osd_req_op_notify_init(struct ceph_osd_request *req, int which, ceph_pagelist_init(pl); ret = ceph_pagelist_encode_32(pl, 1); /* prot_ver */ ret |= ceph_pagelist_encode_32(pl, timeout); - ret |= ceph_pagelist_encode_32(pl, payload_len); - ret |= ceph_pagelist_append(pl, payload, payload_len); + ret |= ceph_pagelist_encode_string(pl, payload, payload_len); if (ret) { ceph_pagelist_release(pl); + if (ret & -ERANGE) + return -ERANGE; return -ENOMEM; }
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@gmx.com> --- 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(-)