From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Dave Chinner <david@fromorbit.com>,
linux-xfs <linux-xfs@vger.kernel.org>,
Linux MM <linux-mm@kvack.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Ext4 <linux-ext4@vger.kernel.org>,
Linux Btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v2 4/4] xfs: don't allow most setxattr to immutable files
Date: Wed, 17 Apr 2019 12:01:24 -0700 [thread overview]
Message-ID: <20190417190124.GA5057@magnolia> (raw)
In-Reply-To: <CAOQ4uxgDQHJntoO6EZ1fn-iBVo8gshsSpHd_UB1cnXUJ3CXOTg@mail.gmail.com>
On Tue, Apr 09, 2019 at 11:24:10AM +0300, Amir Goldstein wrote:
> On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > The chattr manpage has this to say about immutable files:
> >
> > "A file with the 'i' attribute cannot be modified: it cannot be deleted
> > or renamed, no link can be created to this file, most of the file's
> > metadata can not be modified, and the file can not be opened in write
> > mode."
> >
> > However, we don't actually check the immutable flag in the setattr code,
> > which means that we can update project ids and extent size hints on
> > supposedly immutable files. Therefore, reject a setattr call on an
> > immutable file except for the case where we're trying to unset
> > IMMUTABLE.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/xfs_ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 5a1b96dad901..67d12027f563 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1023,6 +1023,40 @@ xfs_ioctl_setattr_flush(
> > return filemap_write_and_wait(inode->i_mapping);
> > }
> >
> > +/*
> > + * If immutable is set and we are not clearing it, we're not allowed to change
> > + * anything else in the inode.
>
> This looks correct, but FYI, neither xfs_io nor chattr clears 'immutable'
> and sets projid/*extsize in one ioctl/xfsctl, so there is no justification to
> making an extra effort to support that use case. You could do with
> checking 'immutable' inside xfs_ioctl_setattr_check_projid/*extsize()
> and leave only the di_flags check here.
However, the API does allow callers to clear immutable and set other
fields in one go, so just because xfs_io won't do it doesn't mean we can
ignore it.
Then again I guess the manpage doesn't explicitly say what the behavior
is supposed to be, so I guess we'll just ... argh, fine I'll go fix the
manpage to document the behavior.
--D
> Some would say that will be cleaner code.
> Its a matter of taste and its your subsystem, so feel free to dismiss
> this comments.
>
> Thanks,
> Amir.
>
> > Don't error out if we're only trying to set
> > + * immutable on an immutable file.
> > + */
> > +static int
> > +xfs_ioctl_setattr_immutable(
> > + struct xfs_inode *ip,
> > + struct fsxattr *fa,
> > + uint16_t di_flags,
> > + uint64_t di_flags2)
> > +{
> > + struct xfs_mount *mp = ip->i_mount;
> > +
> > + if (!(ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE) ||
> > + !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE))
> > + return 0;
> > +
> > + if ((ip->i_d.di_flags & ~XFS_DIFLAG_IMMUTABLE) !=
> > + (di_flags & ~XFS_DIFLAG_IMMUTABLE))
> > + return -EPERM;
> > + if (ip->i_d.di_version >= 3 && ip->i_d.di_flags2 != di_flags2)
> > + return -EPERM;
> > + if (xfs_get_projid(ip) != fa->fsx_projid)
> > + return -EPERM;
> > + if (ip->i_d.di_extsize != fa->fsx_extsize >> mp->m_sb.sb_blocklog)
> > + return -EPERM;
> > + if (ip->i_d.di_version >= 3 && (di_flags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> > + ip->i_d.di_cowextsize != fa->fsx_cowextsize >> mp->m_sb.sb_blocklog)
> > + return -EPERM;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > xfs_ioctl_setattr_xflags(
> > struct xfs_trans *tp,
> > @@ -1030,7 +1064,9 @@ xfs_ioctl_setattr_xflags(
> > struct fsxattr *fa)
> > {
> > struct xfs_mount *mp = ip->i_mount;
> > + uint16_t di_flags;
> > uint64_t di_flags2;
> > + int error;
> >
> > /* Can't change realtime flag if any extents are allocated. */
> > if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> > @@ -1061,12 +1097,18 @@ xfs_ioctl_setattr_xflags(
> > !capable(CAP_LINUX_IMMUTABLE))
> > return -EPERM;
> >
> > - /* diflags2 only valid for v3 inodes. */
> > + /* Don't allow changes to an immutable inode. */
> > + di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> > di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> > + error = xfs_ioctl_setattr_immutable(ip, fa, di_flags, di_flags2);
> > + if (error)
> > + return error;
> > +
> > + /* diflags2 only valid for v3 inodes. */
> > if (di_flags2 && ip->i_d.di_version < 3)
> > return -EINVAL;
> >
> > - ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> > + ip->i_d.di_flags = di_flags;
> > ip->i_d.di_flags2 = di_flags2;
> >
> > xfs_diflags_to_linux(ip);
prev parent reply other threads:[~2019-04-17 19:01 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-07 20:27 [PATCH v2 0/4] vfs: make immutable files actually immutable Darrick J. Wong
2019-04-07 20:27 ` [PATCH 1/4] mm/fs: don't allow writes to immutable files Darrick J. Wong
2019-04-07 20:27 ` [PATCH 2/4] xfs: unlock inode when xfs_ioctl_setattr_get_trans can't get transaction Darrick J. Wong
2019-04-08 5:48 ` Allison Henderson
2019-04-07 20:27 ` [PATCH 3/4] xfs: flush page mappings as part of setting immutable Darrick J. Wong
2019-04-08 5:49 ` Allison Henderson
2019-04-07 20:27 ` [PATCH 4/4] xfs: don't allow most setxattr to immutable files Darrick J. Wong
2019-04-08 5:57 ` Allison Henderson
2019-04-08 6:20 ` Amir Goldstein
2019-04-09 3:18 ` Darrick J. Wong
2019-04-09 3:19 ` [PATCH v2 " Darrick J. Wong
2019-04-09 8:24 ` Amir Goldstein
2019-04-17 19:01 ` Darrick J. Wong [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190417190124.GA5057@magnolia \
--to=darrick.wong@oracle.com \
--cc=amir73il@gmail.com \
--cc=david@fromorbit.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).