add compat_ioctl methods to dasd
Subsystems (2)
PATCH 1/4 add compat_ioctl methods to dasd
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.
arch/s390/kernel/compat_ioctl.c +0 -20
@@ -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)
drivers/s390/block/dasd.c +4 -1
@@ -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 };
drivers/s390/block/dasd_int.h +1 -1
@@ -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 );
drivers/s390/block/dasd_ioctl.c +10 -7
@@ -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
Quoted 3 lines · 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.
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.
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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.
ping on all the four s390
compat_ioctlpatches. These are few of the remaining archcompat_ioctlbits 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.
Signature · 6 lines
blue skies, Martin Martin Schwidefsky Linux for zSeries Development & Services IBM Deutschland Entwicklung GmbH
Quoted 6 lines · Martin Schwidefsky
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · 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.
Quoted 7 lines
Quoted 3 lines
ping on all the four s390
compat_ioctlpatches. These are few of the remaining archcompat_ioctlbits 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 afterlock_kernel())
oops.
Quoted 4 lines
and doesn't work after fixing the compile problem. It's a problem with the
bdev->bd_disk->private_datawhich is NULL at the time the partition detection code calls the BIODASDINFO andHDIO_GETGEOioctl withioctl_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.
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines
and doesn't work after fixing the compile problem. It's a problem with the
bdev->bd_disk->private_datawhich is NULL at the time the partition detection code calls the BIODASDINFO andHDIO_GETGEOioctl withioctl_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_bdevcalls ->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()
drivers/s390/block/dasd_ioctl.c +6 -5
@@ -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; }
Quoted 12 lines · Christoph Hellwig
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines
and doesn't work after fixing the compile problem. It's a problem with the
bdev->bd_disk->private_datawhich is NULL at the time the partition detection code calls the BIODASDINFO andHDIO_GETGEOioctl withioctl_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_bdevcalls ->ioctl without ensuring ->open has been called previously. But I don't see why this couldn't have happened previously.
Quoted 5 lines
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_ioctlis fine aswell as the ioctl layer turns it back. The patch below fix this issue and the missing semicolon afterlock_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 ..
Signature · 6 lines
blue skies, Martin Martin Schwidefsky Linux for zSeries Development & Services IBM Deutschland Entwicklung GmbH
Quoted 4 lines · Martin Schwidefsky
blkdev_ioctlis 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 validf_dentry->d_inode->i_bdevchain inioctl_by_bdevto 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.