On Thu, 6 Dec 2018, Christoph Hellwig wrote:
> This function is a huge mess with duplicated error handling. Split out
> a few useful helpers and use goto labels to untangle the error handling
> and no-data ioctl handling.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/scsi/gdth.c | 244 +++++++++++++++++++++++---------------------
> 1 file changed, 126 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 16709735b546..45e67d4cb3af 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -4155,131 +4155,139 @@ static int ioc_resetdrv(void __user *arg, char *cmnd)
> return 0;
> }
>
> -static int ioc_general(void __user *arg, char *cmnd)
> +static void gdth_ioc_addr32(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> {
> - gdth_ioctl_general gen;
> - char *buf = NULL;
> - u64 paddr;
> - gdth_ha_str *ha;
> - int rval;
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache.DestAddr = 0xffffffff;
> + gen->command.u.cache.sg_canz = 1;
> + gen->command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> + gen->command.u.cache.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache.DestAddr = paddr;
> + gen->command.u.cache.sg_canz = 0;
> + }
> +}
>
> - if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
> - return -EFAULT;
> - ha = gdth_find_ha(gen.ionode);
> - if (!ha)
> - return -EFAULT;
> +static void gdth_ioc_addr64(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache64.DestAddr = (u64)-1;
> + gen->command.u.cache64.sg_canz = 1;
> + gen->command.u.cache64.sg_lst[0].sg_ptr = paddr;
> + gen->command.u.cache64.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache64.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache64.DestAddr = paddr;
> + gen->command.u.cache64.sg_canz = 0;
> + }
> +}
>
> - if (gen.data_len > INT_MAX)
> - return -EINVAL;
> - if (gen.sense_len > INT_MAX)
> - return -EINVAL;
> - if (gen.data_len + gen.sense_len > INT_MAX)
> - return -EINVAL;
> +static void gdth_ioc_cacheservice(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & GDT_64BIT) {
> + /* copy elements from 32-bit IOCTL structure */
> + gen->command.u.cache64.BlockCnt = gen->command.u.cache.BlockCnt;
> + gen->command.u.cache64.BlockNo = gen->command.u.cache.BlockNo;
> + gen->command.u.cache64.DeviceNo = gen->command.u.cache.DeviceNo;
>
> - if (gen.data_len + gen.sense_len != 0) {
> - if (!(buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len,
> - FALSE, &paddr)))
> - return -EFAULT;
> - if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),
> - gen.data_len + gen.sense_len)) {
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return -EFAULT;
> - }
> + gdth_ioc_addr64(ha, gen, paddr);
> + } else {
> + gdth_ioc_addr32(ha, gen, paddr);
> + }
> +}
>
> - if (gen.command.OpCode == GDT_IOCTL) {
> - gen.command.u.ioctl.p_param = paddr;
> - } else if (gen.command.Service == CACHESERVICE) {
> - if (ha->cache_feat & GDT_64BIT) {
> - /* copy elements from 32-bit IOCTL structure */
> - gen.command.u.cache64.BlockCnt = gen.command.u.cache.BlockCnt;
> - gen.command.u.cache64.BlockNo = gen.command.u.cache.BlockNo;
> - gen.command.u.cache64.DeviceNo = gen.command.u.cache.DeviceNo;
> - /* addresses */
> - if (ha->cache_feat & SCATTER_GATHER) {
> - gen.command.u.cache64.DestAddr = (u64)-1;
> - gen.command.u.cache64.sg_canz = 1;
> - gen.command.u.cache64.sg_lst[0].sg_ptr = paddr;
> - gen.command.u.cache64.sg_lst[0].sg_len = gen.data_len;
> - gen.command.u.cache64.sg_lst[1].sg_len = 0;
> - } else {
> - gen.command.u.cache64.DestAddr = paddr;
> - gen.command.u.cache64.sg_canz = 0;
> - }
> - } else {
> - if (ha->cache_feat & SCATTER_GATHER) {
> - gen.command.u.cache.DestAddr = 0xffffffff;
> - gen.command.u.cache.sg_canz = 1;
> - gen.command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> - gen.command.u.cache.sg_lst[0].sg_len = gen.data_len;
> - gen.command.u.cache.sg_lst[1].sg_len = 0;
> - } else {
> - gen.command.u.cache.DestAddr = paddr;
> - gen.command.u.cache.sg_canz = 0;
> - }
> - }
> - } else if (gen.command.Service == SCSIRAWSERVICE) {
> - if (ha->raw_feat & GDT_64BIT) {
> - /* copy elements from 32-bit IOCTL structure */
> - char cmd[16];
> - gen.command.u.raw64.sense_len = gen.command.u.raw.sense_len;
> - gen.command.u.raw64.bus = gen.command.u.raw.bus;
> - gen.command.u.raw64.lun = gen.command.u.raw.lun;
> - gen.command.u.raw64.target = gen.command.u.raw.target;
> - memcpy(cmd, gen.command.u.raw.cmd, 16);
> - memcpy(gen.command.u.raw64.cmd, cmd, 16);
> - gen.command.u.raw64.clen = gen.command.u.raw.clen;
> - gen.command.u.raw64.sdlen = gen.command.u.raw.sdlen;
> - gen.command.u.raw64.direction = gen.command.u.raw.direction;
> - /* addresses */
> - if (ha->raw_feat & SCATTER_GATHER) {
> - gen.command.u.raw64.sdata = (u64)-1;
> - gen.command.u.raw64.sg_ranz = 1;
> - gen.command.u.raw64.sg_lst[0].sg_ptr = paddr;
> - gen.command.u.raw64.sg_lst[0].sg_len = gen.data_len;
> - gen.command.u.raw64.sg_lst[1].sg_len = 0;
> - } else {
> - gen.command.u.raw64.sdata = paddr;
> - gen.command.u.raw64.sg_ranz = 0;
> - }
> - gen.command.u.raw64.sense_data = paddr + gen.data_len;
> - } else {
> - if (ha->raw_feat & SCATTER_GATHER) {
> - gen.command.u.raw.sdata = 0xffffffff;
> - gen.command.u.raw.sg_ranz = 1;
> - gen.command.u.raw.sg_lst[0].sg_ptr = (u32)paddr;
> - gen.command.u.raw.sg_lst[0].sg_len = gen.data_len;
> - gen.command.u.raw.sg_lst[1].sg_len = 0;
> - } else {
> - gen.command.u.raw.sdata = paddr;
> - gen.command.u.raw.sg_ranz = 0;
> - }
> - gen.command.u.raw.sense_data = (u32)paddr + gen.data_len;
> - }
> - } else {
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return -EFAULT;
> - }
> - }
> +static void gdth_ioc_scsiraw(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->raw_feat & GDT_64BIT) {
> + /* copy elements from 32-bit IOCTL structure */
> + char cmd[16];
> +
> + gen->command.u.raw64.sense_len = gen->command.u.raw.sense_len;
> + gen->command.u.raw64.bus = gen->command.u.raw.bus;
> + gen->command.u.raw64.lun = gen->command.u.raw.lun;
> + gen->command.u.raw64.target = gen->command.u.raw.target;
> + memcpy(cmd, gen->command.u.raw.cmd, 16);
> + memcpy(gen->command.u.raw64.cmd, cmd, 16);
> + gen->command.u.raw64.clen = gen->command.u.raw.clen;
> + gen->command.u.raw64.sdlen = gen->command.u.raw.sdlen;
> + gen->command.u.raw64.direction = gen->command.u.raw.direction;
> +
> + gdth_ioc_addr64(ha, gen, paddr);
> +
> + gen->command.u.raw64.sense_data = paddr + gen->data_len;
> + } else {
> + gdth_ioc_addr32(ha, gen, paddr);
>
The new gdth_ioc_scsiraw() function is not equivalent to the old code.
These calls to the new gdth_ioc_addr64() and gdth_ioc_addr32() functions
are not appropriate, as they operate on gen->command.u.cache and
gen->command.u.cache64, not on gen.command.u.raw and gen.command.u.raw64.
> - rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout, &gen.info);
> - if (rval < 0) {
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return rval;
> - }
> - gen.status = rval;
> + gen->command.u.raw.sense_data = (u32)paddr + gen->data_len;
> + }
> +}
>
> - if (copy_to_user(arg + sizeof(gdth_ioctl_general), buf,
> - gen.data_len + gen.sense_len)) {
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return -EFAULT;
> - }
> - if (copy_to_user(arg, &gen,
> - sizeof(gdth_ioctl_general) - sizeof(gdth_cmd_str))) {
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return -EFAULT;
> - }
> - gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> - return 0;
> +static int ioc_general(void __user *arg, char *cmnd)
> +{
> + gdth_ioctl_general gen;
> + gdth_ha_str *ha;
> + char *buf = NULL;
> + u64 paddr;
> + int rval;
> +
> + if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
> + return -EFAULT;
> + ha = gdth_find_ha(gen.ionode);
> + if (!ha)
> + return -EFAULT;
> +
> + if (gen.data_len > INT_MAX)
> + return -EINVAL;
> + if (gen.sense_len > INT_MAX)
> + return -EINVAL;
> + if (gen.data_len + gen.sense_len > INT_MAX)
> + return -EINVAL;
> +
> + if (gen.data_len + gen.sense_len == 0)
> + goto execute;
> +
> + buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len, FALSE, &paddr);
> + if (!buf)
> + return -EFAULT;
> +
> + rval = -EFAULT;
> + if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),
> + gen.data_len + gen.sense_len))
> + goto out_free_buf;
> +
> + if (gen.command.OpCode == GDT_IOCTL)
> + gen.command.u.ioctl.p_param = paddr;
> + else if (gen.command.Service == CACHESERVICE)
> + gdth_ioc_cacheservice(ha, &gen, paddr);
> + else if (gen.command.Service == SCSIRAWSERVICE)
> + gdth_ioc_scsiraw(ha, &gen, paddr);
> + else
> + goto out_free_buf;
> +
> +execute:
I think the old 'if' statement was more readable than the new
'goto execute'.
--
> + rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout,
> + &gen.info);
> + if (rval < 0)
> + goto out_free_buf;
> + gen.status = rval;
> +
> + rval = -EFAULT;
> + if (copy_to_user(arg + sizeof(gdth_ioctl_general), buf,
> + gen.data_len + gen.sense_len))
> + goto out_free_buf;
> + if (copy_to_user(arg, &gen,
> + sizeof(gdth_ioctl_general) - sizeof(gdth_cmd_str)))
> + goto out_free_buf;
> +
> + rval = 0;
> +out_free_buf:
> + gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> + return rval;
> }
>
> static int ioc_hdrlist(void __user *arg, char *cmnd)
>
