On Mon, Jul 24, 2017 at 3:28 PM, Yan, Zheng <zyan@xxxxxxxxxx> wrote:
> The new BUG_ON in encode_request_partial() verifies that space used
> by encoding request front is exactly equal to request message size.
> This is wrong because request messages allocated from mempool always
> have size PAGE_SIZE.
>
> Signed-off-by: "Yan, Zheng" <zyan@xxxxxxxxxx>
> ---
> net/ceph/osd_client.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 5c9d696..81f6199 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1913,10 +1913,11 @@ static void encode_request_partial(struct ceph_osd_request *req,
> }
>
> ceph_encode_32(&p, req->r_attempts); /* retry_attempt */
> - BUG_ON(p != end - 8); /* space for features */
> + BUG_ON(p + 8 > end); /* space for features */
>
> msg->hdr.version = cpu_to_le16(8); /* MOSDOp v8 */
> - /* front_len is finalized in encode_request_finish() */
> + msg->front.iov_len = p + 8 - msg->front.iov_base;
> + msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> msg->hdr.data_len = cpu_to_le32(data_len);
> /*
> * The header "data_off" is a hint to the receiver allowing it
> @@ -1932,7 +1933,7 @@ static void encode_request_partial(struct ceph_osd_request *req,
> static void encode_request_finish(struct ceph_msg *msg)
> {
> void *p = msg->front.iov_base;
> - void *const end = p + msg->front_alloc_len;
> + void *const end = p + msg->front.iov_len;
>
> if (CEPH_HAVE_FEATURE(msg->con->peer_features, RESEND_ON_SPLIT)) {
> /* luminous OSD -- encode features and be done */
> @@ -2008,11 +2009,11 @@ static void encode_request_finish(struct ceph_msg *msg)
> p += tail_len;
>
> msg->hdr.version = cpu_to_le16(4); /* MOSDOp v4 */
> - }
>
> - BUG_ON(p > end);
> - msg->front.iov_len = p - msg->front.iov_base;
> - msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> + BUG_ON(p > end);
> + msg->front.iov_len = p - msg->front.iov_base;
> + msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> + }
>
> dout("%s msg %p tid %llu %u+%u+%u v%d\n", __func__, msg,
> le64_to_cpu(msg->hdr.tid), le32_to_cpu(msg->hdr.front_len),
> @@ -3981,7 +3982,7 @@ static struct ceph_msg *create_backoff_message(
> return NULL;
>
> p = msg->front.iov_base;
> - end = p + msg->front_alloc_len;
> + end = p + msg->front.iov_len;
>
> encode_spgid(&p, &backoff->spgid);
> ceph_encode_32(&p, map_epoch);
Hi Zheng,
How about the attached patch instead? It's shorter and more
importantly preserves the existing structure.
What is create_backoff_message() hunk for? Backoff messages are
never allocated out of ceph_msgpool.
Thanks,
Ilya
From ca074c13f159e10f0b0cc3f2af74647925e600f1 Mon Sep 17 00:00:00 2001
From: Ilya Dryomov <idryomov@xxxxxxxxx>
Date: Tue, 25 Jul 2017 14:40:03 +0200
Subject: [PATCH] libceph: make encode_request_*() work with r_mempool requests
Messages allocated out of ceph_msgpool have a fixed front length
(pool->front_len). Asserting that the entire front has been filled
while encoding is thus wrong.
Fixes: 8cb441c0545d ("libceph: MOSDOp v8 encoding (actual spgid + full hash)")
Reported-by: "Yan, Zheng" <zyan@xxxxxxxxxx>
Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
---
net/ceph/osd_client.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 901bb8221366..b5f016cb9569 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1918,10 +1918,12 @@ static void encode_request_partial(struct ceph_osd_request *req,
}
ceph_encode_32(&p, req->r_attempts); /* retry_attempt */
- BUG_ON(p != end - 8); /* space for features */
+ BUG_ON(p > end - 8); /* space for features */
msg->hdr.version = cpu_to_le16(8); /* MOSDOp v8 */
/* front_len is finalized in encode_request_finish() */
+ msg->front.iov_len = p - msg->front.iov_base;
+ msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
msg->hdr.data_len = cpu_to_le32(data_len);
/*
* The header "data_off" is a hint to the receiver allowing it
@@ -1937,11 +1939,12 @@ static void encode_request_partial(struct ceph_osd_request *req,
static void encode_request_finish(struct ceph_msg *msg)
{
void *p = msg->front.iov_base;
+ void *const partial_end = p + msg->front.iov_len;
void *const end = p + msg->front_alloc_len;
if (CEPH_HAVE_FEATURE(msg->con->peer_features, RESEND_ON_SPLIT)) {
/* luminous OSD -- encode features and be done */
- p = end - 8;
+ p = partial_end;
ceph_encode_64(&p, msg->con->peer_features);
} else {
struct {
@@ -1984,7 +1987,7 @@ static void encode_request_finish(struct ceph_msg *msg)
oid_len = p - oid;
tail = p;
- tail_len = (end - p) - 8;
+ tail_len = partial_end - p;
p = msg->front.iov_base;
ceph_encode_copy(&p, &head.client_inc, sizeof(head.client_inc));
--
2.4.3
