Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command

From: Joe Perches <joe@perches.com>
To: mgross@linux.intel.com, markgross@kernel.org, arnd@arndb.de,
	bp@suse.de, damien.lemoal@wdc.com, dragan.cvetic@xilinx.com,
	gregkh@linuxfoundation.org, corbet@lwn.net,
	leonard.crestez@nxp.com, palmerdabbelt@google.com,
	paul.walmsley@sifive.com, peng.fan@nxp.com, robh+dt@kernel.org,
	shawnguo@kernel.org
Cc: linux-kernel@vger.kernel.org, Seamus Kelly <seamus.kelly@intel.com>
Subject: Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command
Date: Wed, 09 Dec 2020 00:30:46 -0800	[thread overview]
Message-ID: <f54fcb1e9f90b529826d8d6abb2ae99b15108d16.camel@perches.com> (raw)
In-Reply-To: <20201201223511.65542-23-mgross@linux.intel.com>

On Tue, 2020-12-01 at 14:35 -0800, mgross@linux.intel.com wrote:
> Refactor the too large IOCTL function to call helper functions.
[]
> diff --git a/drivers/misc/xlink-core/xlink-ioctl.c b/drivers/misc/xlink-core/xlink-ioctl.c
[]
> +int ioctl_write_data(unsigned long arg)
> +{
> +	struct xlink_handle	devh	= {0};
> +	struct xlinkwritedata		wr	= {0};
> +	int rc = 0;
> +
> +	if (copy_from_user(&wr, (void __user *)arg,
> +			   sizeof(struct xlinkwritedata)))
> +		return -EFAULT;
> +	if (copy_from_user(&devh, (void __user *)wr.handle,
> +			   sizeof(struct xlink_handle)))
> +		return -EFAULT;
> +	if (wr.size <= XLINK_MAX_DATA_SIZE) {
> +		rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage,
> +					   wr.size);
> +		if (copy_to_user((void __user *)wr.return_code, (void *)&rc,
> +				 sizeof(rc)))
> +			return -EFAULT;
> +	} else {
> +		return -EFAULT;
> +	}

Please reverse the test to reduce indentation

	if (wr.size > XLINK_MAX_DATA_SIZE)
		return -EFAULT;
	rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);
	if (copy_to_user((void __user *)wr.return_code, (void *)&rc, sizeof(rc)))
		return -EFAULT;
	return rc;

The last 3 lines here are repeated multiple times in many functions.
It might be sensible to add something like:

int copy_result_to_user(u32 *where, int rc)
{
	if (copy_to_user((void __user *)where, &rc, sizeof(rc)))
		return -EFAULT;
	return rc;
}

so this could be written

	rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);

	return copy_result_to_user(wr.return_code, rc);

IMO: return_code isn't a great name for a pointer as it rather
indicates a value not an address and there's an awful lot of
casting to __user in all this code that perhaps should be marked
in the struct definitions rather than inside the function uses.

> +}
> +
> +int ioctl_write_control_data(unsigned long arg)
> +{
> +	struct xlink_handle	devh	= {0};

All of these initializations with {0} should use {} instead as
the first element of whatever struct is not guaranteed to be
assignable as an int and gcc/clang guarantee 0 initialization

> +	struct xlinkwritedata		wr	= {0};
> +	u8 volbuf[XLINK_MAX_BUF_SIZE];
> +	int rc = 0;
> +
> +	if (copy_from_user(&wr, (void __user *)arg,
> +			   sizeof(struct xlinkwritedata)))
> +		return -EFAULT;
> +	if (copy_from_user(&devh, (void __user *)wr.handle,
> +			   sizeof(struct xlink_handle)))
> +		return -EFAULT;
> +	if (wr.size <= XLINK_MAX_CONTROL_DATA_SIZE) {
> +		if (copy_from_user(volbuf, (void __user *)wr.pmessage,
> +				   wr.size))
> +			return -EFAULT;
> +		rc = xlink_write_control_data(&devh, wr.chan, volbuf,
> +					      wr.size);
> +		if (copy_to_user((void __user *)wr.return_code,
> +				 (void *)&rc, sizeof(rc)))
> +			return -EFAULT;
> +	} else {
> +		return -EFAULT;

Same test reversal and deindentation please.

> +	}
> +	return rc;
> +}
> +