From patchwork Tue Jul 25 13:01:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Dryomov X-Patchwork-Id: 9861963 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D755D602B1 for ; Tue, 25 Jul 2017 13:02:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C55AE2522B for ; Tue, 25 Jul 2017 13:02:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B5E0D2621B; Tue, 25 Jul 2017 13:02:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID, T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 38E032522B for ; Tue, 25 Jul 2017 13:02:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751965AbdGYNBb (ORCPT ); Tue, 25 Jul 2017 09:01:31 -0400 Received: from mail-ua0-f195.google.com ([209.85.217.195]:38639 "EHLO mail-ua0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751929AbdGYNBa (ORCPT ); Tue, 25 Jul 2017 09:01:30 -0400 Received: by mail-ua0-f195.google.com with SMTP id x24so12262989uah.5 for ; Tue, 25 Jul 2017 06:01:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=zkGmHKIi5mG69+5MHBrhwCmZ9Re99voCVV5yn32mSbo=; b=cAJ2hTn29M14vL+PDclJzPDKQqwrnpHT3wlQvi6d4Its8Z7qS3NP/wgcm2GuUrlcdZ HGY8m3TAm8pqLTujFZF81FavhqZ/SDwTCZdh6YGiAEnW29OHKkI1JJCLrJoh2xllaBaR bLB9ZPx0sgw/HqEMnghq5/rZ/am6iEsd85/PqE7Xj8bx04ZytsrO5CiYRcWPuxp7wmLL nruA3vNIH0UXQhS0nzIoOhEW0/+Hw5o3c73XtAoZBR7Bp+gem41zsEsVgDtfWyDVlclq oOAxk9K0XFI1ckN8XxRtmyVljVXTfz59I2prvXKnYeQbtqWH+KIt6GwPWbUXcnhnIIMu /usQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=zkGmHKIi5mG69+5MHBrhwCmZ9Re99voCVV5yn32mSbo=; b=luyZl5imvh1jBP57WTVlEnhiv+YCJmnIQtIpKlwBCK7Bry+rLcy70iUEMORGWG+upK NXBPYtDo3QDdCkFPB1mTvLgs/Zp3kbOEJL7373g1LLNadgT8JMPUNDsGzCQyXUPoxvdA /R9YHHLdeJIdnIbb0GN9jnfGW3U0sjUJxrOxG7+jzhE1cVpRy2VBfVNkwd00NynU8lax qyNtxRGvLAzcDhdSOf9h1BBNcHS7dIOLxoEhV67fMpaSMPRRhIvyBJCRJn5D4BwFToEE 04y4ojkWeEzZ93tSvXcTQ9HNFLl+a5NmuU0PKctgiKzLW3NJpaVf968+MPMldzjxpg2x Gadg== X-Gm-Message-State: AIVw1126cpq0iTtUNAvsfbiS+cSpFuvsuVTH+Svz0FUjAE8lgP9M+02m fJnw/BBgjdK4BsqnvOpRSxiIqrPri3SlwWw= X-Received: by 10.31.186.130 with SMTP id k124mr8721604vkf.124.1500987689337; Tue, 25 Jul 2017 06:01:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.176.1.175 with HTTP; Tue, 25 Jul 2017 06:01:28 -0700 (PDT) In-Reply-To: <20170724132802.7916-1-zyan@redhat.com> References: <20170724132802.7916-1-zyan@redhat.com> From: Ilya Dryomov Date: Tue, 25 Jul 2017 15:01:28 +0200 Message-ID: Subject: Re: [PATCH] libceph: fix osd request encoding regression To: "Yan, Zheng" Cc: Ceph Development Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jul 24, 2017 at 3:28 PM, Yan, Zheng 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" > --- > 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 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" Signed-off-by: Ilya Dryomov --- 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