linux-kernel - Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Aug 2017 13:55:29 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Jan Kara <jack@...e.cz>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        Dave Chinner <david@...morbit.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-xfs@...r.kernel.org, Jeff Moyer <jmoyer@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in
 response to fallocate

On Fri, Aug 4, 2017 at 1:53 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> On Fri, Aug 04, 2017 at 01:47:32PM -0700, Dan Williams wrote:
>> On Fri, Aug 4, 2017 at 1:14 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
>> > On Thu, Aug 03, 2017 at 07:28:35PM -0700, Dan Williams wrote:
>> >> After validating the state of the file as not having holes, shared
>> >> extents, or active mappings try to commit the
>> >> XFS_DIFLAG2_IOMAP_IMMUTABLE flag to the on-disk inode metadata. If that
>> >> succeeds then allow the S_IOMAP_IMMUTABLE to be set on the vfs inode.
>> >>
>> >> Cc: Jan Kara <jack@...e.cz>
>> >> Cc: Jeff Moyer <jmoyer@...hat.com>
>> >> Cc: Christoph Hellwig <hch@....de>
>> >> Cc: Ross Zwisler <ross.zwisler@...ux.intel.com>
>> >> Suggested-by: Dave Chinner <david@...morbit.com>
>> >> Suggested-by: "Darrick J. Wong" <darrick.wong@...cle.com>
>> >> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>> >> ---
>> >>  fs/xfs/xfs_bmap_util.c |   32 ++++++++++++++++++++++++++++++++
>> >>  1 file changed, 32 insertions(+)
>> >>
>> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> >> index 70ac2d33ab27..8464c25a2403 100644
>> >> --- a/fs/xfs/xfs_bmap_util.c
>> >> +++ b/fs/xfs/xfs_bmap_util.c
>> >> @@ -1436,9 +1436,11 @@ xfs_seal_file_space(
>> >>       xfs_off_t               offset,
>> >>       xfs_off_t               len)
>> >>  {
>> >> +     struct xfs_mount        *mp = ip->i_mount;
>> >>       struct inode            *inode = VFS_I(ip);
>> >>       struct address_space    *mapping = inode->i_mapping;
>> >>       int                     error;
>> >> +     struct xfs_trans        *tp;
>> >>
>> >>       ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>> >>
>> >> @@ -1454,6 +1456,10 @@ xfs_seal_file_space(
>> >>       if (error)
>> >>               return error;
>> >>
>> >> +     error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
>> >> +     if (error)
>> >> +             return error;
>> >> +
>> >>       xfs_ilock(ip, XFS_ILOCK_EXCL);
>> >>       /*
>> >>        * Either the size changed after we performed allocation /
>> >> @@ -1486,10 +1492,20 @@ xfs_seal_file_space(
>> >>       if (error < 0)
>> >>               goto out_unlock;
>> >>
>> >> +     xfs_trans_ijoin(tp, ip, 0);
>> >
>> > FWIW if you change that third parameter to XFS_ILOCK_EXCL then
>> > xfs_trans_commit will do the xfs_iunlock(ip, XFS_ILOCK_EXCL) for you if
>> > the commit succeeds...
>> >
>> >> +     ip->i_d.di_flags2 |= XFS_DIFLAG2_IOMAP_IMMUTABLE;
>> >> +     xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> >> +     error = xfs_trans_commit(tp);
>> >> +     tp = NULL; /* nothing to cancel */
>> >> +     if (error)
>> >> +             goto out_unlock;
>> >> +
>> >>       inode->i_flags |= S_IOMAP_IMMUTABLE;
>> >
>> > ...and then you can just return out here.
>>
>> Do we not need to hold XFS_ILOCK_EXCL over ->i_flags changes, or is
>> XFS_IOLOCK_EXCL enough?
>
> Oh, heh, I missed a piece.  Set the flag before the transaction commit,
> because if the commit fails the fs is shut down anyway. :)

Ah, got it, thanks.