linux-kernel - Re: [PATCH v3 09/21] xfs: Do not free EOF blocks for forcealign

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: Thu, 2 May 2024 09:55:44 +0100
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: djwong@...nel.org, hch@....de, viro@...iv.linux.org.uk, brauner@...nel.org,
        jack@...e.cz, chandan.babu@...cle.com, willy@...radead.org,
        axboe@...nel.dk, martin.petersen@...cle.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        tytso@....edu, jbongio@...gle.com, ojaswin@...ux.ibm.com,
        ritesh.list@...il.com, mcgrof@...nel.org, p.raghav@...sung.com,
        linux-xfs@...r.kernel.org, catherine.hoang@...cle.com
Subject: Re: [PATCH v3 09/21] xfs: Do not free EOF blocks for forcealign

On 02/05/2024 02:11, Dave Chinner wrote:
>>> static inline bool
>>> xfs_inode_has_forcealign(struct xfs_inode *ip)
>>> {
>>> 	if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
>>> 		return false;
>>> 	if (ip->i_extsize <= 1)
>>> 		return false;
>>>
>>> 	if (xfs_is_cow_inode(ip))
>>> 		return false;
>> Could we just include this in the forcealign validate checks? Currently we
>> just check CoW extsize is zero there.
> Checking COW extsize is zero doesn't tell us anything useful about
> whether the inode might have shared extents, or that the filesystem
> has had the sysfs "always cow" debug knob turned on. That changes
> filesystem behaviour at mount time and has nothing to do with the
> on-disk format constraints.
> 
> And now that I think about it, checking for COW extsize is
> completely the wrong thing to do because it doesn't get used until
> an extent is shared and a COW trigger is hit. So the presence of COW
> extsize has zero impact on whether we can use forced alignment or
> not.

ok

> 
> IOWs, we have to check for shared extents or always cow here,
> because even a file with correctly set up forced alignment needs to
> have forced alignment disabled when always_cow is enabled. Every
> write is going to use the COW path and AFAICT we don't support
> forced alignment through that path yet.

ok

> 
>>> 	if (ip->di_flags & XFS_DIFLAG_REALTIME)
>>> 		return false;
>> We check this in xfs_inode_validate_forcealign()
> That's kinda my point - we have a random smattering of different
> checks at different layers and in different contexts. i.e.  There's
> no one function that performs -all- the "can we do forced alignment"
> checks that allow forced alignment to be used. This simply adds all
> those checks in the one place and ensures that even if other code
> gets checks wrong, we won't use forcealign inappropriately.

Fine, I can do that if you think it is the best strategy.

Thanks,
John