Messages in this thread | | | From | Daeho Jeong <> | Date | Fri, 16 Oct 2020 11:24:06 +0900 | Subject | Re: [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
| |