| Message ID | 20241122122931.90408-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] fs: require inode_owner_or_capable for F_SET_RW_HINT | expand |
On Fri 22-11-24 13:29:25, Christoph Hellwig wrote: > F_SET_RW_HINT controls the placement of written file data. A read-only > fd should not allow for that. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Here I'm not so sure. Firstly, since you are an owner this doesn't add any additional practical restriction. Secondly, you are not changing anything on disk, just IO hints in memory... Thirdly, we generally don't require writeable fd even to do file attribute changes (like with fchmod, fchown, etc.). So although the check makes some sense, it seems to be mostly inconsistent with how we treat similar stuff. Honza > --- > fs/fcntl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/fcntl.c b/fs/fcntl.c > index 7fc6190da342..12f60c42743a 100644 > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -377,6 +377,8 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd, > > if (!inode_owner_or_capable(file_mnt_idmap(file), inode)) > return -EPERM; > + if (!(file->f_mode & FMODE_WRITE)) > + return -EBADF; > > if (copy_from_user(&hint, argp, sizeof(hint))) > return -EFAULT; > -- > 2.45.2 >
On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote: > Here I'm not so sure. Firstly, since you are an owner this doesn't add any > additional practical restriction. Secondly, you are not changing anything > on disk, just IO hints in memory... Thirdly, we generally don't require > writeable fd even to do file attribute changes (like with fchmod, fchown, > etc.). So although the check makes some sense, it seems to be mostly > inconsistent with how we treat similar stuff. As I said I'm not quite convince either, so just doing the first one is probably fine.
On Fri, Nov 22, 2024 at 05:15:47PM +0100, Christoph Hellwig wrote: > On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote: > > Here I'm not so sure. Firstly, since you are an owner this doesn't add any > > additional practical restriction. Secondly, you are not changing anything > > on disk, just IO hints in memory... Thirdly, we generally don't require > > writeable fd even to do file attribute changes (like with fchmod, fchown, > > etc.). So although the check makes some sense, it seems to be mostly > > inconsistent with how we treat similar stuff. > > As I said I'm not quite convince either, so just doing the first one > is probably fine. We do require FMODE_WRITE to do a dedupe, which isn't exactly the same but is similar in concept (we're not changing the content of the file; we're changing how it's laid out on storage). So I think it's reasonable to require FMODE_WRITE to set the write hints.
On Fri, Nov 22, 2024 at 04:40:31PM +0000, Matthew Wilcox wrote: > On Fri, Nov 22, 2024 at 05:15:47PM +0100, Christoph Hellwig wrote: > > On Fri, Nov 22, 2024 at 01:53:42PM +0100, Jan Kara wrote: > > > Here I'm not so sure. Firstly, since you are an owner this doesn't add any > > > additional practical restriction. Secondly, you are not changing anything > > > on disk, just IO hints in memory... Thirdly, we generally don't require > > > writeable fd even to do file attribute changes (like with fchmod, fchown, > > > etc.). So although the check makes some sense, it seems to be mostly > > > inconsistent with how we treat similar stuff. > > > > As I said I'm not quite convince either, so just doing the first one > > is probably fine. > > We do require FMODE_WRITE to do a dedupe, which isn't exactly the same > but is similar in concept (we're not changing the content of the file; > we're changing how it's laid out on storage). So I think it's reasonable > to require FMODE_WRITE to set the write hints. I tend to agree with Jan. I think the dedupe case is different because you actually use the file to perform a write(-like) operation whereas toggling write hints is just setting an option.
diff --git a/fs/fcntl.c b/fs/fcntl.c index 7fc6190da342..12f60c42743a 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -377,6 +377,8 @@ static long fcntl_set_rw_hint(struct file *file, unsigned int cmd, if (!inode_owner_or_capable(file_mnt_idmap(file), inode)) return -EPERM; + if (!(file->f_mode & FMODE_WRITE)) + return -EBADF; if (copy_from_user(&hint, argp, sizeof(hint))) return -EFAULT;
F_SET_RW_HINT controls the placement of written file data. A read-only fd should not allow for that. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/fcntl.c | 2 ++ 1 file changed, 2 insertions(+)