* [PATCH] ceph: update the time stamps and try to drop the suid/sgid
@ 2023-02-13 11:10 xiubli
2023-02-13 12:37 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: xiubli @ 2023-02-13 11:10 UTC (permalink / raw)
To: idryomov; +Cc: jlayton, vshankar, mchangir, Xiubo Li, stable
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.
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: update the time stamps and try to drop the suid/sgid
2023-02-13 11:10 [PATCH] ceph: update the time stamps and try to drop the suid/sgid xiubli
@ 2023-02-13 12:37 ` Jeff Layton
2023-02-13 12:59 ` Xiubo Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2023-02-13 12:37 UTC (permalink / raw)
To: xiubli, idryomov; +Cc: vshankar, mchangir, stable
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.
> 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: update the time stamps and try to drop the suid/sgid
2023-02-13 12:37 ` Jeff Layton
@ 2023-02-13 12:59 ` Xiubo Li
2023-02-13 13:09 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Xiubo Li @ 2023-02-13 12:59 UTC (permalink / raw)
To: Jeff Layton, idryomov, Ceph Development; +Cc: vshankar, mchangir, stable
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.
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: update the time stamps and try to drop the suid/sgid
2023-02-13 12:59 ` Xiubo Li
@ 2023-02-13 13:09 ` Jeff Layton
2023-02-13 13:21 ` Xiubo Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2023-02-13 13:09 UTC (permalink / raw)
To: Xiubo Li, idryomov, Ceph Development; +Cc: vshankar, mchangir, stable
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.
> 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>
> >
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ceph: update the time stamps and try to drop the suid/sgid
2023-02-13 13:09 ` Jeff Layton
@ 2023-02-13 13:21 ` Xiubo Li
0 siblings, 0 replies; 6+ messages in thread
From: Xiubo Li @ 2023-02-13 13:21 UTC (permalink / raw)
To: Jeff Layton, idryomov, Ceph Development; +Cc: vshankar, mchangir, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ceph: update the time stamps and try to drop the suid/sgid
@ 2023-02-13 11:28 xiubli
0 siblings, 0 replies; 6+ messages in thread
From: xiubli @ 2023-02-13 11:28 UTC (permalink / raw)
To: idryomov, ceph-devel; +Cc: jlayton, vshankar, mchangir, Xiubo Li, stable
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.
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-13 13:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-13 11:10 [PATCH] ceph: update the time stamps and try to drop the suid/sgid xiubli
2023-02-13 12:37 ` Jeff Layton
2023-02-13 12:59 ` Xiubo Li
2023-02-13 13:09 ` Jeff Layton
2023-02-13 13:21 ` Xiubo Li
2023-02-13 11:28 xiubli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.