Re: [PATCH 1/4] gdth: refactor ioc_general

[Christoph Hellwig]

> > +   switch (gen.command.OpCode) {
> > +   case GDT_IOCTL:
> > +   gen.command.u.ioctl.p_param = paddr;
> > +   break;
> > +   case CACHESERVICE:
> > +   gdth_ioc_cacheservice(ha, , paddr);
> > +   break;
> > +   case SCSIRAWSERVICE:
> > +   gdth_ioc_scsiraw(ha, , paddr);
> > +   break;
> > +   default:
> > +   goto out_free_buf;
> >  }
> > -}
> 
> AFAICT, CACHESERVICE never gets assigned to command.OpCode.

Thanks, fixed.

> > -}
> > -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;

Also fixed.

Re: [PATCH 1/4] gdth: refactor ioc_general

[Finn Thain]

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(, 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, );
> + 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, , paddr);
> + break;
> + case SCSIRAWSERVICE:
> + gdth_ioc_scsiraw(ha, , 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, , cmnd, gen.timeout, 
> );
> -if (rval < 0) {
> +execute:
> + rval = __gdth_execute(ha->sdev, , cmnd, gen.timeout,
> + );
> + 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, ,
> + 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, , 
> -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)
> 

[PATCH 1/4] gdth: refactor ioc_general

[Christoph Hellwig]

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 
---
 drivers/scsi/gdth.c | 247 +++-
 1 file changed, 130 insertions(+), 117 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 16709735b546..2bec840018ad 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -4155,131 +4155,144 @@ 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 = 0x;
+   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(, 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, )))
-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 = 0x;
-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