Re: [PATCH 2/3] xfs: restrict the h_size fixup in xlog_do_recovery_pass

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Apr 29, 2024 at 09:01:59AM +0200, Christoph Hellwig wrote:
> The reflink and rmap features require a fixed xfsprogs, so don't allow
> this fixup for them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log_recover.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index bb8957927c3c2e..d73bec65f93b46 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3040,10 +3040,14 @@ xlog_do_recovery_pass(
>  		 * Detect this condition here. Use lsunit for the buffer size as
>  		 * long as this looks like the mkfs case. Otherwise, return an
>  		 * error to avoid a buffer overrun.
> +		 *
> +		 * Reject the invalid size if the file system has new enough
> +		 * features that require a fixed mkfs.
>  		 */
>  		h_size = be32_to_cpu(rhead->h_size);
>  		h_len = be32_to_cpu(rhead->h_len);
> -		if (h_len > h_size && h_len <= log->l_mp->m_logbsize &&
> +		if (!xfs_has_reflink(log->l_mp) && xfs_has_reflink(log->l_mp) &&
> +		    h_len > h_size && h_len <= log->l_mp->m_logbsize &&
>  		    rhead->h_num_logops == cpu_to_be32(1)) {

Same comment about do you want to test for rmap and reflink here?

I also wonder if this multiline predicate should turn into a static
inline helper.  I nearly wrote you one, but then I realize that I don't
remember enough about the xfsprogs problem to know if the problem was
limited to mkfs, or if xfs_repair zeroing the log would also write out a
bad h_size?

--D

>  			xfs_warn(log->l_mp,
>  		"invalid iclog size (%d bytes), using lsunit (%d bytes)",
> -- 
> 2.39.2
> 
>