[PATCH] ceph: fix incorrectly assigning random values to peer's members

[PATCH] ceph: fix incorrectly assigning random values to peer's members

[Xiubo Li]

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

Re: [PATCH] ceph: fix incorrectly assigning random values to peer's members

[Luís Henriques]

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,
-- 
Luís

Re: [PATCH] ceph: fix incorrectly assigning random values to peer's members

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: fix incorrectly assigning random values to peer's members

[Xiubo Li]

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