[RFC] ceph: fix cap revoke race
diff mbox series

Message ID 20191127104549.33305-1-xiubli@redhat.com
State New
Headers show
Series
  • [RFC] ceph: fix cap revoke race
Related show

Commit Message

Xiubo Li Nov. 27, 2019, 10:45 a.m. UTC
From: Xiubo Li <xiubli@redhat.com>

The cap->implemented is one subset of the cap->issued, the logic
here want to exclude the revoking caps, but the following code
will be (~cap->implemented | cap->issued) == 0xFFFF, so it will
make no sense when we doing the "have &= 0xFFFF".

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/caps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff Layton Nov. 27, 2019, 11:03 a.m. UTC | #1
On Wed, 2019-11-27 at 05:45 -0500, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The cap->implemented is one subset of the cap->issued, the logic
> here want to exclude the revoking caps, but the following code
> will be (~cap->implemented | cap->issued) == 0xFFFF, so it will
> make no sense when we doing the "have &= 0xFFFF".
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/caps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index c62e88da4fee..a9335402c2a5 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented)
>  	 */
>  	if (ci->i_auth_cap) {
>  		cap = ci->i_auth_cap;
> -		have &= ~cap->implemented | cap->issued;
> +		have &= ~(cap->implemented & ~cap->issued);
>  	}
>  	return have;
>  }

Nice catch. This patch looks correct to me. I'll merge it into the
testing branch and we'll see if anything breaks.
Yan, Zheng Nov. 28, 2019, 2:25 a.m. UTC | #2
On 11/27/19 6:45 PM, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The cap->implemented is one subset of the cap->issued, the logic
> here want to exclude the revoking caps, but the following code
> will be (~cap->implemented | cap->issued) == 0xFFFF, so it will
> make no sense when we doing the "have &= 0xFFFF".
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>   fs/ceph/caps.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index c62e88da4fee..a9335402c2a5 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented)
>   	 */
>   	if (ci->i_auth_cap) {
>   		cap = ci->i_auth_cap;
> -		have &= ~cap->implemented | cap->issued;
> +		have &= ~(cap->implemented & ~cap->issued);

The end result is the same.

See https://en.wikipedia.org/wiki/De_Morgan%27s_laws

>   	}
>   	return have;
>   }
>
Xiubo Li Nov. 28, 2019, 7:53 a.m. UTC | #3
On 2019/11/28 10:25, Yan, Zheng wrote:
> On 11/27/19 6:45 PM, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The cap->implemented is one subset of the cap->issued, the logic
>> here want to exclude the revoking caps, but the following code
>> will be (~cap->implemented | cap->issued) == 0xFFFF, so it will
>> make no sense when we doing the "have &= 0xFFFF".
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/caps.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index c62e88da4fee..a9335402c2a5 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info 
>> *ci, int *implemented)
>>        */
>>       if (ci->i_auth_cap) {
>>           cap = ci->i_auth_cap;
>> -        have &= ~cap->implemented | cap->issued;
>> +        have &= ~(cap->implemented & ~cap->issued);
>
> The end result is the same.
>
> See https://en.wikipedia.org/wiki/De_Morgan%27s_laws
>
Yeah, right, it is.

BRs



>>       }
>>       return have;
>>   }
>>
>
Jeff Layton Dec. 2, 2019, 1:24 p.m. UTC | #4
On Thu, 2019-11-28 at 15:53 +0800, Xiubo Li wrote:
> On 2019/11/28 10:25, Yan, Zheng wrote:
> > On 11/27/19 6:45 PM, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > > 
> > > The cap->implemented is one subset of the cap->issued, the logic
> > > here want to exclude the revoking caps, but the following code
> > > will be (~cap->implemented | cap->issued) == 0xFFFF, so it will
> > > make no sense when we doing the "have &= 0xFFFF".
> > > 
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >   fs/ceph/caps.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > index c62e88da4fee..a9335402c2a5 100644
> > > --- a/fs/ceph/caps.c
> > > +++ b/fs/ceph/caps.c
> > > @@ -812,7 +812,7 @@ int __ceph_caps_issued(struct ceph_inode_info 
> > > *ci, int *implemented)
> > >        */
> > >       if (ci->i_auth_cap) {
> > >           cap = ci->i_auth_cap;
> > > -        have &= ~cap->implemented | cap->issued;
> > > +        have &= ~(cap->implemented & ~cap->issued);
> > 
> > The end result is the same.
> > 
> > See https://en.wikipedia.org/wiki/De_Morgan%27s_laws
> > 
> Yeah, right, it is.
> 
> BRs
> 

Dropping this patch.

Cheers,

Patch
diff mbox series

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index c62e88da4fee..a9335402c2a5 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -812,7 +812,7 @@  int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented)
 	 */
 	if (ci->i_auth_cap) {
 		cap = ci->i_auth_cap;
-		have &= ~cap->implemented | cap->issued;
+		have &= ~(cap->implemented & ~cap->issued);
 	}
 	return have;
 }