| Message ID | 1395944299-21970-10-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/27/2014 01:17 PM, Ilya Dryomov wrote: > The size of the memory area feeded to crush_decode() should be limited > not only by osdmap end, but also by the crush map length. Also, drop You're also letting crush_decode() verify it has the buffer space it needs internally, rather than checking it before making the call, which is good. (Though I guess you don't have to mention it.) > unnecessary dout() (dout() in crush_decode() conveys the same info) and > step past crush map only if it is decoded successfully. I actually think crush_decode() should take a (void **) instead, as its first argument and advance the pointer by as much as it uses (like most of the other routines do). That's a suggestion, but I don't really care, this is fine. Reviewed-by: Alex Elder <elder@linaro.org> > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > net/ceph/osdmap.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c > index 19aca4d3c5dd..b70357adbdc0 100644 > --- a/net/ceph/osdmap.c > +++ b/net/ceph/osdmap.c > @@ -796,16 +796,13 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) > > /* crush */ > ceph_decode_32_safe(p, end, len, e_inval); > - dout("osdmap_decode crush len %d from off 0x%x\n", len, > - (int)(*p - start)); > - ceph_decode_need(p, end, len, e_inval); > - map->crush = crush_decode(*p, end); > - *p += len; > + map->crush = crush_decode(*p, min(*p + len, end)); > if (IS_ERR(map->crush)) { > err = PTR_ERR(map->crush); > map->crush = NULL; > goto bad; > } > + *p += len; > > /* ignore the rest */ > *p = end; > -- 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 Thu, Mar 27, 2014 at 9:45 PM, Alex Elder <elder@ieee.org> wrote: > On 03/27/2014 01:17 PM, Ilya Dryomov wrote: >> The size of the memory area feeded to crush_decode() should be limited >> not only by osdmap end, but also by the crush map length. Also, drop > > You're also letting crush_decode() verify it has the buffer space > it needs internally, rather than checking it before making the call, > which is good. (Though I guess you don't have to mention it.) Yes. > >> unnecessary dout() (dout() in crush_decode() conveys the same info) and >> step past crush map only if it is decoded successfully. > > I actually think crush_decode() should take a (void **) > instead, as its first argument and advance the pointer > by as much as it uses (like most of the other routines do). > That's a suggestion, but I don't really care, this is fine. Me too, and I considered it, but it's the only decode helper that takes a (void *) and it even names it "pbyval", which suggests that it was intentional, so I kept it the way it is. 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/net/ceph/osdmap.c b/net/ceph/osdmap.c index 19aca4d3c5dd..b70357adbdc0 100644 --- a/net/ceph/osdmap.c +++ b/net/ceph/osdmap.c @@ -796,16 +796,13 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) /* crush */ ceph_decode_32_safe(p, end, len, e_inval); - dout("osdmap_decode crush len %d from off 0x%x\n", len, - (int)(*p - start)); - ceph_decode_need(p, end, len, e_inval); - map->crush = crush_decode(*p, end); - *p += len; + map->crush = crush_decode(*p, min(*p + len, end)); if (IS_ERR(map->crush)) { err = PTR_ERR(map->crush); map->crush = NULL; goto bad; } + *p += len; /* ignore the rest */ *p = end;
The size of the memory area feeded to crush_decode() should be limited not only by osdmap end, but also by the crush map length. Also, drop unnecessary dout() (dout() in crush_decode() conveys the same info) and step past crush map only if it is decoded successfully. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- net/ceph/osdmap.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)