lkml.org 
[lkml]   [2020]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl
> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
> if (!(filp->f_mode & FMODE_WRITE))
> return -EBADF;
>
> file_start_write(filp);
> inode_lock(inode);
> ...
> inode_unlock(inode);
> file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?

Yep, this looks more accurate.

> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file. It just guarantees that there aren't any other
> writable file descriptors. (Actually, file descriptions.) Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.

Yep, I agree this is not a proper way. I think we don't need this
check here, because
compress routine doesn't compress any file data when it detects the
file is mmaped
using f2fs_is_mmap_file().

Thanks~


2020년 10월 15일 (목) 오후 1:04, Eric Biggers <ebiggers@kernel.org>님이 작성:
>
> On Wed, Oct 14, 2020 at 11:27:30AM +0900, Daeho Jeong wrote:
> > > f2fs_readonly() is redundant with mnt_want_write_file().
> > >
> > > Also, shouldn't this require a writable file descriptor? As-is, this ioctl can
> > > be called on a file owned by another user, as long as the caller has read
> > > access.
> > >
> > > Note: if you change this to require a writable file descriptor, then
> > > f2fs_readonly(), mnt_want_write_file(), and IS_IMMUTABLE() all would no longer
> > > be needed.
> >
> > I agree that f2fs_readonly() is redundant.
> > But, sorry, I don't get the rest. I thought mnt_want_write_file() is a
> > way to check whether the caller has a proper write permission or not.
> > I think just using mnt_want_write_file() is enough for this ioctl. Am
> > I missing something?
>
> mnt_want_write_file() checks for write permission to the mount, not to the file.
>
> I think this ioctl wants what f2fs_sec_trim_file() does:
>
> if (!(filp->f_mode & FMODE_WRITE))
> return -EBADF;
>
> file_start_write(filp);
> inode_lock(inode);
> ...
> inode_unlock(inode);
> file_end_write(filp);
>
>
> After all you shouldn't be able to change the compression options of a file
> given only read access to it, right?
>
> > > I don't think the check for i_writecount == 1 accomplishes anything because it
> > > just means there are no *other* writable file descriptors. It doesn't mean that
> > > some other thread isn't concurrently trying to write to this same file
> > > descriptor. So the lock needs to be enough. Is it?
> >
> > This is to detect any possibility of other threads mmap-ing and
> > writing the file.
> > Using only inode lock is not enough to prevent them from making dirty pages.
>
> Well, as I said, i_writecount == 1 doesn't guarantee that other threads aren't
> mmap'ing or writing to the file. It just guarantees that there aren't any other
> writable file descriptors. (Actually, file descriptions.) Multiple threads can
> be using the same file descriptor (or the same file description) concurrently.
>
> - Eric

\
 
 \ /
  Last update: 2020-10-16 04:24    [W:1.474 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site