| Message ID | 20190417183959.11296-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: dentry->d_name handing fixes | expand |
On Thu, Apr 18, 2019 at 2:43 AM Jeff Layton <jlayton@kernel.org> wrote: > > We make copies of the dentry name in set_request_path_attr, but then > create_request_message re-fetches the lengths out of the dentry. Those > lengths may not be correct if we race with a rename. Use the pathlen > values that set_request_path_attr returned instead. > > Cc: stable@vger.kernel.org > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/mds_client.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 4cfefe118128..f362c16c5119 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -2305,9 +2305,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc, > (!!req->r_inode_drop + !!req->r_dentry_drop + > !!req->r_old_inode_drop + !!req->r_old_dentry_drop); > if (req->r_dentry_drop) > - len += req->r_dentry->d_name.len; > + len += pathlen1; > if (req->r_old_dentry_drop) > - len += req->r_old_dentry->d_name.len; > + len += pathlen2; > > msg = ceph_msg_new2(CEPH_MSG_CLIENT_REQUEST, len, 1, GFP_NOFS, false); > if (!msg) { > -- > 2.20.1 > This patch may not be needed. because when r_dentry_drop or r_old_dentry_drop is set, parent dir should be locked.
On Wed, Apr 17, 2019 at 10:33 PM Yan, Zheng <ukernel@gmail.com> wrote: > > On Thu, Apr 18, 2019 at 2:43 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > We make copies of the dentry name in set_request_path_attr, but then > > create_request_message re-fetches the lengths out of the dentry. Those > > lengths may not be correct if we race with a rename. Use the pathlen > > values that set_request_path_attr returned instead. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/mds_client.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index 4cfefe118128..f362c16c5119 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -2305,9 +2305,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc, > > (!!req->r_inode_drop + !!req->r_dentry_drop + > > !!req->r_old_inode_drop + !!req->r_old_dentry_drop); > > if (req->r_dentry_drop) > > - len += req->r_dentry->d_name.len; > > + len += pathlen1; > > if (req->r_old_dentry_drop) > > - len += req->r_old_dentry->d_name.len; > > + len += pathlen2; > > > > msg = ceph_msg_new2(CEPH_MSG_CLIENT_REQUEST, len, 1, GFP_NOFS, false); > > if (!msg) { > > -- > > 2.20.1 > > > > This patch may not be needed. because when r_dentry_drop or > r_old_dentry_drop is set, parent dir should be locked. Got it. Ok, let's drop this one for v5.1 and stable. That said, I'd prefer we not rely on those sorts of subtle assumptions. How about we take this one for v5.2?
On Thu, Apr 18, 2019 at 7:18 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, Apr 17, 2019 at 10:33 PM Yan, Zheng <ukernel@gmail.com> wrote: > > > > On Thu, Apr 18, 2019 at 2:43 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > We make copies of the dentry name in set_request_path_attr, but then > > > create_request_message re-fetches the lengths out of the dentry. Those > > > lengths may not be correct if we race with a rename. Use the pathlen > > > values that set_request_path_attr returned instead. > > > > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/ceph/mds_client.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > index 4cfefe118128..f362c16c5119 100644 > > > --- a/fs/ceph/mds_client.c > > > +++ b/fs/ceph/mds_client.c > > > @@ -2305,9 +2305,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc, > > > (!!req->r_inode_drop + !!req->r_dentry_drop + > > > !!req->r_old_inode_drop + !!req->r_old_dentry_drop); > > > if (req->r_dentry_drop) > > > - len += req->r_dentry->d_name.len; > > > + len += pathlen1; > > > if (req->r_old_dentry_drop) > > > - len += req->r_old_dentry->d_name.len; > > > + len += pathlen2; > > > > > > msg = ceph_msg_new2(CEPH_MSG_CLIENT_REQUEST, len, 1, GFP_NOFS, false); > > > if (!msg) { > > > -- > > > 2.20.1 > > > > > > > This patch may not be needed. because when r_dentry_drop or > > r_old_dentry_drop is set, parent dir should be locked. > > Got it. Ok, let's drop this one for v5.1 and stable. > > That said, I'd prefer we not rely on those sorts of subtle > assumptions. How about we take this one for v5.2? But ceph_encode_dentry_release() still uses r_dentry directly. It needs to be updated too. > -- > Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 4cfefe118128..f362c16c5119 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2305,9 +2305,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc, (!!req->r_inode_drop + !!req->r_dentry_drop + !!req->r_old_inode_drop + !!req->r_old_dentry_drop); if (req->r_dentry_drop) - len += req->r_dentry->d_name.len; + len += pathlen1; if (req->r_old_dentry_drop) - len += req->r_old_dentry->d_name.len; + len += pathlen2; msg = ceph_msg_new2(CEPH_MSG_CLIENT_REQUEST, len, 1, GFP_NOFS, false); if (!msg) {
We make copies of the dentry name in set_request_path_attr, but then create_request_message re-fetches the lengths out of the dentry. Those lengths may not be correct if we race with a rename. Use the pathlen values that set_request_path_attr returned instead. Cc: stable@vger.kernel.org Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/mds_client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)