On Thu, 18 Oct 2018, Christoph Hellwig wrote:
> +
> +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;
> +
> + switch (gen.command.OpCode) {
> + case GDT_IOCTL:
> + gen.command.u.ioctl.p_param = paddr;
> + break;
> + case CACHESERVICE:
> + gdth_ioc_cacheservice(ha, &gen, paddr);
> + break;
> + case SCSIRAWSERVICE:
> + gdth_ioc_scsiraw(ha, &gen, paddr);
> + break;
> + default:
> + goto out_free_buf;
> }
> - }
AFAICT, CACHESERVICE never gets assigned to command.OpCode.
That means your switch() is not equivalent to the original construction --
if (gen.command.OpCode == GDT_IOCTL) {
} else if (gen.command.Service == CACHESERVICE) {
} else if (gen.command.Service == SCSIRAWSERVICE) {
} else {
}
>
> - rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout,
> &gen.info);
> - if (rval < 0) {
> +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;
> - }
> - gen.status = rval;
> -
> - 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;
> + return 0;
This appears to be wrong also. I think you wanted,
return rval;
--
> }
>
> static int ioc_hdrlist(void __user *arg, char *cmnd)
>
