stop using ioctl_by_bdev in the s390 DASD driver

stop using ioctl_by_bdev in the s390 DASD driver

[Christoph Hellwig]

Hi Jens and DASD maintainers,

can you take a look at this series, which stops the DASD driver from
issuing ioctls from kernel space, in preparation of removing
ioctl_by_bdev.  I don't really like the new s390-only method, but short
of forcing the dasd driver to be built into the kernel I can't think of
anything better.  But maybe the s390 maintainers are fine with forcing
the DASD driver to be built in, in which case we could go down that
route?

[PATCH 1/3] dasd: refactor dasd_ioctl_information

[Christoph Hellwig]

Prepare for in-kernel callers of this functionality.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/block/dasd_ioctl.c | 38 +++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index 9a5f3add325f..dabcb4ce92da 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -457,10 +457,9 @@ static int dasd_ioctl_read_profile(struct dasd_block *block, void __user *argp)
 /*
  * Return dasd information. Used for BIODASDINFO and BIODASDINFO2.
  */
-static int dasd_ioctl_information(struct dasd_block *block,
-				  unsigned int cmd, void __user *argp)
+static int __dasd_ioctl_information(struct dasd_block *block,
+		struct dasd_information2_t *dasd_info)
 {
-	struct dasd_information2_t *dasd_info;
 	struct subchannel_id sch_id;
 	struct ccw_dev_id dev_id;
 	struct dasd_device *base;
@@ -473,10 +472,6 @@ static int dasd_ioctl_information(struct dasd_block *block,
 	if (!base->discipline || !base->discipline->fill_info)
 		return -EINVAL;
 
-	dasd_info = kzalloc(sizeof(struct dasd_information2_t), GFP_KERNEL);
-	if (dasd_info == NULL)
-		return -ENOMEM;
-
 	rc = base->discipline->fill_info(base, dasd_info);
 	if (rc) {
 		kfree(dasd_info);
@@ -520,15 +515,24 @@ static int dasd_ioctl_information(struct dasd_block *block,
 	list_for_each(l, &base->ccw_queue)
 		dasd_info->chanq_len++;
 	spin_unlock_irqrestore(&block->queue_lock, flags);
+	return 0;
+}
 
-	rc = 0;
-	if (copy_to_user(argp, dasd_info,
-			 ((cmd == (unsigned int) BIODASDINFO2) ?
-			  sizeof(struct dasd_information2_t) :
-			  sizeof(struct dasd_information_t))))
-		rc = -EFAULT;
+static int dasd_ioctl_information(struct dasd_block *block, void __user *argp,
+		size_t copy_size)
+{
+	struct dasd_information2_t *dasd_info;
+	int error = 0;
+
+	dasd_info = kzalloc(sizeof(*dasd_info), GFP_KERNEL);
+	if (!dasd_info)
+		return -ENOMEM;
+
+	error = __dasd_ioctl_information(block, dasd_info);
+	if (!error && copy_to_user(argp, dasd_info, copy_size))
+		error = -EFAULT;
 	kfree(dasd_info);
-	return rc;
+	return error;
 }
 
 /*
@@ -622,10 +626,12 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 		rc = dasd_ioctl_check_format(bdev, argp);
 		break;
 	case BIODASDINFO:
-		rc = dasd_ioctl_information(block, cmd, argp);
+		rc = dasd_ioctl_information(block, argp,
+				sizeof(struct dasd_information_t));
 		break;
 	case BIODASDINFO2:
-		rc = dasd_ioctl_information(block, cmd, argp);
+		rc = dasd_ioctl_information(block, argp,
+				sizeof(struct dasd_information2_t));
 		break;
 	case BIODASDPRRD:
 		rc = dasd_ioctl_read_profile(block, argp);
-- 
2.26.1

[PATCH 2/3] block: add a s390-only biodasdinfo method

[Christoph Hellwig]

The IBM partition parser needs to query the DASD driver for details that
are very s390 specific.  Instead of using ioctl_by_bdev with a fake user
space pointer just add a s390-specific method to get the information
directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/block/dasd.c       |  1 +
 drivers/s390/block/dasd_int.h   |  1 +
 drivers/s390/block/dasd_ioctl.c | 13 +++++++++++++
 include/linux/blkdev.h          |  5 +++++
 4 files changed, 20 insertions(+)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index cf87eb27879f..26edd93167e7 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3394,6 +3394,7 @@ dasd_device_operations = {
 	.ioctl		= dasd_ioctl,
 	.compat_ioctl	= dasd_ioctl,
 	.getgeo		= dasd_getgeo,
+	.biodasdinfo	= dasd_biodasdinfo,
 };
 
 /*******************************************************************************
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index fa552f9f1666..6eac7b11c75b 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -845,6 +845,7 @@ void dasd_destroy_partitions(struct dasd_block *);
 
 /* externals in dasd_ioctl.c */
 int  dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
+int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info);
 
 /* externals in dasd_proc.c */
 int dasd_proc_init(void);
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index dabcb4ce92da..332836d46ba0 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -666,3 +666,16 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 	dasd_put_device(base);
 	return rc;
 }
+
+int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info)
+{
+	struct dasd_device *base;
+	int error;
+
+	base = dasd_device_from_gendisk(disk);
+	if (!base)
+		return -ENODEV;
+	error = __dasd_ioctl_information(base->block, info);
+	dasd_put_device(base);
+	return error;
+}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 32868fbedc9e..3b92cc970083 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -43,6 +43,7 @@ struct pr_ops;
 struct rq_qos;
 struct blk_queue_stats;
 struct blk_stat_callback;
+struct dasd_information2_t;
 
 #define BLKDEV_MIN_RQ	4
 #define BLKDEV_MAX_RQ	128	/* Default maximum */
@@ -1700,6 +1701,10 @@ struct block_device_operations {
 	int (*report_zones)(struct gendisk *, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data);
 	char *(*devnode)(struct gendisk *disk, umode_t *mode);
+#ifdef CONFIG_S390
+	int (*biodasdinfo)(struct gendisk *disk,
+			struct dasd_information2_t *info);
+#endif
 	struct module *owner;
 	const struct pr_ops *pr_ops;
 };
-- 
2.26.1

[PATCH 3/3] partitions/ibm: stop using ioctl_by_bdev

[Christoph Hellwig]

Just call the getgeo and biodasdinfo methods directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/partitions/ibm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 073faa6a69b8..21dc6da20ff2 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -289,6 +289,7 @@ static int find_cms1_partitions(struct parsed_partitions *state,
 int ibm_partition(struct parsed_partitions *state)
 {
 	struct block_device *bdev = state->bdev;
+	struct gendisk *disk = bdev->bd_disk;
 	int blocksize, res;
 	loff_t i_size, offset, size;
 	dasd_information2_t *info;
@@ -308,15 +309,16 @@ int ibm_partition(struct parsed_partitions *state)
 	info = kmalloc(sizeof(dasd_information2_t), GFP_KERNEL);
 	if (info == NULL)
 		goto out_exit;
-	geo = kmalloc(sizeof(struct hd_geometry), GFP_KERNEL);
+	geo = kzalloc(sizeof(struct hd_geometry), GFP_KERNEL);
 	if (geo == NULL)
 		goto out_nogeo;
 	label = kmalloc(sizeof(union label_t), GFP_KERNEL);
 	if (label == NULL)
 		goto out_nolab;
-	if (ioctl_by_bdev(bdev, HDIO_GETGEO, (unsigned long)geo) != 0)
+	geo->start = get_start_sect(bdev);
+	if (!disk->fops->getgeo || disk->fops->getgeo(bdev, geo))
 		goto out_freeall;
-	if (ioctl_by_bdev(bdev, BIODASDINFO2, (unsigned long)info) != 0) {
+	if (!disk->fops->biodasdinfo || disk->fops->biodasdinfo(disk, info)) {
 		kfree(info);
 		info = NULL;
 	}
-- 
2.26.1

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Christian Borntraeger]

On 21.04.20 08:12, Christoph Hellwig wrote:
> Hi Jens and DASD maintainers,
> 
> can you take a look at this series, which stops the DASD driver from
> issuing ioctls from kernel space, in preparation of removing
> ioctl_by_bdev.  I don't really like the new s390-only method, but short
> of forcing the dasd driver to be built into the kernel I can't think of
> anything better.  But maybe the s390 maintainers are fine with forcing
> the DASD driver to be built in, in which case we could go down that
> route?

Hmm the defconfig results in dasd built-in anyway. But distros really like
to keep it modular.

Hmm, we do have

obj-$(CONFIG_DASD) += dasd_mod.o
obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
obj-$(CONFIG_DASD_FBA)  += dasd_fba_mod.o

Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
Not sure about the implications. 

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Cornelia Huck]

On Tue, 21 Apr 2020 11:58:31 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 21.04.20 08:12, Christoph Hellwig wrote:
> > Hi Jens and DASD maintainers,
> > 
> > can you take a look at this series, which stops the DASD driver from
> > issuing ioctls from kernel space, in preparation of removing
> > ioctl_by_bdev.  I don't really like the new s390-only method, but short
> > of forcing the dasd driver to be built into the kernel I can't think of
> > anything better.  But maybe the s390 maintainers are fine with forcing
> > the DASD driver to be built in, in which case we could go down that
> > route?  
> 
> Hmm the defconfig results in dasd built-in anyway. But distros really like
> to keep it modular.
> 
> Hmm, we do have
> 
> obj-$(CONFIG_DASD) += dasd_mod.o
> obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
> obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
> obj-$(CONFIG_DASD_FBA)  += dasd_fba_mod.o
> 
> Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
> Not sure about the implications. 
> 

I don't think non-eckd dasd drivers are really useful outside of z/VM
guests, so keeping at least the disciplines modular would be good.

Also, what about special purpose environments like the zfcp dumper?
Would be good to be able to keep these small.

How big is the dasd code in the end?

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Christian Borntraeger]

On 21.04.20 12:32, Cornelia Huck wrote:
> On Tue, 21 Apr 2020 11:58:31 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 21.04.20 08:12, Christoph Hellwig wrote:
>>> Hi Jens and DASD maintainers,
>>>
>>> can you take a look at this series, which stops the DASD driver from
>>> issuing ioctls from kernel space, in preparation of removing
>>> ioctl_by_bdev.  I don't really like the new s390-only method, but short
>>> of forcing the dasd driver to be built into the kernel I can't think of
>>> anything better.  But maybe the s390 maintainers are fine with forcing
>>> the DASD driver to be built in, in which case we could go down that
>>> route?  
>>
>> Hmm the defconfig results in dasd built-in anyway. But distros really like
>> to keep it modular.
>>
>> Hmm, we do have
>>
>> obj-$(CONFIG_DASD) += dasd_mod.o
>> obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
>> obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
>> obj-$(CONFIG_DASD_FBA)  += dasd_fba_mod.o
>>
>> Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
>> Not sure about the implications. 
>>
> 
> I don't think non-eckd dasd drivers are really useful outside of z/VM
> guests, so keeping at least the disciplines modular would be good.
> 
> Also, what about special purpose environments like the zfcp dumper?
> Would be good to be able to keep these small.
> 
> How big is the dasd code in the end?


  File: drivers/s390/block/dasd_diag_mod.ko
  Size: 519976    	Blocks: 1016       IO Block: 4096   regular file
--
  File: drivers/s390/block/dasd_eckd_mod.ko
  Size: 2125976   	Blocks: 4160       IO Block: 4096   regular file
--
  File: drivers/s390/block/dasd_fba_mod.ko
  Size: 524256    	Blocks: 1024       IO Block: 4096   regular file
--
  File: drivers/s390/block/dasd_mod.ko
  Size: 3273464   	Blocks: 6400       IO Block: 4096   regular file


So 3 MB seems quite a lot for special purpose Linuxes like the zfcp dumper. 

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Christoph Hellwig]

On Tue, Apr 21, 2020 at 12:43:03PM +0200, Christian Borntraeger wrote:
> So 3 MB seems quite a lot for special purpose Linuxes like the zfcp dumper. 

Does the zfcp dumper need DASD support at all?  We don't have to always
build in the DASD core to avoid the strange dasd-specific block_device
operation, but only ensure it is built-in if it is selected at all.

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Stefan Haberland]

Hi Christoph,

thanks for addressing this. But I must say that I do not like this approach.
I get your point why you want this ioctl to be removed and I agree with
that.

Having those implicit partitions at all is really ugly but I fear that this
is widely used int the field. So I can not simply remove this code although
I would like to. Maybe we find a way to deprecate this.
But anyway...

Forcing the driver to be build in may have a lot of implications which we
should at least have a look at and maybe discuss with the distributors.
All major distributions have the driver build as modules and use module
parameters in the initrd for example.

The second thing is that I do not really like this s390-specific blockdevice
operation.

I can imagine some ways to get rid of this ioctl_by_bdev. Maybe having a
udev
rule to add a partition from userspace or having the driver add the implicit
partition at the end. Or maybe something else.

If it is OK I will have a look at this and discuss this issue with my
colleagues and come up with a different approach.

Regards,
Stefan



Am 21.04.20 um 08:12 schrieb Christoph Hellwig:
> Hi Jens and DASD maintainers,
>
> can you take a look at this series, which stops the DASD driver from
> issuing ioctls from kernel space, in preparation of removing
> ioctl_by_bdev.  I don't really like the new s390-only method, but short
> of forcing the dasd driver to be built into the kernel I can't think of
> anything better.  But maybe the s390 maintainers are fine with forcing
> the DASD driver to be built in, in which case we could go down that
> route?

Re: stop using ioctl_by_bdev in the s390 DASD driver

[Christoph Hellwig]

On Tue, Apr 21, 2020 at 04:17:53PM +0200, Stefan Haberland wrote:
> I can imagine some ways to get rid of this ioctl_by_bdev. Maybe having a
> udev
> rule to add a partition from userspace or having the driver add the implicit
> partition at the end. Or maybe something else.
> 
> If it is OK I will have a look at this and discuss this issue with my
> colleagues and come up with a different approach.

Sure, we can wait a few days.  Note that I don't want to break existing
userspace, which kinda speaks against a udev solution.