[PATCH 1/4] add compat_ioctl methods to dasd

[PATCH 1/4] add compat_ioctl methods to dasd

[Christoph Hellwig]

all dasd ioctls are directly useable from 32bit process, thus switch
the dasd driver to unlocked_ioctl/compat_ioctl and get rid of the
translations in the global table.


Index: linux-2.6/arch/s390/kernel/compat_ioctl.c
===================================================================
--- linux-2.6.orig/arch/s390/kernel/compat_ioctl.c	2005-11-04 13:49:43.000000000 +0100
+++ linux-2.6/arch/s390/kernel/compat_ioctl.c	2005-11-04 14:01:19.000000000 +0100
@@ -44,26 +44,6 @@
 #include "../../../fs/compat_ioctl.c"
 
 /* s390 only ioctls */
-COMPATIBLE_IOCTL(DASDAPIVER)
-COMPATIBLE_IOCTL(BIODASDDISABLE)
-COMPATIBLE_IOCTL(BIODASDENABLE)
-COMPATIBLE_IOCTL(BIODASDRSRV)
-COMPATIBLE_IOCTL(BIODASDRLSE)
-COMPATIBLE_IOCTL(BIODASDSLCK)
-COMPATIBLE_IOCTL(BIODASDINFO)
-COMPATIBLE_IOCTL(BIODASDINFO2)
-COMPATIBLE_IOCTL(BIODASDFMT)
-COMPATIBLE_IOCTL(BIODASDPRRST)
-COMPATIBLE_IOCTL(BIODASDQUIESCE)
-COMPATIBLE_IOCTL(BIODASDRESUME)
-COMPATIBLE_IOCTL(BIODASDPRRD)
-COMPATIBLE_IOCTL(BIODASDPSRD)
-COMPATIBLE_IOCTL(BIODASDGATTR)
-COMPATIBLE_IOCTL(BIODASDSATTR)
-COMPATIBLE_IOCTL(BIODASDCMFENABLE)
-COMPATIBLE_IOCTL(BIODASDCMFDISABLE)
-COMPATIBLE_IOCTL(BIODASDREADALLCMB)
-
 COMPATIBLE_IOCTL(TUBICMD)
 COMPATIBLE_IOCTL(TUBOCMD)
 COMPATIBLE_IOCTL(TUBGETI)
Index: linux-2.6/drivers/s390/block/dasd.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/dasd.c	2005-11-04 13:49:43.000000000 +0100
+++ linux-2.6/drivers/s390/block/dasd.c	2005-11-04 14:01:19.000000000 +0100
@@ -1726,7 +1726,10 @@
 	.owner		= THIS_MODULE,
 	.open		= dasd_open,
 	.release	= dasd_release,
-	.ioctl		= dasd_ioctl,
+	.unlocked_ioctl	= dasd_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= dasd_ioctl,
+#endif
 };
 
 
Index: linux-2.6/drivers/s390/block/dasd_int.h
===================================================================
--- linux-2.6.orig/drivers/s390/block/dasd_int.h	2005-11-04 13:49:43.000000000 +0100
+++ linux-2.6/drivers/s390/block/dasd_int.h	2005-11-04 14:01:19.000000000 +0100
@@ -525,7 +525,7 @@
 void dasd_ioctl_exit(void);
 int  dasd_ioctl_no_register(struct module *, int, dasd_ioctl_fn_t);
 int  dasd_ioctl_no_unregister(struct module *, int, dasd_ioctl_fn_t);
-int  dasd_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
+long dasd_ioctl(struct file *, unsigned int, unsigned long);
 
 /* externals in dasd_proc.c */
 int dasd_proc_init(void);
Index: linux-2.6/drivers/s390/block/dasd_ioctl.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/dasd_ioctl.c	2005-11-04 13:49:43.000000000 +0100
+++ linux-2.6/drivers/s390/block/dasd_ioctl.c	2005-11-04 14:01:19.000000000 +0100
@@ -79,15 +79,14 @@
 	return 0;
 }
 
-int
-dasd_ioctl(struct inode *inp, struct file *filp,
-	   unsigned int no, unsigned long data)
+long
+dasd_ioctl(struct file *filp, unsigned int no, unsigned long data)
 {
-	struct block_device *bdev = inp->i_bdev;
+	struct block_device *bdev = filp->f_dentry->d_inode->i_bdev;
 	struct dasd_device *device = bdev->bd_disk->private_data;
 	struct dasd_ioctl *ioctl;
 	const char *dir;
-	int rc;
+	int rc = -EINVAL;
 
 	if ((_IOC_DIR(no) != _IOC_NONE) && (data == 0)) {
 		PRINT_DEBUG("empty data ptr");
@@ -100,6 +99,8 @@
 	DBF_DEV_EVENT(DBF_DEBUG, device,
 		      "ioctl 0x%08x %s'0x%x'%d(%d) with data %8lx", no,
 		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
+
+	lock_kernel()
 	/* Search for ioctl no in the ioctl list. */
 	list_for_each_entry(ioctl, &dasd_ioctl_list, list) {
 		if (ioctl->no == no) {
@@ -108,14 +109,16 @@
 				continue;
 			rc = ioctl->handler(bdev, no, data);
 			module_put(ioctl->owner);
-			return rc;
+			goto out;
 		}
 	}
 	/* No ioctl with number no. */
 	DBF_DEV_EVENT(DBF_INFO, device,
 		      "unknown ioctl 0x%08x=%s'0x%x'%d(%d) data %8lx", no,
 		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
-	return -EINVAL;
+ out:
+	unlock_kernel();
+	return rc;
 }
 
 static int

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Christoph Hellwig]

On Fri, Nov 04, 2005 at 11:16:52PM +0100, Christoph Hellwig wrote:
> all dasd ioctls are directly useable from 32bit process, thus switch
> the dasd driver to unlocked_ioctl/compat_ioctl and get rid of the
> translations in the global table.

ping on all the four s390 compat_ioctl patches.  These are few of the
remaining arch compat_ioctl bits and I'd really really like to get rid
of them soonish.

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Martin Schwidefsky]

On Sat, 2005-11-12 at 10:33 +0100, Christoph Hellwig wrote:
> On Fri, Nov 04, 2005 at 11:16:52PM +0100, Christoph Hellwig wrote:
> > all dasd ioctls are directly useable from 32bit process, thus switch
> > the dasd driver to unlocked_ioctl/compat_ioctl and get rid of the
> > translations in the global table.
> 
> ping on all the four s390 compat_ioctl patches.  These are few of the
> remaining arch compat_ioctl bits and I'd really really like to get rid
> of them soonish.

Current status on the four patches:
1) dasd ioctl patch didn't compile (missing semicolon after
lock_kernel()) and doesn't work after fixing the compile problem. It's a
problem with the bdev->bd_disk->private_data which is NULL at the time
the partition detection code calls the BIODASDINFO and HDIO_GETGEO ioctl
with ioctl_by_bdev. I don't see an easy way to fix this right now.
2) fs3270 ioctl patch is fine. Fullscreen test works after fixing the
independent class_device_create problem (NULL argument missing, patch
will follow).
3) remove TIOCGSERIAL/TIOCSSERIAL patch. Fine with me. 
4) tape ioctl patch. Not yet sure about this one. Need to ask Stefan
Bader.

-- 
blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Christoph Hellwig]

On Tue, Nov 15, 2005 at 03:51:17PM +0100, Martin Schwidefsky wrote:
> On Sat, 2005-11-12 at 10:33 +0100, Christoph Hellwig wrote:
> > On Fri, Nov 04, 2005 at 11:16:52PM +0100, Christoph Hellwig wrote:
> > > all dasd ioctls are directly useable from 32bit process, thus switch
> > > the dasd driver to unlocked_ioctl/compat_ioctl and get rid of the
> > > translations in the global table.
> > 
> > ping on all the four s390 compat_ioctl patches.  These are few of the
> > remaining arch compat_ioctl bits and I'd really really like to get rid
> > of them soonish.
> 
> Current status on the four patches:
> 1) dasd ioctl patch didn't compile (missing semicolon after
> lock_kernel())

oops.

> and doesn't work after fixing the compile problem. It's a
> problem with the bdev->bd_disk->private_data which is NULL at the time
> the partition detection code calls the BIODASDINFO and HDIO_GETGEO ioctl
> with ioctl_by_bdev. I don't see an easy way to fix this right now.

my patch doesn't change anything related to dereferencing those fields.

I see the problem that you're probably having: ioctl_by_bdev calls
->ioctl without ensuring ->open has been called previously.  But I don't
see why this couldn't have happened previously.

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Christoph Hellwig]

On Tue, Nov 15, 2005 at 06:24:38PM +0100, Christoph Hellwig wrote:
> > and doesn't work after fixing the compile problem. It's a
> > problem with the bdev->bd_disk->private_data which is NULL at the time
> > the partition detection code calls the BIODASDINFO and HDIO_GETGEO ioctl
> > with ioctl_by_bdev. I don't see an easy way to fix this right now.
> 
> my patch doesn't change anything related to dereferencing those fields.
> 
> I see the problem that you're probably having: ioctl_by_bdev calls
> ->ioctl without ensuring ->open has been called previously.  But I don't
> see why this couldn't have happened previously.

So looking at it again I found a bug:  we return EINVAL on an invalid
ioctl, but the compat layer expects ENOIOCTLCMD so it returns using the
generic compat bits.  Returning ENOIOCTLCMD from unlocked_ioctl is fine
aswell as the ioctl layer turns it back.  The patch below fix this issue
and the missing semicolon after lock_kernel()


Index: linux-2.6/drivers/s390/block/dasd_ioctl.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/dasd_ioctl.c	2005-11-16 00:59:04.000000000 +0100
+++ linux-2.6/drivers/s390/block/dasd_ioctl.c	2005-11-16 01:02:36.000000000 +0100
@@ -86,11 +86,11 @@
 	struct dasd_device *device = bdev->bd_disk->private_data;
 	struct dasd_ioctl *ioctl;
 	const char *dir;
-	int rc = -EINVAL;
+	int rc = -ENOIOCTLCMD;
 
 	if ((_IOC_DIR(no) != _IOC_NONE) && (data == 0)) {
 		PRINT_DEBUG("empty data ptr");
-		return -EINVAL;
+		goto out;
 	}
 	dir = _IOC_DIR (no) == _IOC_NONE ? "0" :
 		_IOC_DIR (no) == _IOC_READ ? "r" :
@@ -100,7 +100,7 @@
 		      "ioctl 0x%08x %s'0x%x'%d(%d) with data %8lx", no,
 		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
 
-	lock_kernel()
+	lock_kernel();
 	/* Search for ioctl no in the ioctl list. */
 	list_for_each_entry(ioctl, &dasd_ioctl_list, list) {
 		if (ioctl->no == no) {
@@ -109,15 +109,16 @@
 				continue;
 			rc = ioctl->handler(bdev, no, data);
 			module_put(ioctl->owner);
-			goto out;
+			break;
 		}
 	}
 	/* No ioctl with number no. */
 	DBF_DEV_EVENT(DBF_INFO, device,
 		      "unknown ioctl 0x%08x=%s'0x%x'%d(%d) data %8lx", no,
 		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
- out:
+ out_unlock:
 	unlock_kernel();
+ out:
 	return rc;
 }
 

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Martin Schwidefsky]

On Wed, 2005-11-16 at 09:45 +0100, Christoph Hellwig wrote:
> On Tue, Nov 15, 2005 at 06:24:38PM +0100, Christoph Hellwig wrote:
> > > and doesn't work after fixing the compile problem. It's a
> > > problem with the bdev->bd_disk->private_data which is NULL at the time
> > > the partition detection code calls the BIODASDINFO and HDIO_GETGEO ioctl
> > > with ioctl_by_bdev. I don't see an easy way to fix this right now.
> > 
> > my patch doesn't change anything related to dereferencing those fields.
> > 
> > I see the problem that you're probably having: ioctl_by_bdev calls
> > ->ioctl without ensuring ->open has been called previously.  But I don't
> > see why this couldn't have happened previously.
> 
> So looking at it again I found a bug:  we return EINVAL on an invalid
> ioctl, but the compat layer expects ENOIOCTLCMD so it returns using the
> generic compat bits.  Returning ENOIOCTLCMD from unlocked_ioctl is fine
> aswell as the ioctl layer turns it back.  The patch below fix this issue
> and the missing semicolon after lock_kernel()

Still doesn't work. The reason is ioctl_by_bdev:

int ioctl_by_bdev(struct block_device *bdev,
                  unsigned cmd, unsigned long arg)
{
        int res;
        mm_segment_t old_fs = get_fs();
        set_fs(KERNEL_DS);
        res = blkdev_ioctl(bdev->bd_inode, NULL, cmd, arg);
        set_fs(old_fs);
        return res;
}

blkdev_ioctl is explicitly called with a NULL file pointer. That can't
work with block device drivers that use unlocked ioctls. We'd need to
create a struct file with at least a valid f_dentry->d_inode->i_bdev
chain in ioctl_by_bdev to make it work with unlocked ioctls. Not nice ..

-- 
blue skies,
   Martin

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

Re: [PATCH 1/4] add compat_ioctl methods to dasd

[Christoph Hellwig]

On Thu, Nov 17, 2005 at 01:27:32PM +0100, Martin Schwidefsky wrote:
> blkdev_ioctl is explicitly called with a NULL file pointer. That can't
> work with block device drivers that use unlocked ioctls. We'd need to
> create a struct file with at least a valid f_dentry->d_inode->i_bdev
> chain in ioctl_by_bdev to make it work with unlocked ioctls. Not nice ..

Yes, you're right.  Looks like we finally need to do the long-planned
sanitizing of the blkdev ioctl interface first.

Let's drop the patch for now.