From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 23C01C43217 for ; Tue, 1 Mar 2022 13:11:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234927AbiCANLi (ORCPT ); Tue, 1 Mar 2022 08:11:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47104 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232939AbiCANLh (ORCPT ); Tue, 1 Mar 2022 08:11:37 -0500 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 98B7620F56; Tue, 1 Mar 2022 05:10:55 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id 0B264CE0490; Tue, 1 Mar 2022 13:10:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CC0D7C340EE; Tue, 1 Mar 2022 13:10:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1646140252; bh=Q3ALUNiJP3tH93zumbyGEqNo1zCA/OC+QE23POQCNbU=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=nFwmcyaPWK3dIfFZ9+OZtl+DJKoA7Tw8vdbIeMHs5Nse7yRwpWPWWn2PJcVH16WJ6 MFgBmxJ+Kn+6bLqpslvT83T8l0SF60iFHgKTQIc3cePSzc1pu8ym6SA3DwZfsumiNq 6UFLaVXAQKclYEGQ2y+cK4E8B8ml7GBRWwOMxg9r1FPkJ2YVQCCjm/I9XtVjLbUWXf SNp8h41jgJgoGCZPifePeq8mH+ZVWoO7J3ljGlwuV1foYPl/mzjOWoOQpFN0JQIr+e HPAful414j7iiFg0eYW8M+whaEpO9dcO20ctEYa3hJPJ7ZOpTsMELmVMmtvYCqPIWP xUzXfJ2Z6TIcw== Message-ID: Subject: Re: [RFC PATCH v10 11/48] ceph: decode alternate_name in lease info From: Jeff Layton To: Xiubo Li , ceph-devel@vger.kernel.org, linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, idryomov@gmail.com Date: Tue, 01 Mar 2022 08:10:50 -0500 In-Reply-To: References: <20220111191608.88762-1-jlayton@kernel.org> <20220111191608.88762-12-jlayton@kernel.org> Content-Type: text/plain; charset="ISO-8859-15" User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Tue, 2022-03-01 at 18:57 +0800, Xiubo Li wrote: > On 1/12/22 3:15 AM, Jeff Layton wrote: > > Ceph is a bit different from local filesystems, in that we don't want > > to store filenames as raw binary data, since we may also be dealing > > with clients that don't support fscrypt. > > > > We could just base64-encode the encrypted filenames, but that could > > leave us with filenames longer than NAME_MAX. It turns out that the > > MDS doesn't care much about filename length, but the clients do. > > > > To manage this, we've added a new "alternate name" field that can be > > optionally added to any dentry that we'll use to store the binary > > crypttext of the filename if its base64-encoded value will be longer > > than NAME_MAX. When a dentry has one of these names attached, the MDS > > will send it along in the lease info, which we can then store for > > later usage. > > > > Signed-off-by: Jeff Layton > > --- > > fs/ceph/mds_client.c | 40 ++++++++++++++++++++++++++++++---------- > > fs/ceph/mds_client.h | 11 +++++++---- > > 2 files changed, 37 insertions(+), 14 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index 34a4f6dbac9d..709f3f654555 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -306,27 +306,44 @@ static int parse_reply_info_dir(void **p, void *end, > > > > static int parse_reply_info_lease(void **p, void *end, > > struct ceph_mds_reply_lease **lease, > > - u64 features) > > + u64 features, u32 *altname_len, u8 **altname) > > { > > + u8 struct_v; > > + u32 struct_len; > > + > > if (features == (u64)-1) { > > - u8 struct_v, struct_compat; > > - u32 struct_len; > > + u8 struct_compat; > > + > > ceph_decode_8_safe(p, end, struct_v, bad); > > ceph_decode_8_safe(p, end, struct_compat, bad); > > + > > /* struct_v is expected to be >= 1. we only understand > > * encoding whose struct_compat == 1. */ > > if (!struct_v || struct_compat != 1) > > goto bad; > > + > > ceph_decode_32_safe(p, end, struct_len, bad); > > - ceph_decode_need(p, end, struct_len, bad); > > - end = *p + struct_len; > > Hi Jeff, > > This is buggy, more detail please see https://tracker.ceph.com/issues/54430. > > The following patch will fix it. We should skip the extra memories anyway. > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 94b4c6508044..3dea96df4769 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -326,6 +326,7 @@ static int parse_reply_info_lease(void **p, void *end, >                         goto bad; > >                 ceph_decode_32_safe(p, end, struct_len, bad); > +               end = *p + struct_len; There may be a bug here, but this doesn't look like the right fix. "end" denotes the end of the buffer we're decoding. We don't generally want to go changing it like this. Consider what would happen if the original "end" was shorter than *p + struct_len. >         } else { >                 struct_len = sizeof(**lease); >                 *altname_len = 0; > @@ -346,6 +347,7 @@ static int parse_reply_info_lease(void **p, void *end, >                         *altname = NULL; >                         *altname_len = 0; >                 } > +               *p = end; I think we just have to do the math here. Maybe this should be something like this? *p += struct_len - sizeof(**lease) - *altname_len; >         } >         return 0; >  bad: > > > > > + } else { > > + struct_len = sizeof(**lease); > > + *altname_len = 0; > > + *altname = NULL; > > } > > > > - ceph_decode_need(p, end, sizeof(**lease), bad); > > + ceph_decode_need(p, end, struct_len, bad); > > *lease = *p; > > *p += sizeof(**lease); > > - if (features == (u64)-1) > > - *p = end; > > + > > + if (features == (u64)-1) { > > + if (struct_v >= 2) { > > + ceph_decode_32_safe(p, end, *altname_len, bad); > > + ceph_decode_need(p, end, *altname_len, bad); > > + *altname = *p; > > + *p += *altname_len; > > + } else { > > + *altname = NULL; > > + *altname_len = 0; > > + } > > + } > > return 0; > > bad: > > return -EIO; > > @@ -356,7 +373,8 @@ static int parse_reply_info_trace(void **p, void *end, > > info->dname = *p; > > *p += info->dname_len; > > > > - err = parse_reply_info_lease(p, end, &info->dlease, features); > > + err = parse_reply_info_lease(p, end, &info->dlease, features, > > + &info->altname_len, &info->altname); > > if (err < 0) > > goto out_bad; > > } > > @@ -423,9 +441,11 @@ static int parse_reply_info_readdir(void **p, void *end, > > dout("parsed dir dname '%.*s'\n", rde->name_len, rde->name); > > > > /* dentry lease */ > > - err = parse_reply_info_lease(p, end, &rde->lease, features); > > + err = parse_reply_info_lease(p, end, &rde->lease, features, > > + &rde->altname_len, &rde->altname); > > if (err) > > goto out_bad; > > + > > /* inode */ > > err = parse_reply_info_in(p, end, &rde->inode, features); > > if (err < 0) > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h > > index e7d2c8a1b9c1..128901a847af 100644 > > --- a/fs/ceph/mds_client.h > > +++ b/fs/ceph/mds_client.h > > @@ -29,8 +29,8 @@ enum ceph_feature_type { > > CEPHFS_FEATURE_MULTI_RECONNECT, > > CEPHFS_FEATURE_DELEG_INO, > > CEPHFS_FEATURE_METRIC_COLLECT, > > - > > - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT, > > + CEPHFS_FEATURE_ALTERNATE_NAME, > > + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_ALTERNATE_NAME, > > }; > > > > /* > > @@ -45,8 +45,7 @@ enum ceph_feature_type { > > CEPHFS_FEATURE_MULTI_RECONNECT, \ > > CEPHFS_FEATURE_DELEG_INO, \ > > CEPHFS_FEATURE_METRIC_COLLECT, \ > > - \ > > - CEPHFS_FEATURE_MAX, \ > > + CEPHFS_FEATURE_ALTERNATE_NAME, \ > > } > > #define CEPHFS_FEATURES_CLIENT_REQUIRED {} > > > > @@ -98,7 +97,9 @@ struct ceph_mds_reply_info_in { > > > > struct ceph_mds_reply_dir_entry { > > char *name; > > + u8 *altname; > > u32 name_len; > > + u32 altname_len; > > struct ceph_mds_reply_lease *lease; > > struct ceph_mds_reply_info_in inode; > > loff_t offset; > > @@ -117,7 +118,9 @@ struct ceph_mds_reply_info_parsed { > > struct ceph_mds_reply_info_in diri, targeti; > > struct ceph_mds_reply_dirfrag *dirfrag; > > char *dname; > > + u8 *altname; > > u32 dname_len; > > + u32 altname_len; > > struct ceph_mds_reply_lease *dlease; > > > > /* extra */ > -- Jeff Layton