Re: [PATCH v2 5/6] s390/uvdevice: Add List Secrets Ext IOCTL

[Heiko Carstens <hca@linux.ibm.com>]

On Wed, Oct 02, 2024 at 06:05:31PM +0200, Steffen Eiden wrote:
> Add an extended List Secrets IOCTL. In contrast to the first list IOCTL
> this accepts an index as the first two bytes of the provided page as an
> input. This index is then taken as the index offset for the list UVC to
> receive later entries for the list.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  arch/s390/include/uapi/asm/uvdevice.h |  4 ++
>  drivers/s390/char/uvdevice.c          | 96 ++++++++++++++++++++-------
>  2 files changed, 75 insertions(+), 25 deletions(-)

...

> +/* The actual list(_ext) IOCTL.
> + * If list_ext is true, the first two bytes of the user buffer set the starting index of the
> + * list-UVC
> + */

This is not standard kernel comment style; there is no need for a line
with more than >80 characters here; and in this case it would be nice
to add a "." at the end of the sentence :)

> +static int list_secrets(struct uvio_ioctl_cb *uv_ioctl, bool list_ext)
> +{
> +	void __user *user_buf_arg = (void __user *)uv_ioctl->argument_addr;
> +	u16 __user *user_index = (u16 __user *)uv_ioctl->argument_addr;
> +	u8 *secrets = NULL;

No need to preinitialize.

> +	u16 start_idx = 0;
> +	int ret;
> +
> +	if (uv_ioctl->argument_len != UVIO_LIST_SECRETS_LEN)
> +		return -EINVAL;
> +
> +	BUILD_BUG_ON(UVIO_LIST_SECRETS_LEN != PAGE_SIZE);
> +	secrets = (u8 *)get_zeroed_page(GFP_KERNEL);
> +	if (!secrets)
> +		return -ENOMEM;
> +
> +	/* The extended call accepts an u16 index as input */
> +	ret = -EFAULT;
> +	if (list_ext) {
> +		if (get_user(start_idx, user_index))
> +			goto err;
> +	}
> +
> +	uv_list_secrets(secrets, start_idx, &uv_ioctl->uv_rc, &uv_ioctl->uv_rrc);
> +
> +	if (copy_to_user(user_buf_arg, secrets, UVIO_LIST_SECRETS_LEN))
> +		goto err;
> +	ret = 0;
> +
> +err:
> +	free_pages((unsigned long)secrets, 0);
> +	return ret;
> +}

If you would change the order of the code a bit, it would be possible
to write this shorter:

	int ret = 0;
	...
	if (list_ext && get_user(start_idx, user_index))
		return -EFAULT;
	secrets = (u8 *)get_zeroed_page(GFP_KERNEL);
	if (!secrets)
		return -ENOMEM;
	uv_list_secrets(secrets, start_idx, &uv_ioctl->uv_rc, &uv_ioctl->uv_rrc);
	if (copy_to_user(user_buf_arg, secrets, UVIO_LIST_SECRETS_LEN))
		ret = -EFAULT;
	free_page((unsigned long)secrets);
	return ret;

> +/** uvio_list_secrets_ext() - perform a List Secret UVC with a starting index
> + * @uv_ioctl: ioctl control block

This is not kernel doc style.

Anyway, just my 0.02.