Message ID | 20220606072835.302935-1-xiubli@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ceph: fix incorrectly assigning random values to peer's members | expand |
Xiubo Li <xiubli@redhat.com> writes: > For export the peer is empty in ceph. > > URL: https://tracker.ceph.com/issues/55857 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0a48bf829671..8efa46ff4282 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -4127,16 +4127,11 @@ void ceph_handle_caps(struct ceph_mds_session *session, > p += flock_len; > } > > - if (msg_version >= 3) { > - if (op == CEPH_CAP_OP_IMPORT) { > - if (p + sizeof(*peer) > end) > - goto bad; > - peer = p; > - p += sizeof(*peer); > - } else if (op == CEPH_CAP_OP_EXPORT) { > - /* recorded in unused fields */ > - peer = (void *)&h->size; > - } > + if (msg_version >= 3 && op == CEPH_CAP_OP_IMPORT) { > + if (p + sizeof(*peer) > end) > + goto bad; > + peer = p; > + p += sizeof(*peer); > } > > if (msg_version >= 4) { > -- > > 2.36.0.rc1 > Are you sure this isn't breaking anything? Looking at MClientCaps.h, it seems to be doing something similar, i.e. for the CAP_OP_EXPORT, the 'peer' is encoded where the 'size', 'max_size', 'truncate_size',... are for the CAP_OP_IMPORT. This is definitely confusing and messy, but not sure if your change isn't breaking something. Cheers,
On Mon, 2022-06-06 at 15:28 +0800, Xiubo Li wrote: > For export the peer is empty in ceph. > > URL: https://tracker.ceph.com/issues/55857 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/caps.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 0a48bf829671..8efa46ff4282 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -4127,16 +4127,11 @@ void ceph_handle_caps(struct ceph_mds_session *session, > p += flock_len; > } > > - if (msg_version >= 3) { > - if (op == CEPH_CAP_OP_IMPORT) { > - if (p + sizeof(*peer) > end) > - goto bad; > - peer = p; > - p += sizeof(*peer); > - } else if (op == CEPH_CAP_OP_EXPORT) { > - /* recorded in unused fields */ > - peer = (void *)&h->size; > - } > + if (msg_version >= 3 && op == CEPH_CAP_OP_IMPORT) { > + if (p + sizeof(*peer) > end) > + goto bad; > + peer = p; > + p += sizeof(*peer); > } > > if (msg_version >= 4) { This was added in commit 11df2dfb61 (ceph: add imported caps when handling cap export message). If peer should always be NULL on an export, I wonder what he was thinking by adding this in the first place? Zheng, could you take a look here? If this does turn out to be correct, then I think there is some further cleanup you can do here, as you should be able to drop the peer argument from handle_cap_export. That should also collapse some of the code down in that function as well since lot of the target fields end up being 0s.
On 6/6/22 6:23 PM, Jeff Layton wrote: > On Mon, 2022-06-06 at 15:28 +0800, Xiubo Li wrote: >> For export the peer is empty in ceph. >> >> URL: https://tracker.ceph.com/issues/55857 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/caps.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 0a48bf829671..8efa46ff4282 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -4127,16 +4127,11 @@ void ceph_handle_caps(struct ceph_mds_session *session, >> p += flock_len; >> } >> >> - if (msg_version >= 3) { >> - if (op == CEPH_CAP_OP_IMPORT) { >> - if (p + sizeof(*peer) > end) >> - goto bad; >> - peer = p; >> - p += sizeof(*peer); >> - } else if (op == CEPH_CAP_OP_EXPORT) { >> - /* recorded in unused fields */ >> - peer = (void *)&h->size; >> - } >> + if (msg_version >= 3 && op == CEPH_CAP_OP_IMPORT) { >> + if (p + sizeof(*peer) > end) >> + goto bad; >> + peer = p; >> + p += sizeof(*peer); >> } >> >> if (msg_version >= 4) { > This was added in commit 11df2dfb61 (ceph: add imported caps when > handling cap export message). If peer should always be NULL on an > export, I wonder what he was thinking by adding this in the first place? > Zheng, could you take a look here? > > If this does turn out to be correct, then I think there is some further > cleanup you can do here, as you should be able to drop the peer argument > from handle_cap_export. That should also collapse some of the code down > in that function as well since lot of the target fields end up being 0s. Okay, will drop this. The head structs are different in ceph and kernel. -- Xiubo
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 0a48bf829671..8efa46ff4282 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -4127,16 +4127,11 @@ void ceph_handle_caps(struct ceph_mds_session *session, p += flock_len; } - if (msg_version >= 3) { - if (op == CEPH_CAP_OP_IMPORT) { - if (p + sizeof(*peer) > end) - goto bad; - peer = p; - p += sizeof(*peer); - } else if (op == CEPH_CAP_OP_EXPORT) { - /* recorded in unused fields */ - peer = (void *)&h->size; - } + if (msg_version >= 3 && op == CEPH_CAP_OP_IMPORT) { + if (p + sizeof(*peer) > end) + goto bad; + peer = p; + p += sizeof(*peer); } if (msg_version >= 4) {
For export the peer is empty in ceph. URL: https://tracker.ceph.com/issues/55857 Signed-off-by: Xiubo Li <xiubli@redhat.com> --- fs/ceph/caps.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)