Error in generic/397 test script?

Error in generic/397 test script?

[David Howells]

Hi Eric,

In the generic/397 test script, you placed:

	$XFS_IO_PROG -f $SCRATCH_MNT/edir/newfile |& _filter_scratch
	$XFS_IO_PROG -f $SCRATCH_MNT/edir/0123456789abcdef |& _filter_scratch

but neither of those lines actually has a command on it, and when I run it,
I'm seeing xfs_io hang just waiting endlessly for someone to type commands on
stdin.

Would it be better to do:

	echo >$SCRATCH_MNT/edir/newfile |& _filter_scratch
	echo >$SCRATCH_MNT/edir/0123456789abcdef |& _filter_scratch

instead?

David?

Re: Error in generic/397 test script?

[David Howells]

generic/429 can also hang:


	show_file_contents()
	{
		echo "--- Contents of files using plaintext names:"
		cat $SCRATCH_MNT/edir/@@@ |& _filter_scratch
		cat $SCRATCH_MNT/edir/abcd |& _filter_scratch
		echo "--- Contents of files using no-key names:"
		cat ${nokey_names[@]} |& _filter_scratch | _filter_nokey_filenames edir
	}
	...
	nokey_names=( $(find $SCRATCH_MNT/edir -mindepth 1 | sort) )
	printf '%s\n' "${nokey_names[@]}" | \
		_filter_scratch | _filter_nokey_filenames edir
	show_file_contents

on the 'cat ...' at the end of show_file_contents().  A check that
${nokey_names[0]} is not nothing might be in order.

However, in this case (in which I'm running these against ceph), I don't think
that the find should return nothing, so it's not a bug in the test script per
se.

David

Re: Error in generic/397 test script?

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> However, in this case (in which I'm running these against ceph), I don't think
> that the find should return nothing, so it's not a bug in the test script per
> se.

Turned out that I hadn't enabled XTS and so the tests for xts(aes) failed and
produced no files.

David

Re: Error in generic/397 test script?

[Eric Biggers]

On Mon, Jan 20, 2025 at 03:43:46PM +0000, David Howells wrote:
> David Howells <dhowells@redhat.com> wrote:
> 
> > However, in this case (in which I'm running these against ceph), I don't think
> > that the find should return nothing, so it's not a bug in the test script per
> > se.
> 
> Turned out that I hadn't enabled XTS and so the tests for xts(aes) failed and
> produced no files.
> 
> David

AES-XTS support is mandatory for fscrypt, as is documented in fscrypt.rst.  The
filesystems that support fscrypt select FS_ENCRYPTION_ALGS, which has "imply
CRYPTO_XTS" and "imply CRYPTO_AES".  As explained in the comment just above it,
the reason that it's "imply" rather than "select" is so people can disable the
generic implementations of these algorithms if they know that an optimized
implementation will always be available.

It would be enlightening to understand what the issue was here.  Did you
explicitly disable these options, overriding the imply, without providing a
replacement?  Or was this another issue specific to unmerged kernel patches?

- Eric

Re: Error in generic/397 test script?

[David Howells]

Eric Biggers <ebiggers@kernel.org> wrote:

> It would be enlightening to understand what the issue was here.  Did you
> explicitly disable these options, overriding the imply, without providing a
> replacement?  Or was this another issue specific to unmerged kernel patches?

I enabled CONFIG_FS_ENCRYPTION in addition to the options I normally use, but
didn't realise I needed to enable CONFIG_CRYPTO_XTS as well.

David

Re: Error in generic/397 test script?

[Eric Biggers]

On Mon, Jan 20, 2025 at 05:41:40PM +0000, David Howells wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > It would be enlightening to understand what the issue was here.  Did you
> > explicitly disable these options, overriding the imply, without providing a
> > replacement?  Or was this another issue specific to unmerged kernel patches?
> 
> I enabled CONFIG_FS_ENCRYPTION in addition to the options I normally use, but
> didn't realise I needed to enable CONFIG_CRYPTO_XTS as well.
> 
> David
> 

So you had an explicit '# CONFIG_CRYPTO_XTS is not set' somewhere in your
kconfig that overrode the imply, right?

Wondering if the following commit should maybe be reconsidered:

    commit a0fc20333ee4bac1147c4cf75dea098c26671a2f
    Author: Ard Biesheuvel <ardb@kernel.org>
    Date:   Wed Apr 21 09:55:10 2021 +0200

        fscrypt: relax Kconfig dependencies for crypto API algorithms
        
        Even if FS encryption has strict functional dependencies on various
        crypto algorithms and chaining modes. those dependencies could potentially
        be satisified by other implementations than the generic ones, and no link
        time dependency exists on the 'depends on' claused defined by
        CONFIG_FS_ENCRYPTION_ALGS.
        
        So let's relax these clauses to 'imply', so that the default behavior
        is still to pull in those generic algorithms, but in a way that permits
        them to be disabled again in Kconfig.
        
        Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
        Acked-by: Eric Biggers <ebiggers@google.com>
        Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Re: Error in generic/397 test script?

[Eric Biggers]

On Mon, Jan 20, 2025 at 09:46:29AM -0800, Eric Biggers wrote:
> On Mon, Jan 20, 2025 at 05:41:40PM +0000, David Howells wrote:
> > Eric Biggers <ebiggers@kernel.org> wrote:
> > 
> > > It would be enlightening to understand what the issue was here.  Did you
> > > explicitly disable these options, overriding the imply, without providing a
> > > replacement?  Or was this another issue specific to unmerged kernel patches?
> > 
> > I enabled CONFIG_FS_ENCRYPTION in addition to the options I normally use, but
> > didn't realise I needed to enable CONFIG_CRYPTO_XTS as well.
> > 
> > David
> > 
> 
> So you had an explicit '# CONFIG_CRYPTO_XTS is not set' somewhere in your
> kconfig that overrode the imply, right?
> 
> Wondering if the following commit should maybe be reconsidered:
> 
>     commit a0fc20333ee4bac1147c4cf75dea098c26671a2f
>     Author: Ard Biesheuvel <ardb@kernel.org>
>     Date:   Wed Apr 21 09:55:10 2021 +0200
> 
>         fscrypt: relax Kconfig dependencies for crypto API algorithms
>         
>         Even if FS encryption has strict functional dependencies on various
>         crypto algorithms and chaining modes. those dependencies could potentially
>         be satisified by other implementations than the generic ones, and no link
>         time dependency exists on the 'depends on' claused defined by
>         CONFIG_FS_ENCRYPTION_ALGS.
>         
>         So let's relax these clauses to 'imply', so that the default behavior
>         is still to pull in those generic algorithms, but in a way that permits
>         them to be disabled again in Kconfig.
>         
>         Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>         Acked-by: Eric Biggers <ebiggers@google.com>
>         Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

By the way, CRYPTO_XTS is basically useless.  E.g. on x86_64 you really want
CRYPTO_AES_NI_INTEL.  I should probably throw in a selection of that (similar to
what CONFIG_WIREGUARD does where it selects optimized code for each arch),
though I've been hoping for this to be properly solved at the crypto API level.

- Eric

Re: Error in generic/397 test script?

[Eric Biggers]

On Mon, Jan 20, 2025 at 02:20:06PM +0000, David Howells wrote:
> generic/429 can also hang:
> 
> 
> 	show_file_contents()
> 	{
> 		echo "--- Contents of files using plaintext names:"
> 		cat $SCRATCH_MNT/edir/@@@ |& _filter_scratch
> 		cat $SCRATCH_MNT/edir/abcd |& _filter_scratch
> 		echo "--- Contents of files using no-key names:"
> 		cat ${nokey_names[@]} |& _filter_scratch | _filter_nokey_filenames edir
> 	}
> 	...
> 	nokey_names=( $(find $SCRATCH_MNT/edir -mindepth 1 | sort) )
> 	printf '%s\n' "${nokey_names[@]}" | \
> 		_filter_scratch | _filter_nokey_filenames edir
> 	show_file_contents
> 
> on the 'cat ...' at the end of show_file_contents().  A check that
> ${nokey_names[0]} is not nothing might be in order.
> 
> However, in this case (in which I'm running these against ceph), I don't think
> that the find should return nothing, so it's not a bug in the test script per
> se.

Similar to what I explained with generic/397, this can only happen if there is a
kernel bug.  Feel free to send a patch that updates the test to not hang in this
scenario, but the kernel bug (which is presumably in unmerged patches and not
yet upstream) will need to be fixed too.

- Eric

Re: Error in generic/397 test script?

[Eric Biggers]

On Mon, Jan 20, 2025 at 12:32:28PM +0000, David Howells wrote:
> Hi Eric,
> 
> In the generic/397 test script, you placed:
> 
> 	$XFS_IO_PROG -f $SCRATCH_MNT/edir/newfile |& _filter_scratch
> 	$XFS_IO_PROG -f $SCRATCH_MNT/edir/0123456789abcdef |& _filter_scratch
> 
> but neither of those lines actually has a command on it, and when I run it,
> I'm seeing xfs_io hang just waiting endlessly for someone to type commands on
> stdin.
> 
> Would it be better to do:
> 
> 	echo >$SCRATCH_MNT/edir/newfile |& _filter_scratch
> 	echo >$SCRATCH_MNT/edir/0123456789abcdef |& _filter_scratch

Those commands try to create new files and are supposed to fail with ENOKEY
because the directory's encryption key is not present, as is explained in the
comment just above them and can also be seen from 397.out.

First, I'm guessing the context here is that you're testing some (not yet
upstream) kernel patches that introduce a bug where creating these files does
not in fact fail?  That bug will need to be fixed before your patches are
merged, of course.

Second, yes it would be a good idea to replace these with something that don't
hang in the case of a kernel bug that allows the creation of these files.  This
could be done using a shell redirection as you've proposed, but it would have to
go in a subshell for the stderr to be filtered by _filter_scratch.  Feel free to
send a patch after you've fixed that and tested it with upstream.

- Eric

Re: Error in generic/397 test script?

[David Howells]

Eric Biggers <ebiggers@kernel.org> wrote:

> First, I'm guessing the context here is that you're testing some (not yet
> upstream) kernel patches that introduce a bug where creating these files does
> not in fact fail?  That bug will need to be fixed before your patches are
> merged, of course.

Nope.  I'm using v6.13 as-is with ceph.

David