[RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

From the posix and the initial statx supporting commit comments,
the AT_STATX_DONT_SYNC is a lightweight stat flag and the
AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
the other current usage about these two flags they are all doing
the same, that is only when the AT_STATX_FORCE_SYNC is not set
and the AT_STATX_DONT_SYNC is set will they skip sync retriving
the attributes from storage.

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

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 6788a1f88eb6..1ee6685def83 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
 		return -ESTALE;
 
 	/* Skip the getattr altogether if we're asked not to sync */
-	if (!(flags & AT_STATX_DONT_SYNC)) {
+	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
 		err = ceph_do_getattr(inode,
 				statx_to_caps(request_mask, inode->i_mode),
 				flags & AT_STATX_FORCE_SYNC);
-- 
2.27.0

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Jeff Layton]

On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> From the posix and the initial statx supporting commit comments,
> the AT_STATX_DONT_SYNC is a lightweight stat flag and the
> AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
> the other current usage about these two flags they are all doing
> the same, that is only when the AT_STATX_FORCE_SYNC is not set
> and the AT_STATX_DONT_SYNC is set will they skip sync retriving
> the attributes from storage.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 6788a1f88eb6..1ee6685def83 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
>  		return -ESTALE;
>  
>  	/* Skip the getattr altogether if we're asked not to sync */
> -	if (!(flags & AT_STATX_DONT_SYNC)) {
> +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
>  		err = ceph_do_getattr(inode,
>  				statx_to_caps(request_mask, inode->i_mode),
>  				flags & AT_STATX_FORCE_SYNC);

I don't get it.

The only way I can see that this is a problem is if someone sent down a
mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I
don't see that ignoring FORCE_SYNC would be wrong...

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Xiubo Li]

On 4/18/22 6:15 PM, Jeff Layton wrote:
> On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>>  From the posix and the initial statx supporting commit comments,
>> the AT_STATX_DONT_SYNC is a lightweight stat flag and the
>> AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
>> the other current usage about these two flags they are all doing
>> the same, that is only when the AT_STATX_FORCE_SYNC is not set
>> and the AT_STATX_DONT_SYNC is set will they skip sync retriving
>> the attributes from storage.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/inode.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 6788a1f88eb6..1ee6685def83 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
>>   		return -ESTALE;
>>   
>>   	/* Skip the getattr altogether if we're asked not to sync */
>> -	if (!(flags & AT_STATX_DONT_SYNC)) {
>> +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
>>   		err = ceph_do_getattr(inode,
>>   				statx_to_caps(request_mask, inode->i_mode),
>>   				flags & AT_STATX_FORCE_SYNC);
> I don't get it.
>
> The only way I can see that this is a problem is if someone sent down a
> mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I
> don't see that ignoring FORCE_SYNC would be wrong...
>
There has 3 cases for the flags:

case1: flags & AT_STATX_SYNC_TYPE == 0

case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC

case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | 
AT_STATX_FORCE_SYNC


Only in case2, which is only the DONT_SYNC bit is set, will ignore 
calling ceph_do_getattr() here. And for case3 it will ignore the 
DONT_SYNC bit.

-- Xiubo

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Jeff Layton]

On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote:
> On 4/18/22 6:15 PM, Jeff Layton wrote:
> > On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > > 
> > >  From the posix and the initial statx supporting commit comments,
> > > the AT_STATX_DONT_SYNC is a lightweight stat flag and the
> > > AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
> > > the other current usage about these two flags they are all doing
> > > the same, that is only when the AT_STATX_FORCE_SYNC is not set
> > > and the AT_STATX_DONT_SYNC is set will they skip sync retriving
> > > the attributes from storage.
> > > 
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >   fs/ceph/inode.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 6788a1f88eb6..1ee6685def83 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
> > >   		return -ESTALE;
> > >   
> > >   	/* Skip the getattr altogether if we're asked not to sync */
> > > -	if (!(flags & AT_STATX_DONT_SYNC)) {
> > > +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
> > >   		err = ceph_do_getattr(inode,
> > >   				statx_to_caps(request_mask, inode->i_mode),
> > >   				flags & AT_STATX_FORCE_SYNC);
> > I don't get it.
> > 
> > The only way I can see that this is a problem is if someone sent down a
> > mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I
> > don't see that ignoring FORCE_SYNC would be wrong...
> > 
> There has 3 cases for the flags:
> 
> case1: flags & AT_STATX_SYNC_TYPE == 0
> 
> case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC
> 
> case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC | 
> AT_STATX_FORCE_SYNC
> 
> 
> Only in case2, which is only the DONT_SYNC bit is set, will ignore 
> calling ceph_do_getattr() here. And for case3 it will ignore the 
> DONT_SYNC bit.
> 

Sure, but the patch doesn't functionally change the behavior of the
code. It may make the condition more idiomatic to read, but I don't
think there is a bug here.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Xiubo Li]

On 4/18/22 6:29 PM, Jeff Layton wrote:
> On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote:
>> On 4/18/22 6:15 PM, Jeff Layton wrote:
>>> On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>>   From the posix and the initial statx supporting commit comments,
>>>> the AT_STATX_DONT_SYNC is a lightweight stat flag and the
>>>> AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
>>>> the other current usage about these two flags they are all doing
>>>> the same, that is only when the AT_STATX_FORCE_SYNC is not set
>>>> and the AT_STATX_DONT_SYNC is set will they skip sync retriving
>>>> the attributes from storage.
>>>>
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>    fs/ceph/inode.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index 6788a1f88eb6..1ee6685def83 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
>>>>    		return -ESTALE;
>>>>    
>>>>    	/* Skip the getattr altogether if we're asked not to sync */
>>>> -	if (!(flags & AT_STATX_DONT_SYNC)) {
>>>> +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
>>>>    		err = ceph_do_getattr(inode,
>>>>    				statx_to_caps(request_mask, inode->i_mode),
>>>>    				flags & AT_STATX_FORCE_SYNC);
>>> I don't get it.
>>>
>>> The only way I can see that this is a problem is if someone sent down a
>>> mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I
>>> don't see that ignoring FORCE_SYNC would be wrong...
>>>
>> There has 3 cases for the flags:
>>
>> case1: flags & AT_STATX_SYNC_TYPE == 0
>>
>> case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC
>>
>> case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC |
>> AT_STATX_FORCE_SYNC
>>
>>
>> Only in case2, which is only the DONT_SYNC bit is set, will ignore
>> calling ceph_do_getattr() here. And for case3 it will ignore the
>> DONT_SYNC bit.
>>
> Sure, but the patch doesn't functionally change the behavior of the
> code. It may make the condition more idiomatic to read, but I don't
> think there is a bug here.

-	if (!(flags & AT_STATX_DONT_SYNC)) {


For example, in both case2 and case3 the above condition is false, right 
? That means for case3 it will ignore the FORCE_SYNC always.

+	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {

For exmaple in case2 the above condition is false and then it will skip 
calling the ceph_do_getattr(). And in case3 the above condition is true, 
it will call the ceph_do_getattr(..., flags & FORCE_SYNC).

The logic changed, right ?

-- Xiubo

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Jeff Layton]

On Mon, 2022-04-18 at 18:51 +0800, Xiubo Li wrote:
> On 4/18/22 6:29 PM, Jeff Layton wrote:
> > On Mon, 2022-04-18 at 18:25 +0800, Xiubo Li wrote:
> > > On 4/18/22 6:15 PM, Jeff Layton wrote:
> > > > On Mon, 2022-04-11 at 17:34 +0800, xiubli@redhat.com wrote:
> > > > > From: Xiubo Li <xiubli@redhat.com>
> > > > > 
> > > > >   From the posix and the initial statx supporting commit comments,
> > > > > the AT_STATX_DONT_SYNC is a lightweight stat flag and the
> > > > > AT_STATX_FORCE_SYNC is a heaverweight one. And also checked all
> > > > > the other current usage about these two flags they are all doing
> > > > > the same, that is only when the AT_STATX_FORCE_SYNC is not set
> > > > > and the AT_STATX_DONT_SYNC is set will they skip sync retriving
> > > > > the attributes from storage.
> > > > > 
> > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > > > ---
> > > > >    fs/ceph/inode.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > index 6788a1f88eb6..1ee6685def83 100644
> > > > > --- a/fs/ceph/inode.c
> > > > > +++ b/fs/ceph/inode.c
> > > > > @@ -2887,7 +2887,7 @@ int ceph_getattr(struct user_namespace *mnt_userns, const struct path *path,
> > > > >    		return -ESTALE;
> > > > >    
> > > > >    	/* Skip the getattr altogether if we're asked not to sync */
> > > > > -	if (!(flags & AT_STATX_DONT_SYNC)) {
> > > > > +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
> > > > >    		err = ceph_do_getattr(inode,
> > > > >    				statx_to_caps(request_mask, inode->i_mode),
> > > > >    				flags & AT_STATX_FORCE_SYNC);
> > > > I don't get it.
> > > > 
> > > > The only way I can see that this is a problem is if someone sent down a
> > > > mask with both DONT_SYNC and FORCE_SYNC set in it, and in that case I
> > > > don't see that ignoring FORCE_SYNC would be wrong...
> > > > 
> > > There has 3 cases for the flags:
> > > 
> > > case1: flags & AT_STATX_SYNC_TYPE == 0
> > > 
> > > case2: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC
> > > 
> > > case3: flags & AT_STATX_SYNC_TYPE == AT_STATX_DONT_SYNC |
> > > AT_STATX_FORCE_SYNC
> > > 
> > > 
> > > Only in case2, which is only the DONT_SYNC bit is set, will ignore
> > > calling ceph_do_getattr() here. And for case3 it will ignore the
> > > DONT_SYNC bit.
> > > 
> > Sure, but the patch doesn't functionally change the behavior of the
> > code. It may make the condition more idiomatic to read, but I don't
> > think there is a bug here.
> 
> -	if (!(flags & AT_STATX_DONT_SYNC)) {
> 
> 
> For example, in both case2 and case3 the above condition is false, right 
> ? That means for case3 it will ignore the FORCE_SYNC always.
> 
> +	if ((flags & AT_STATX_SYNC_TYPE) != AT_STATX_DONT_SYNC) {
> 
> For exmaple in case2 the above condition is false and then it will skip 
> calling the ceph_do_getattr(). And in case3 the above condition is true, 
> it will call the ceph_do_getattr(..., flags & FORCE_SYNC).
> 
> The logic changed, right ?
> 

Yes, but my argument is that sending down a mask that has
AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC makes no sense whatsoever. You're
simultaneously asking it to not fetch attributes and to forcibly fetch
attributes. We're within our rights to just ignore FORCE_SYNC at that
point.

Arguably, this ought to be caught in vfs_statx before we ever call down
into the fs driver with something like:

    if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))
	    return -EINVAL;

David, should we add something like this, or is this too risky a change?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[David Howells]

Jeff Layton <jlayton@kernel.org> wrote:

>     if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))

You can't do that.  DONT_SYNC and FORCE_SYNC aren't bit flags - they're an
enumeration in a bit field.  There's a reserved value at 0x6000 that doesn't
have a symbol assigned.

David

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Jeff Layton]

On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote:
> Jeff Layton <jlayton@kernel.org> wrote:
> 
> >     if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))
> 
> You can't do that.  DONT_SYNC and FORCE_SYNC aren't bit flags - they're an
> enumeration in a bit field.  There's a reserved value at 0x6000 that doesn't
> have a symbol assigned.
> 

Right, but nothing prevents you from setting both flags at the same
time. How should we interpret such a request?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[David Howells]

Jeff Layton <jlayton@kernel.org> wrote:

> On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote:
> > Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > >     if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))
> > 
> > You can't do that.  DONT_SYNC and FORCE_SYNC aren't bit flags - they're an
> > enumeration in a bit field.  There's a reserved value at 0x6000 that doesn't
> > have a symbol assigned.
> > 
> 
> Right, but nothing prevents you from setting both flags at the same
> time. How should we interpret such a request?

A good question without a necessarily right answer.

Possibly we should do:

 #define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
 #define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
 #define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
 #define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
+#define AT_STATX_SYNC_RESERVED	0x6000

and give EINVAL if we see the reserved value.  But also these values can be
considered a hint, so possibly we should just ignore the reserved value.  Oh
for fsinfo()...

David

Re: [RFC resend PATCH] ceph: fix statx AT_STATX_DONT_SYNC vs AT_STATX_FORCE_SYNC check

[Jeff Layton]

On Tue, 2022-04-19 at 14:30 +0100, David Howells wrote:
> Jeff Layton <jlayton@kernel.org> wrote:
> 
> > On Tue, 2022-04-19 at 14:22 +0100, David Howells wrote:
> > > Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > >     if ((flags & AT_STATX_SYNC_TYPE) == (AT_STATX_DONT_SYNC|AT_STATX_FORCE_SYNC))
> > > 
> > > You can't do that.  DONT_SYNC and FORCE_SYNC aren't bit flags - they're an
> > > enumeration in a bit field.  There's a reserved value at 0x6000 that doesn't
> > > have a symbol assigned.
> > > 
> > 
> > Right, but nothing prevents you from setting both flags at the same
> > time. How should we interpret such a request?
> 
> A good question without a necessarily right answer.
> 
> Possibly we should do:
> 
>  #define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
>  #define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
>  #define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
>  #define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
> +#define AT_STATX_SYNC_RESERVED	0x6000
> 
> and give EINVAL if we see the reserved value.  But also these values can be
> considered a hint, so possibly we should just ignore the reserved value.  Oh
> for fsinfo()...
> 
> David
> 

That was what the code (pre-patch) did. If someone set
DONT_SYNC|FORCE_SYNC, it would just ignore FORCE_SYNC. It's not ideal,
but I suppose we're within our rights to prefer either behaviour in that
case if someone sets both flags.

In hindsight, setting both should have probably caused the syscall to
throw back -EINVAL, but changing that now is probably a bit dangerous.
-- 
Jeff Layton <jlayton@kernel.org>