Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201013022429.454161-1-daeho43@gmail.com> <20201013022429.454161-2-daeho43@gmail.com>
 <20201013061150.GC1062@sol.localdomain> <CACOAw_y31yAu=AGAEqvyo2Ankt-ux80E6g6m_sWnz6LyUgBXSg@mail.gmail.com>
 <20201015040441.GA834@sol.localdomain>
In-Reply-To: <20201015040441.GA834@sol.localdomain>
From:   Daeho Jeong <daeho43@gmail.com>
Date:   Fri, 16 Oct 2020 11:24:06 +0900
Message-ID: <CACOAw_wNWw2bcZy5kg0hoo1GQhrst6bEe87Dp4Q_uR+p7ZBkyA@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH 2/2] f2fs: add F2FS_IOC_SET_COMPRESS_OPTION ioctl
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com,
        Daeho Jeong <daehojeong@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> mnt_want_write_file() checks for write permission to the mount, not to th=
e 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 fi=
le
> given only read access to it, right?

Yep, this looks more accurate.

> Well, as I said, i_writecount =3D=3D 1 doesn't guarantee that other threa=
ds aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't an=
y other
> writable file descriptors.  (Actually, file descriptions.)  Multiple thre=
ads can
> be using the same file descriptor (or the same file description) concurre=
ntly.

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=EB=85=84 10=EC=9B=94 15=EC=9D=BC (=EB=AA=A9) =EC=98=A4=ED=9B=84 1:04, =
Eric Biggers <ebiggers@kernel.org>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> 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 th=
e 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 fi=
le
> given only read access to it, right?
>
> > > I don't think the check for i_writecount =3D=3D 1 accomplishes anythi=
ng 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 fil=
e
> > > 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 p=
ages.
>
> Well, as I said, i_writecount =3D=3D 1 doesn't guarantee that other threa=
ds aren't
> mmap'ing or writing to the file.  It just guarantees that there aren't an=
y other
> writable file descriptors.  (Actually, file descriptions.)  Multiple thre=
ads can
> be using the same file descriptor (or the same file description) concurre=
ntly.
>
> - Eric