Re: [PATCH] libceph: fix osd request encoding regression

[Ilya Dryomov <idryomov@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3917 bytes --]

On Tue, Jul 25, 2017 at 3:33 PM, Ilya Dryomov <idryomov@gmail.com> wrote:
> On Tue, Jul 25, 2017 at 3:14 PM, Yan, Zheng <zyan@redhat.com> wrote:
>>
>>> On 25 Jul 2017, at 21:01, Ilya Dryomov <idryomov@gmail.com> wrote:
>>>
>>> On Mon, Jul 24, 2017 at 3:28 PM, Yan, Zheng <zyan@redhat.com> 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@redhat.com>
>>>> ---
>>>> 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.
>>
>> does encode_request_finish() get called each time we re-send a message?
>> If it does, your patch seems incorrect. encode_request_finish() appends extra
>> 8 bytes to the message each time it get called.
>
> Hrm, it is.  But then your patch doesn't fix it either because the
> problem is the ->reencode_message() call itself -- I didn't intend it
> to be called on every resend.  Let me run some tests...

OK, so ->reencode_message() being called more than once could affect
the MDS client, but since only the OSD client is using it, there are no
immediate issues.  I'll post the attached patch for the sake of
robustness though.

Thanks,

                Ilya

[-- Attachment #2: 0001-libceph-don-t-call-reencode_message-more-than-once-p.patch --]
[-- Type: text/x-patch, Size: 1355 bytes --]

From 8524b57a5881b548ecb17b150aaf599e647b8982 Mon Sep 17 00:00:00 2001
From: Ilya Dryomov <idryomov@gmail.com>
Date: Wed, 26 Jul 2017 09:59:15 +0200
Subject: [PATCH] libceph: don't call ->reencode_message() more than once per
 message

Reencoding an already reencoded message is a bad idea.  This could
happen on Policy::stateful_server connections (!CEPH_MSG_CONNECT_LOSSY),
such as MDS sessions.

This didn't pop up in testing because currently only OSD requests are
reencoded and OSD sessions are always lossy.

Fixes: 98ad5ebd1505 ("libceph: ceph_connection_operations::reencode_message() method")
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/messenger.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index b7cc615d42ef..a67298c7e0cd 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1287,10 +1287,10 @@ static void prepare_write_message(struct ceph_connection *con)
 	if (m->needs_out_seq) {
 		m->hdr.seq = cpu_to_le64(++con->out_seq);
 		m->needs_out_seq = false;
-	}
 
-	if (con->ops->reencode_message)
-		con->ops->reencode_message(m);
+		if (con->ops->reencode_message)
+			con->ops->reencode_message(m);
+	}
 
 	dout("prepare_write_message %p seq %lld type %d len %d+%d+%zd\n",
 	     m, con->out_seq, le16_to_cpu(m->hdr.type),
-- 
2.4.3