Re: [patch 05/10] s390: add missing validation for dasd discipline specific ioctls

Re: [patch 05/10] s390: add missing validation for dasd discipline specific ioctls

[Horst Hummel]

Christoph Hellwig <hch@lst.de> wrote on 08.02.2006 15:08:39:

> On Wed, Feb 08, 2006 at 01:37:09PM +0100, Heiko Carstens wrote:
> > From: Horst Hummel <horst.hummel@de.ibm.com>
> > 
> > Because of missing discipline validition dasd ioctls calls may not
return
> > when called on a device handled by a different discipline.
> > (e.g calling ECKD specific BIODASDGATTR on an FBA device).
> > This addresses one of the issues Christoph has with the dasd driver.
> 
> Nack.  the right way to do this is to have per-discipline ioctl
methods,
> even if we can't remove the dasd_ioctl_register interface yet due to
> other disagreements.  Just resurect those parts of my patch.
> 
As I already tried to explain, the DASD ioctls are _not_ discipline
related. A discipline is more or less a 'access method to a physical 
DASD'. Sine the ioctls are related to 'functional components', each
ioctl could be valid in combination with none, one, two or every of
the currently supported disciplines (ECKD, FBA, DIAG).

Re: [patch 05/10] s390: add missing validation for dasd discipline specific ioctls

[Christoph Hellwig]

On Wed, Feb 08, 2006 at 05:56:59PM +0100, Horst Hummel wrote:
> Christoph Hellwig <hch@lst.de> wrote on 08.02.2006 15:08:39:
> 
> > On Wed, Feb 08, 2006 at 01:37:09PM +0100, Heiko Carstens wrote:
> > > From: Horst Hummel <horst.hummel@de.ibm.com>
> > > 
> > > Because of missing discipline validition dasd ioctls calls may not
> return
> > > when called on a device handled by a different discipline.
> > > (e.g calling ECKD specific BIODASDGATTR on an FBA device).
> > > This addresses one of the issues Christoph has with the dasd driver.
> > 
> > Nack.  the right way to do this is to have per-discipline ioctl
> methods,
> > even if we can't remove the dasd_ioctl_register interface yet due to
> > other disagreements.  Just resurect those parts of my patch.
> > 
> As I already tried to explain, the DASD ioctls are _not_ discipline
> related. A discipline is more or less a 'access method to a physical 
> DASD'. Sine the ioctls are related to 'functional components', each
> ioctl could be valid in combination with none, one, two or every of
> the currently supported disciplines (ECKD, FBA, DIAG).

Right now there's either ioctls that are valid for one (ECKD) or all
disciplines.   As a special case you've put ioctls that are valid for
all disciplines into odd modules, but that's just another case of lots
of crap in s390 drivers that needs sorting out.

[patch 05/10] s390: add missing validation for dasd discipline specific ioctls

[Heiko Carstens]

From: Horst Hummel <horst.hummel@de.ibm.com>

Because of missing discipline validition dasd ioctls calls may not return
when called on a device handled by a different discipline.
(e.g calling ECKD specific BIODASDGATTR on an FBA device).
This addresses one of the issues Christoph has with the dasd driver.

Signed-off-by: Horst Hummel <horst.hummel@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 drivers/s390/block/dasd.c      |    8 ++++-
 drivers/s390/block/dasd_eckd.c |   59 ++++++++++++++++++++++++++++-------------
 drivers/s390/block/dasd_eer.c  |   39 +++++++++++++++++++++------
 3 files changed, 78 insertions(+), 28 deletions(-)

diff -urpN linux-2.6/drivers/s390/block/dasd.c linux-2.6-patched/drivers/s390/block/dasd.c
--- linux-2.6/drivers/s390/block/dasd.c	2006-02-08 10:48:22.000000000 +0100
+++ linux-2.6-patched/drivers/s390/block/dasd.c	2006-02-08 10:48:45.000000000 +0100
@@ -670,8 +670,10 @@ dasd_term_IO(struct dasd_ccw_req * cqr)
 
 	/* Check the cqr */
 	rc = dasd_check_cqr(cqr);
-	if (rc)
+	if (rc) {
+		cqr->status = DASD_CQR_FAILED;
 		return rc;
+	}
 	retries = 0;
 	device = (struct dasd_device *) cqr->device;
 	while ((retries < 5) && (cqr->status == DASD_CQR_IN_IO)) {
@@ -724,8 +726,10 @@ dasd_start_IO(struct dasd_ccw_req * cqr)
 
 	/* Check the cqr */
 	rc = dasd_check_cqr(cqr);
-	if (rc)
+	if (rc) {
+		cqr->status = DASD_CQR_FAILED;
 		return rc;
+	}
 	device = (struct dasd_device *) cqr->device;
 	if (cqr->retries < 0) {
 		DEV_MESSAGE(KERN_DEBUG, device,
diff -urpN linux-2.6/drivers/s390/block/dasd_eckd.c linux-2.6-patched/drivers/s390/block/dasd_eckd.c
--- linux-2.6/drivers/s390/block/dasd_eckd.c	2006-02-08 10:48:22.000000000 +0100
+++ linux-2.6-patched/drivers/s390/block/dasd_eckd.c	2006-02-08 10:48:45.000000000 +0100
@@ -224,6 +224,28 @@ check_XRC (struct ccw1         *de_ccw,
 
 } /* end check_XRC */
 
+/*
+ * Check if called ioctl is valid on this device type.
+ * Returns device if anything is fine, ERR_PTR otherwise.
+ */
+static inline struct dasd_device *
+dasd_eckd_validate_ioctl(struct block_device *bdev, int no)
+{
+	struct dasd_device *device;
+
+	device = bdev->bd_disk->private_data;
+	if (device == NULL)
+		return ERR_PTR(-ENODEV);
+
+	if (strncmp((char *)&device->discipline->name, "ECKD", 4)) {
+		DEV_MESSAGE(KERN_WARNING, device,
+			    "ioctl (%x) not supported for %.4s device.",
+			    no, (char *)&device->discipline->name);
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+	return device;
+}
+
 static inline void
 define_extent(struct ccw1 * ccw, struct DE_eckd_data * data, int trk,
 	      int totrk, int cmd, struct dasd_device * device)
@@ -1236,9 +1258,9 @@ dasd_eckd_release(struct block_device *b
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	cqr = dasd_smalloc_request(dasd_eckd_discipline.name,
 				   1, 32, device);
@@ -1281,9 +1303,9 @@ dasd_eckd_reserve(struct block_device *b
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	cqr = dasd_smalloc_request(dasd_eckd_discipline.name,
 				   1, 32, device);
@@ -1325,9 +1347,9 @@ dasd_eckd_steal_lock(struct block_device
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	cqr = dasd_smalloc_request(dasd_eckd_discipline.name,
 				   1, 32, device);
@@ -1367,9 +1389,9 @@ dasd_eckd_performance(struct block_devic
 	struct ccw1 *ccw;
 	int rc;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	cqr = dasd_smalloc_request(dasd_eckd_discipline.name,
 				   1 /* PSF */  + 1 /* RSSD */ ,
@@ -1438,9 +1460,9 @@ dasd_eckd_get_attrib (struct block_devic
         if (!args)
                 return -EINVAL;
 
-        device = bdev->bd_disk->private_data;
-        if (device == NULL)
-                return -ENODEV;
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
         private = (struct dasd_eckd_private *) device->private;
         attrib = private->attrib;
@@ -1467,9 +1489,10 @@ dasd_eckd_set_attrib(struct block_device
 	if (!args)
 		return -EINVAL;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+
+	device = dasd_eckd_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	if (copy_from_user(&attrib, (void __user *) args,
 			   sizeof (struct attrib_data_t))) {
diff -urpN linux-2.6/drivers/s390/block/dasd_eer.c linux-2.6-patched/drivers/s390/block/dasd_eer.c
--- linux-2.6/drivers/s390/block/dasd_eer.c	2006-02-08 10:48:22.000000000 +0100
+++ linux-2.6-patched/drivers/s390/block/dasd_eer.c	2006-02-08 10:48:45.000000000 +0100
@@ -109,6 +109,28 @@ static spinlock_t bufferlock = SPIN_LOCK
 DECLARE_WAIT_QUEUE_HEAD(dasd_eer_read_wait_queue);
 
 /*
+ * Check if called ioctl is valid on this device type.
+ * Returns device if anything is fine, ERR_PTR otherwise.
+ */
+static inline struct dasd_device *
+dasd_eer_validate_ioctl(struct block_device *bdev, int no)
+{
+	struct dasd_device *device;
+
+	device = bdev->bd_disk->private_data;
+	if (device == NULL)
+		return ERR_PTR(-ENODEV);
+
+	if (strncmp((char *)&device->discipline->name, "ECKD", 4)) {
+		DEV_MESSAGE(KERN_WARNING, device,
+			    "ioctl (%x) not supported for %.4s device.",
+			    no, (char *)&device->discipline->name);
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+	return device;
+}
+
+/*
  * How many free bytes are available on the buffer.
  * needs to be called with bufferlock held
  */
@@ -523,8 +545,7 @@ static int
 dasd_eer_enable_on_device(struct dasd_device *device)
 {
 	void *eer;
-	if (!device)
-		return -ENODEV;
+
 	if (device->eer)
 		return 0;
 	if (!try_module_get(THIS_MODULE)) {
@@ -575,9 +596,10 @@ dasd_ioctl_set_eer(struct block_device *
 		return -EINVAL;
 	if (get_user(intval, (int __user *) args))
 		return -EFAULT;
-	device =  bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+
+	device = dasd_eer_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
 
 	intval = (intval != 0);
 	DEV_MESSAGE (KERN_DEBUG, device,
@@ -597,9 +619,10 @@ dasd_ioctl_get_eer(struct block_device *
 {
 	struct dasd_device *device;
 
-	device =  bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+	device = dasd_eer_validate_ioctl(bdev, no);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
+
 	return put_user((device->eer != NULL), (int __user *) args);
 }
 

Re: [patch 05/10] s390: add missing validation for dasd discipline specific ioctls

[Christoph Hellwig]

On Wed, Feb 08, 2006 at 01:37:09PM +0100, Heiko Carstens wrote:
> From: Horst Hummel <horst.hummel@de.ibm.com>
> 
> Because of missing discipline validition dasd ioctls calls may not return
> when called on a device handled by a different discipline.
> (e.g calling ECKD specific BIODASDGATTR on an FBA device).
> This addresses one of the issues Christoph has with the dasd driver.

Nack.  the right way to do this is to have per-discipline ioctl methods,
even if we can't remove the dasd_ioctl_register interface yet due to
other disagreements.  Just resurect those parts of my patch.