Skip to contentSkip to conversation

ioctl cleanups: move SG_IO translation where it belongs

PATCH Pavel Machek 2 participants 3 messages May 6, 2003

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Hi!

This enables sharing of 200 lines of SG_IO support by all 64-bit architectures. If it looks okay, more such patches will follow.

Pavel

arch/x86_64/ia32/ia32_ioctl.c +0 -236
@@ -108,10 +108,6 @@
 
 #include <asm/mtrr.h>
 
-
-#define A(__x) ((void *)(unsigned long)(__x))
-#define AA(__x)	A(__x)
-
 /* Aiee. Someone does not find a difference between int and long */
 #define EXT2_IOC32_GETFLAGS               _IOR('f', 1, int)
 #define EXT2_IOC32_SETFLAGS               _IOW('f', 2, int)
@@ -1333,235 +1329,6 @@
 	return err;
 }
 
-
-typedef struct sg_io_hdr32 {
-	s32 interface_id;	/* [i] 'S' for SCSI generic (required) */
-	s32 dxfer_direction;	/* [i] data transfer direction  */
-	u8  cmd_len;		/* [i] SCSI command length ( <= 16 bytes) */
-	u8  mx_sb_len;		/* [i] max length to write to sbp */
-	u16 iovec_count;	/* [i] 0 implies no scatter gather */
-	u32 dxfer_len;		/* [i] byte count of data transfer */
-	u32 dxferp;		/* [i], [*io] points to data transfer memory
-					      or scatter gather list */
-	u32 cmdp;		/* [i], [*i] points to command to perform */
-	u32 sbp;		/* [i], [*o] points to sense_buffer memory */
-	u32 timeout;		/* [i] MAX_UINT->no timeout (unit: millisec) */
-	u32 flags;		/* [i] 0 -> default, see SG_FLAG... */
-	s32 pack_id;		/* [i->o] unused internally (normally) */
-	u32 usr_ptr;		/* [i->o] unused internally */
-	u8  status;		/* [o] scsi status */
-	u8  masked_status;	/* [o] shifted, masked scsi status */
-	u8  msg_status;		/* [o] messaging level data (optional) */
-	u8  sb_len_wr;		/* [o] byte count actually written to sbp */
-	u16 host_status;	/* [o] errors from host adapter */
-	u16 driver_status;	/* [o] errors from software driver */
-	s32 resid;		/* [o] dxfer_len - actual_transferred */
-	u32 duration;		/* [o] time taken by cmd (unit: millisec) */
-	u32 info;		/* [o] auxiliary information */
-} sg_io_hdr32_t;  /* 64 bytes long (on sparc32) */
-
-typedef struct sg_iovec32 {
-	u32 iov_base;
-	u32 iov_len;
-} sg_iovec32_t;
-
-static int alloc_sg_iovec(sg_io_hdr_t *sgp, u32 uptr32)
-{
-	sg_iovec32_t *uiov = (sg_iovec32_t *) A(uptr32);
-	sg_iovec_t *kiov;
-	int i;
-
-	sgp->dxferp = kmalloc(sgp->iovec_count *
-			      sizeof(sg_iovec_t), GFP_KERNEL);
-	if (!sgp->dxferp)
-		return -ENOMEM;
-	memset(sgp->dxferp, 0,
-	       sgp->iovec_count * sizeof(sg_iovec_t));
-
-	kiov = (sg_iovec_t *) sgp->dxferp;
-	for (i = 0; i < sgp->iovec_count; i++) {
-		u32 iov_base32;
-		if (__get_user(iov_base32, &uiov->iov_base) ||
-		    __get_user(kiov->iov_len, &uiov->iov_len))
-			return -EFAULT;
-
-		kiov->iov_base = kmalloc(kiov->iov_len, GFP_KERNEL);
-		if (!kiov->iov_base)
-			return -ENOMEM;
-		if (copy_from_user(kiov->iov_base,
-				   (void *) A(iov_base32),
-				   kiov->iov_len))
-			return -EFAULT;
-
-		uiov++;
-		kiov++;
-	}
-
-	return 0;
-}
-
-static int copy_back_sg_iovec(sg_io_hdr_t *sgp, u32 uptr32)
-{
-	sg_iovec32_t *uiov = (sg_iovec32_t *) A(uptr32);
-	sg_iovec_t *kiov = (sg_iovec_t *) sgp->dxferp;
-	int i;
-
-	for (i = 0; i < sgp->iovec_count; i++) {
-		u32 iov_base32;
-
-		if (__get_user(iov_base32, &uiov->iov_base))
-			return -EFAULT;
-
-		if (copy_to_user((void *) A(iov_base32),
-				 kiov->iov_base,
-				 kiov->iov_len))
-			return -EFAULT;
-
-		uiov++;
-		kiov++;
-	}
-
-	return 0;
-}
-
-static void free_sg_iovec(sg_io_hdr_t *sgp)
-{
-	sg_iovec_t *kiov = (sg_iovec_t *) sgp->dxferp;
-	int i;
-
-	for (i = 0; i < sgp->iovec_count; i++) {
-		if (kiov->iov_base) {
-			kfree(kiov->iov_base);
-			kiov->iov_base = NULL;
-		}
-		kiov++;
-	}
-	kfree(sgp->dxferp);
-	sgp->dxferp = NULL;
-}
-
-static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
-{
-	sg_io_hdr32_t *sg_io32;
-	sg_io_hdr_t sg_io64;
-	u32 dxferp32, cmdp32, sbp32;
-	mm_segment_t old_fs;
-	int err = 0;
-
-	sg_io32 = (sg_io_hdr32_t *)arg;
-	err = __get_user(sg_io64.interface_id, &sg_io32->interface_id);
-	err |= __get_user(sg_io64.dxfer_direction, &sg_io32->dxfer_direction);
-	err |= __get_user(sg_io64.cmd_len, &sg_io32->cmd_len);
-	err |= __get_user(sg_io64.mx_sb_len, &sg_io32->mx_sb_len);
-	err |= __get_user(sg_io64.iovec_count, &sg_io32->iovec_count);
-	err |= __get_user(sg_io64.dxfer_len, &sg_io32->dxfer_len);
-	err |= __get_user(sg_io64.timeout, &sg_io32->timeout);
-	err |= __get_user(sg_io64.flags, &sg_io32->flags);
-	err |= __get_user(sg_io64.pack_id, &sg_io32->pack_id);
-
-	sg_io64.dxferp = NULL;
-	sg_io64.cmdp = NULL;
-	sg_io64.sbp = NULL;
-
-	err |= __get_user(cmdp32, &sg_io32->cmdp);
-	sg_io64.cmdp = kmalloc(sg_io64.cmd_len, GFP_KERNEL);
-	if (!sg_io64.cmdp) {
-		err = -ENOMEM;
-		goto out;
-	}
-	if (copy_from_user(sg_io64.cmdp,
-			   (void *) A(cmdp32),
-			   sg_io64.cmd_len)) {
-		err = -EFAULT;
-		goto out;
-	}
-
-	err |= __get_user(sbp32, &sg_io32->sbp);
-	sg_io64.sbp = kmalloc(sg_io64.mx_sb_len, GFP_KERNEL);
-	if (!sg_io64.sbp) {
-		err = -ENOMEM;
-		goto out;
-	}
-	if (copy_from_user(sg_io64.sbp,
-			   (void *) A(sbp32),
-			   sg_io64.mx_sb_len)) {
-		err = -EFAULT;
-		goto out;
-	}
-
-	err |= __get_user(dxferp32, &sg_io32->dxferp);
-	if (sg_io64.iovec_count) {
-		int ret;
-
-		if ((ret = alloc_sg_iovec(&sg_io64, dxferp32))) {
-			err = ret;
-			goto out;
-		}
-	} else {
-		sg_io64.dxferp = kmalloc(sg_io64.dxfer_len, GFP_KERNEL);
-		if (!sg_io64.dxferp) {
-			err = -ENOMEM;
-			goto out;
-		}
-		if (copy_from_user(sg_io64.dxferp,
-				   (void *) A(dxferp32),
-				   sg_io64.dxfer_len)) {
-			err = -EFAULT;
-			goto out;
-		}
-	}
-
-	/* Unused internally, do not even bother to copy it over. */
-	sg_io64.usr_ptr = NULL;
-
-	if (err)
-		return -EFAULT;
-
-	old_fs = get_fs();
-	set_fs (KERNEL_DS);
-	err = sys_ioctl (fd, cmd, (unsigned long) &sg_io64);
-	set_fs (old_fs);
-
-	if (err < 0)
-		goto out;
-
-	err = __put_user(sg_io64.pack_id, &sg_io32->pack_id);
-	err |= __put_user(sg_io64.status, &sg_io32->status);
-	err |= __put_user(sg_io64.masked_status, &sg_io32->masked_status);
-	err |= __put_user(sg_io64.msg_status, &sg_io32->msg_status);
-	err |= __put_user(sg_io64.sb_len_wr, &sg_io32->sb_len_wr);
-	err |= __put_user(sg_io64.host_status, &sg_io32->host_status);
-	err |= __put_user(sg_io64.driver_status, &sg_io32->driver_status);
-	err |= __put_user(sg_io64.resid, &sg_io32->resid);
-	err |= __put_user(sg_io64.duration, &sg_io32->duration);
-	err |= __put_user(sg_io64.info, &sg_io32->info);
-	err |= copy_to_user((void *)A(sbp32), sg_io64.sbp, sg_io64.mx_sb_len);
-	if (sg_io64.dxferp) {
-		if (sg_io64.iovec_count)
-			err |= copy_back_sg_iovec(&sg_io64, dxferp32);
-		else
-			err |= copy_to_user((void *)A(dxferp32),
-					    sg_io64.dxferp,
-					    sg_io64.dxfer_len);
-	}
-	if (err)
-		err = -EFAULT;
-
-out:
-	if (sg_io64.cmdp)
-		kfree(sg_io64.cmdp);
-	if (sg_io64.sbp)
-		kfree(sg_io64.sbp);
-	if (sg_io64.dxferp) {
-		if (sg_io64.iovec_count) {
-			free_sg_iovec(&sg_io64);
-		} else {
-			kfree(sg_io64.dxferp);
-		}
-	}
-	return err;
-}
-
 struct sock_fprog32 {
 	__u16	len;
 	__u32	filter;
@@ -3190,8 +2957,6 @@
 
 /* And these ioctls need translation */
 HANDLE_IOCTL(TIOCGDEV, tiocgdev)
-HANDLE_IOCTL(TIOCGSERIAL, serial_struct_ioctl)
-HANDLE_IOCTL(TIOCSSERIAL, serial_struct_ioctl)
 HANDLE_IOCTL(MEMREADOOB32, mtd_rw_oob)
 HANDLE_IOCTL(MEMWRITEOOB32, mtd_rw_oob)
 #ifdef CONFIG_NET
@@ -3274,7 +3039,6 @@
 HANDLE_IOCTL(FDPOLLDRVSTAT32, fd_ioctl_trans)
 HANDLE_IOCTL(FDGETFDCSTAT32, fd_ioctl_trans)
 HANDLE_IOCTL(FDWERRORGET32, fd_ioctl_trans)
-HANDLE_IOCTL(SG_IO,sg_ioctl_trans)
 HANDLE_IOCTL(PPPIOCGIDLE32, ppp_ioctl_trans)
 HANDLE_IOCTL(PPPIOCSCOMPRESS32, ppp_ioctl_trans)
 HANDLE_IOCTL(PPPIOCSPASS32, ppp_sock_fprog_ioctl_trans)
drivers/block/Makefile +1 -1
@@ -25,7 +25,7 @@
 obj-$(CONFIG_BLK_DEV_PS2)	+= ps2esdi.o
 obj-$(CONFIG_BLK_DEV_XD)	+= xd.o
 obj-$(CONFIG_BLK_CPQ_DA)	+= cpqarray.o
-obj-$(CONFIG_BLK_CPQ_CISS_DA)  += cciss.o
+obj-$(CONFIG_BLK_CPQ_CISS_DA)	+= cciss.o
 obj-$(CONFIG_BLK_DEV_DAC960)	+= DAC960.o
 
 obj-$(CONFIG_BLK_DEV_UMEM)	+= umem.o
drivers/block/scsi_ioctl.c +244 -1

268 lines not on this page · raw message

drivers/scsi/scsi_ioctl.c +2 -4
@@ -24,12 +24,12 @@
 #include <scsi/scsi_ioctl.h>
 
 #define NORMAL_RETRIES			5
-#define IOCTL_NORMAL_TIMEOUT			(10 * HZ)
+#define IOCTL_NORMAL_TIMEOUT		(10 * HZ)
 #define FORMAT_UNIT_TIMEOUT		(2 * 60 * 60 * HZ)
 #define START_STOP_TIMEOUT		(60 * HZ)
 #define MOVE_MEDIUM_TIMEOUT		(5 * 60 * HZ)
 #define READ_ELEMENT_STATUS_TIMEOUT	(5 * 60 * HZ)
-#define READ_DEFECT_DATA_TIMEOUT	(60 * HZ )  /* ZIP-250 on parallel port takes as long! */
+#define READ_DEFECT_DATA_TIMEOUT	(60 * HZ)  /* ZIP-250 on parallel port takes as long! */
 
 #define MAX_BUF PAGE_SIZE
 
@@ -362,8 +362,6 @@
 error:
 	if (buf)
 		kfree(buf);
-
-
 	return result;
 #else
 	{
drivers/serial/core.c +62 -0

74 lines not on this page · raw message

include/asm-x86_64/compat.h +4 -0
@@ -128,4 +128,8 @@
 	return (void *)(unsigned long)uptr;
 }
 
+/* For compat_ioctl handlers */
+#define A(__x) ((void *)(unsigned long)(__x))
+#define AA(__x)	A(__x)
+
 #endif /* _ASM_X86_64_COMPAT_H */
include/linux/ioctl32.h +6 -3
@@ -2,9 +2,9 @@
 #define IOCTL32_H 1
 
 struct file;
-
 extern long sys_ioctl(unsigned int, unsigned int, unsigned long);
 
+
 /* 
  * Register an 32bit ioctl translation handler for ioctl cmd.
  *
@@ -14,12 +14,15 @@
  *                        arg: ioctl argument
  *                        struct file *file: file descriptor pointer.
  */ 
+typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int, unsigned long, struct file *);
 
 extern int register_ioctl32_conversion(unsigned int cmd, int (*handler)(unsigned int, unsigned int, unsigned long, struct file *));
-
 extern int unregister_ioctl32_conversion(unsigned int cmd);
 
-typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int, unsigned long, struct file *);
+/* FIXME: several conversions make sense for one cmd depending on
+ * device being used (don't complain, its ioctl, its supposed to be
+ * ugly). Therefore unregisters needs *handler, too.
+ */
 
 struct ioctl_trans {
 	unsigned long cmd;
Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#1 lore raw
Quoted 4 lines · Pavel Machek

Hi!

This enables sharing of 200 lines of SG_IO support by all 64-bit architectures. If it looks okay, more such patches will follow.

I currently have some patches for this function pending. When an unchanged data buffer is passed it is ok to just verify_area it, no need to kmalloc and copy. This simplifies this handler vastly.

Here is the part from the 2.4 patch; haven't tried it with 2.5 yet, but should apply there too.

Also adds some boundary checking.

-Andi

Index: linux-work/arch/x86_64/ia32/ia32_ioctl.c
===================================================================
RCS file: /home/cvs/Repository/linux/arch/x86_64/ia32/ia32_ioctl.c,v retrieving revision 1.31

arch/x86_64/ia32/ia32_ioctl.c +12 -43
diff -u -u -r1.31 ia32_ioctl.c
@@ -1373,12 +1381,16 @@
 	u32 iov_len;
 } sg_iovec32_t;
 
+#define EMU_SG_MAX 128
+
 static int alloc_sg_iovec(sg_io_hdr_t *sgp, u32 uptr32)
 {
 	sg_iovec32_t *uiov = (sg_iovec32_t *) A(uptr32);
 	sg_iovec_t *kiov;
 	int i;
 
+	if (sgp->iovec_count > EMU_SG_MAX)
+		return -EINVAL;
 	sgp->dxferp = kmalloc(sgp->iovec_count *
 			      sizeof(sg_iovec_t), GFP_KERNEL);
 	if (!sgp->dxferp)
@@ -1391,40 +1403,10 @@
 		u32 iov_base32;
 		if (__get_user(iov_base32, &uiov->iov_base) ||
 		    __get_user(kiov->iov_len, &uiov->iov_len))
-			return -EFAULT;
-
-		kiov->iov_base = kmalloc(kiov->iov_len, GFP_KERNEL);
-		if (!kiov->iov_base)
-			return -ENOMEM;
-		if (copy_from_user(kiov->iov_base,
-				   (void *) A(iov_base32),
-				   kiov->iov_len))
-			return -EFAULT;
-
-		uiov++;
-		kiov++;
-	}
-
-	return 0;
-}
-
-static int copy_back_sg_iovec(sg_io_hdr_t *sgp, u32 uptr32)
-{
-	sg_iovec32_t *uiov = (sg_iovec32_t *) A(uptr32);
-	sg_iovec_t *kiov = (sg_iovec_t *) sgp->dxferp;
-	int i;
-
-	for (i = 0; i < sgp->iovec_count; i++) {
-		u32 iov_base32;
-
-		if (__get_user(iov_base32, &uiov->iov_base))
 			return -EFAULT;
-
-		if (copy_to_user((void *) A(iov_base32),
-				 kiov->iov_base,
-				 kiov->iov_len))
+		if (verify_area(VERIFY_WRITE, (void *)A(iov_base32), kiov->iov_len))
 			return -EFAULT;
-
+		kiov->iov_base = (void *)A(iov_base32);
 		uiov++;
 		kiov++;
 	}
@@ -1434,16 +1416,6 @@
 
 static void free_sg_iovec(sg_io_hdr_t *sgp)
 {
-	sg_iovec_t *kiov = (sg_iovec_t *) sgp->dxferp;
-	int i;
-
-	for (i = 0; i < sgp->iovec_count; i++) {
-		if (kiov->iov_base) {
-			kfree(kiov->iov_base);
-			kiov->iov_base = NULL;
-		}
-		kiov++;
-	}
 	kfree(sgp->dxferp);
 	sgp->dxferp = NULL;
 }
@@ -1506,6 +1483,11 @@
 			goto out;
 		}
 	} else {
+		if (sg_io64.dxfer_len > 4*PAGE_SIZE) { 
+			err = -EINVAL;
+			goto out;
+		}
+
 		sg_io64.dxferp = kmalloc(sg_io64.dxfer_len, GFP_KERNEL);
 		if (!sg_io64.dxferp) {
 			err = -ENOMEM;
@@ -1546,7 +1528,7 @@
 	err |= copy_to_user((void *)A(sbp32), sg_io64.sbp, sg_io64.mx_sb_len);
 	if (sg_io64.dxferp) {
 		if (sg_io64.iovec_count)
-			err |= copy_back_sg_iovec(&sg_io64, dxferp32);
+			;
 		else
 			err |= copy_to_user((void *)A(dxferp32),
 					    sg_io64.dxferp,
#2 lore raw

Hi!

Quoted 4 lines
Quoted 2 lines

This enables sharing of 200 lines of SG_IO support by all 64-bit architectures. If it looks okay, more such patches will follow.

Quoted 8 lines

I currently have some patches for this function pending. When an unchanged data buffer is passed it is ok to just verify_area it, no need to kmalloc and copy. This simplifies this handler vastly.

Here is the part from the 2.4 patch; haven't tried it with 2.5 yet, but should apply there too.

Also adds some boundary checking.

I'll merge it. Can you mail me whole 2.4 patch [I had it but deleted it, sorry]. I'll attempt to port it to 2.5 and return it back to you...

Pavel

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]