Re: [PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Eric Sandeen <sandeen@redhat.com>]

On 10/21/24 1:09 AM, Tyler Hicks wrote:
> On 2024-10-07 10:27:43, Eric Sandeen wrote:
>> Convert ecryptfs to the new mount API.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/ecryptfs/ecryptfs_kernel.h |   7 -
>>  fs/ecryptfs/main.c            | 393 +++++++++++++++++-----------------
>>  2 files changed, 198 insertions(+), 202 deletions(-)
>>

...

>> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
>> index d03f1c6ccc1c..a7829983304e 100644
>> --- a/fs/ecryptfs/main.c
>> +++ b/fs/ecryptfs/main.c

...

>> +enum { Opt_sig, Opt_ecryptfs_sig,
>> +       Opt_cipher, Opt_ecryptfs_cipher,
>> +       Opt_ecryptfs_key_bytes,
>> +       Opt_passthrough, Opt_xattr_metadata,
>> +       Opt_encrypted_view, Opt_fnek_sig,
>> +       Opt_fn_cipher, Opt_fn_cipher_key_bytes,
>> +       Opt_unlink_sigs, Opt_mount_auth_tok_only,
>> +       Opt_check_dev_ruid };
> 
> checkpatch complains about these lines starting with spaces rather than
> a tab. I think that's a valid nit. Can we switch to tabs?

Oh, sure. They have spaces upstream and I didn't catch that when I tweaked
the enum.
 
>> -		case ecryptfs_opt_err:
>> -		default:
>> -			printk(KERN_WARNING
>> -			       "%s: eCryptfs: unrecognized option [%s]\n",
>> -			       __func__, p);
> 
> I think we lost this error message, which can be helpful for users when
> debugging mount(2) failures.

Invalid options never make it to the parsing function under the new mount
API, see below. Honestly we should be able to even remove default: but it seems
like most prior conversions don't do that, maybe to keep static checkers happy,
not sure.

> See below in your new code where we handle the default case for what I
> think we should include.
> 
>> +
>> +	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
>> +	if (opt < 0)
>> +		return opt;
>> +
>> +	switch (opt) {
>> +	case Opt_sig:
>> +	case Opt_ecryptfs_sig:
>> +		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
>> +						  param->string, 0);
>> +		if (rc) {
>> +			printk(KERN_ERR "Error attempting to register "
>> +			       "global sig; rc = [%d]\n", rc);
> 
> Are we expected to be using errorf() and friends here rather than
> printk()?

That's kind of a debate. If you'd rather get rid of the kernel message and
send it out through the mount api message channel instead, I can make that
change. But if userspace doesn't capture the message from errorf, that change
would lose the message altogether.

I kind of feel like once userspace is really making use of the message channel,
we could go back and selectively change printks to the message channel where it
makes sense.

>> +			return rc;;
> 
> There's an extra semicolon here.

Ugh, sorry.

>>  		}
>> +		ctx->sig_set = 1;
>> +		break;
>> +	case Opt_cipher:
>> +	case Opt_ecryptfs_cipher:
>> +		strscpy(mount_crypt_stat->global_default_cipher_name,
>> +			param->string);
>> +		ctx->cipher_name_set = 1;
>> +		break;
>> +	case Opt_ecryptfs_key_bytes:
>> +		mount_crypt_stat->global_default_cipher_key_size =
>> +			result.uint_32;
>> +		ctx->cipher_key_bytes_set = 1;
>> +		break;
>> +	case Opt_passthrough:
>> +		mount_crypt_stat->flags |=
>> +			ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
>> +		break;
>> +	case Opt_xattr_metadata:
>> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
>> +		break;
>> +	case Opt_encrypted_view:
>> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
>> +		mount_crypt_stat->flags |= ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
>> +		break;
>> +	case Opt_fnek_sig:
>> +		strscpy(mount_crypt_stat->global_default_fnek_sig,
>> +			param->string);
>> +		rc = ecryptfs_add_global_auth_tok(
>> +			mount_crypt_stat,
>> +			mount_crypt_stat->global_default_fnek_sig,
>> +			ECRYPTFS_AUTH_TOK_FNEK);
>> +		if (rc) {
>> +			printk(KERN_ERR "Error attempting to register "
>> +			       "global fnek sig [%s]; rc = [%d]\n",
>> +			       mount_crypt_stat->global_default_fnek_sig, rc);
> 
> Same errorf() question here.

Same answer. :) If you don't mind those messages disappearing from kernel logs
I can change it.

>> +			return rc;
>> +		}
>> +		mount_crypt_stat->flags |=
>> +			(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
>> +			 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
>> +		break;
>> +	case Opt_fn_cipher:
>> +		strscpy(mount_crypt_stat->global_default_fn_cipher_name,
>> +			param->string);
>> +		ctx->fn_cipher_name_set = 1;
>> +		break;
>> +	case Opt_fn_cipher_key_bytes:
>> +		mount_crypt_stat->global_default_fn_cipher_key_bytes =
>> +			result.uint_32;
>> +		ctx->fn_cipher_key_bytes_set = 1;
>> +		break;
>> +	case Opt_unlink_sigs:
>> +		mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
>> +		break;
>> +	case Opt_mount_auth_tok_only:
>> +		mount_crypt_stat->flags |= ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
>> +		break;
>> +	case Opt_check_dev_ruid:
>> +		ctx->check_ruid = 1;
>> +		break;
>> +	default:
> 
> To retain the unrecognized option warning, I think we need this:
> 
> 
>                 printk(KERN_WARNING "%s: eCryptfs: unrecognized option [%s]\n",
>                        __func__, param->key);

I think you'll see that if you make that change, you never hit the default:
case and it's still not printed.

vfs_parse_fs_param() does do "invalf(fc, "%s: Unknown parameter '%s'", ..."
which sends the message back out over the api message interface, and modern util-linux
mount will emit it on the console.

For example (just since I had jfs handy and it's mount is simple) if I add:

        default:
                printk("default yo\n");
                return -EINVAL;

and try it:

# mount -o loop,blahblah jfsfile mnt
mount: /root/jfs-test/mnt: fsconfig system call failed: jfs: Unknown parameter 'blahblah'.
       dmesg(1) may have more information after failed mount system call.
# dmesg | tail -n 2
[61556.764697] JFS: nTxBlock = 8192, nTxLock = 65536
[61561.280700] loop1: detected capacity change from 0 to 204800
#

Thanks for the review, and sorry for the missed nitpicks.
-Eric


> Tyler