Re: [PATCH] ceph: update the time stamps and try to drop the suid/sgid

[Xiubo Li <xiubli@redhat.com>]

On 13/02/2023 21:09, Jeff Layton wrote:
> On Mon, 2023-02-13 at 20:59 +0800, Xiubo Li wrote:
>> On 13/02/2023 20:37, Jeff Layton wrote:
>>> On Mon, 2023-02-13 at 19:10 +0800, xiubli@redhat.com wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>> The fallocate will try to clear the suid/sgid if a unprevileged user
>>>> changed the file.
>>>>
>>>> There is no Posix item requires that we should clear the suid/sgid
>>>> in fallocate code path but this is the default behaviour for most of
>>>> the filesystems and the VFS layer. And also the same for the write
>>>> code path, which have already support it.
>>>>
>>> Huh, you're right. It really doesn't say anything about the timestamps
>>> or setuid bits:
>>>
>>>       https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
>>>
>>>
>>> That's arguably a bug in the spec. It really does need to do those
>>> things.
>> Yeah.
>>
> Actually, posix_fallocate doesn't do hole punching. It just ensures that
> blocks are reserved to back future writes. Given that that's not
> something "observable" and won't change the contents of the file, then
> there really is no need to change the times and clear set{u,g}id bits
> there.
>
> Linux' fallocate is different. It's a GNU API not covered by POSIX, and
> can result in an observable change to the contents of the file. There,
> we _must_ clear the setuid/setgid bits and update timestamps, at least
> in the cases where the content can change.
>
Okay, get it.

Thank you Jeff for detailed explaining about this.

- Xiubo


>> Also the kernel fuse code and libfuse also need to be improved to make
>> ceph-fuse work.
>>
>> Thanks Jeff.
>>
>> - Xiubo
>>
>>>> And also we need to update the time stamps since the fallocate will
>>>> change the file contents.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> URL: https://tracker.ceph.com/issues/58054
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>    fs/ceph/file.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>>> index 903de296f0d3..dee3b445f415 100644
>>>> --- a/fs/ceph/file.c
>>>> +++ b/fs/ceph/file.c
>>>> @@ -2502,6 +2502,9 @@ static long ceph_fallocate(struct file *file, int mode,
>>>>    	loff_t endoff = 0;
>>>>    	loff_t size;
>>>>    
>>>> +	dout("%s %p %llx.%llx mode %x, offset %llu length %llu\n", __func__,
>>>> +	     inode, ceph_vinop(inode), mode, offset, length);
>>>> +
>>>>    	if (mode != (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> @@ -2539,6 +2542,10 @@ static long ceph_fallocate(struct file *file, int mode,
>>>>    	if (ret < 0)
>>>>    		goto unlock;
>>>>    
>>>> +	ret = file_modified(file);
>>>> +	if (ret)
>>>> +		goto put_caps;
>>>> +
>>>>    	filemap_invalidate_lock(inode->i_mapping);
>>>>    	ceph_fscache_invalidate(inode, false);
>>>>    	ceph_zero_pagecache_range(inode, offset, length);
>>>> @@ -2554,6 +2561,7 @@ static long ceph_fallocate(struct file *file, int mode,
>>>>    	}
>>>>    	filemap_invalidate_unlock(inode->i_mapping);
>>>>    
>>>> +put_caps:
>>>>    	ceph_put_cap_refs(ci, got);
>>>>    unlock:
>>>>    	inode_unlock(inode);
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>
-- 
Best Regards,

Xiubo Li (李秀波)

Email: xiubli@redhat.com/xiubli@ibm.com
Slack: @Xiubo Li