[PATCH 0/2] DASD DIAG patches

[PATCH 0/2] DASD DIAG patches

[Stefan Haberland]

Hi Jens,

please see the following two patches that fix and improve the DASD DIAG driver.

Regards,
Stefan

Gustavo A. R. Silva (1):
  s390/dasd: Use struct_size() helper

Stefan Haberland (1):
  s390/dasd: fix inability to use DASD with DIAG driver

 drivers/s390/block/dasd_diag.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH 1/2] s390/dasd: fix inability to use DASD with DIAG driver

[Stefan Haberland]

During initialization of the DASD DIAG driver a request is issued
that has a bio structure that resides on the stack. With virtually
mapped kernel stacks this bio address might be in virtual storage
which is unsuitable for usage with the diag250 call.
In this case the device can not be set online using the DIAG
discipline and fails with -EOPNOTSUP.
In the system journal the following error message is presented:

dasd: X.X.XXXX Setting the DASD online with discipline DIAG failed
with rc=-95

Fix by allocating the bio structure instead of having it on the stack.

Fixes: ce3dc447493f ("s390: add support for virtually mapped kernel stacks")
Cc: stable@vger.kernel.org #4.20
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
---
 drivers/s390/block/dasd_diag.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
index facb588d09e4..069d6b39cacf 100644
--- a/drivers/s390/block/dasd_diag.c
+++ b/drivers/s390/block/dasd_diag.c
@@ -319,7 +319,7 @@ dasd_diag_check_device(struct dasd_device *device)
 	struct dasd_diag_characteristics *rdc_data;
 	struct vtoc_cms_label *label;
 	struct dasd_block *block;
-	struct dasd_diag_bio bio;
+	struct dasd_diag_bio *bio;
 	unsigned int sb, bsize;
 	blocknum_t end_block;
 	int rc;
@@ -395,29 +395,36 @@ dasd_diag_check_device(struct dasd_device *device)
 		rc = -ENOMEM;
 		goto out;
 	}
+	bio = kzalloc(sizeof(*bio), GFP_KERNEL);
+	if (bio == NULL)  {
+		DBF_DEV_EVENT(DBF_WARNING, device, "%s",
+			      "No memory to allocate initialization bio");
+		rc = -ENOMEM;
+		goto out_label;
+	}
 	rc = 0;
 	end_block = 0;
 	/* try all sizes - needed for ECKD devices */
 	for (bsize = 512; bsize <= PAGE_SIZE; bsize <<= 1) {
 		mdsk_init_io(device, bsize, 0, &end_block);
-		memset(&bio, 0, sizeof (struct dasd_diag_bio));
-		bio.type = MDSK_READ_REQ;
-		bio.block_number = private->pt_block + 1;
-		bio.buffer = label;
+		memset(bio, 0, sizeof(*bio));
+		bio->type = MDSK_READ_REQ;
+		bio->block_number = private->pt_block + 1;
+		bio->buffer = label;
 		memset(&private->iob, 0, sizeof (struct dasd_diag_rw_io));
 		private->iob.dev_nr = rdc_data->dev_nr;
 		private->iob.key = 0;
 		private->iob.flags = 0;	/* do synchronous io */
 		private->iob.block_count = 1;
 		private->iob.interrupt_params = 0;
-		private->iob.bio_list = &bio;
+		private->iob.bio_list = bio;
 		private->iob.flaga = DASD_DIAG_FLAGA_DEFAULT;
 		rc = dia250(&private->iob, RW_BIO);
 		if (rc == 3) {
 			pr_warn("%s: A 64-bit DIAG call failed\n",
 				dev_name(&device->cdev->dev));
 			rc = -EOPNOTSUPP;
-			goto out_label;
+			goto out_bio;
 		}
 		mdsk_term_io(device);
 		if (rc == 0)
@@ -427,7 +434,7 @@ dasd_diag_check_device(struct dasd_device *device)
 		pr_warn("%s: Accessing the DASD failed because of an incorrect format (rc=%d)\n",
 			dev_name(&device->cdev->dev), rc);
 		rc = -EIO;
-		goto out_label;
+		goto out_bio;
 	}
 	/* check for label block */
 	if (memcmp(label->label_id, DASD_DIAG_CMS1,
@@ -457,6 +464,8 @@ dasd_diag_check_device(struct dasd_device *device)
 			(rc == 4) ? ", read-only device" : "");
 		rc = 0;
 	}
+out_bio:
+	kfree(bio);
 out_label:
 	free_page((long) label);
 out:
-- 
2.17.1

[PATCH 2/2] s390/dasd: Use struct_size() helper

[Stefan Haberland]

From: "Gustavo A. R. Silva" <gustavoars@kernel.org>

Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes. Also, remove unnecessary
variable _datasize_.

This code was detected with the help of Coccinelle and, audited and
fixed manually.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_diag.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
index 069d6b39cacf..1b9e1442e6a5 100644
--- a/drivers/s390/block/dasd_diag.c
+++ b/drivers/s390/block/dasd_diag.c
@@ -515,7 +515,7 @@ static struct dasd_ccw_req *dasd_diag_build_cp(struct dasd_device *memdev,
 	struct req_iterator iter;
 	struct bio_vec bv;
 	char *dst;
-	unsigned int count, datasize;
+	unsigned int count;
 	sector_t recid, first_rec, last_rec;
 	unsigned int blksize, off;
 	unsigned char rw_cmd;
@@ -543,10 +543,8 @@ static struct dasd_ccw_req *dasd_diag_build_cp(struct dasd_device *memdev,
 	if (count != last_rec - first_rec + 1)
 		return ERR_PTR(-EINVAL);
 	/* Build the request */
-	datasize = sizeof(struct dasd_diag_req) +
-		count*sizeof(struct dasd_diag_bio);
-	cqr = dasd_smalloc_request(DASD_DIAG_MAGIC, 0, datasize, memdev,
-				   blk_mq_rq_to_pdu(req));
+	cqr = dasd_smalloc_request(DASD_DIAG_MAGIC, 0, struct_size(dreq, bio, count),
+				   memdev, blk_mq_rq_to_pdu(req));
 	if (IS_ERR(cqr))
 		return cqr;
 
-- 
2.17.1

Re: [PATCH 1/2] s390/dasd: fix inability to use DASD with DIAG driver

[Jens Axboe]

On 7/14/20 2:03 PM, Stefan Haberland wrote:
> During initialization of the DASD DIAG driver a request is issued
> that has a bio structure that resides on the stack. With virtually
> mapped kernel stacks this bio address might be in virtual storage
> which is unsuitable for usage with the diag250 call.
> In this case the device can not be set online using the DIAG
> discipline and fails with -EOPNOTSUP.
> In the system journal the following error message is presented:
> 
> dasd: X.X.XXXX Setting the DASD online with discipline DIAG failed
> with rc=-95
> 
> Fix by allocating the bio structure instead of having it on the stack.
> 
> Fixes: ce3dc447493f ("s390: add support for virtually mapped kernel stacks")
> Cc: stable@vger.kernel.org #4.20
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_diag.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
> index facb588d09e4..069d6b39cacf 100644
> --- a/drivers/s390/block/dasd_diag.c
> +++ b/drivers/s390/block/dasd_diag.c
> @@ -319,7 +319,7 @@ dasd_diag_check_device(struct dasd_device *device)
>  	struct dasd_diag_characteristics *rdc_data;
>  	struct vtoc_cms_label *label;
>  	struct dasd_block *block;
> -	struct dasd_diag_bio bio;
> +	struct dasd_diag_bio *bio;
>  	unsigned int sb, bsize;
>  	blocknum_t end_block;
>  	int rc;
> @@ -395,29 +395,36 @@ dasd_diag_check_device(struct dasd_device *device)
>  		rc = -ENOMEM;
>  		goto out;
>  	}
> +	bio = kzalloc(sizeof(*bio), GFP_KERNEL);
> +	if (bio == NULL)  {
> +		DBF_DEV_EVENT(DBF_WARNING, device, "%s",
> +			      "No memory to allocate initialization bio");
> +		rc = -ENOMEM;
> +		goto out_label;
> +	}

Just curious, any reason this isn't just using bio_alloc()?

-- 
Jens Axboe

Re: [PATCH 1/2] s390/dasd: fix inability to use DASD with DIAG driver

[Christoph Hellwig]

On Tue, Jul 14, 2020 at 02:12:27PM -0600, Jens Axboe wrote:
> Just curious, any reason this isn't just using bio_alloc()?

The dasd_diag_bio doesn't seem to have anything to do with the
block layer struct bio..

Re: [PATCH 1/2] s390/dasd: fix inability to use DASD with DIAG driver

[Stefan Haberland]

Am 15.07.20 um 08:48 schrieb Christoph Hellwig:
> On Tue, Jul 14, 2020 at 02:12:27PM -0600, Jens Axboe wrote:
>> Just curious, any reason this isn't just using bio_alloc()?
> The dasd_diag_bio doesn't seem to have anything to do with the
> block layer struct bio..

exactly, that's a struct dasd_diag_bio.

Re: [PATCH 1/2] s390/dasd: fix inability to use DASD with DIAG driver

[Jens Axboe]

On 7/15/20 7:32 AM, Stefan Haberland wrote:
> Am 15.07.20 um 08:48 schrieb Christoph Hellwig:
>> On Tue, Jul 14, 2020 at 02:12:27PM -0600, Jens Axboe wrote:
>>> Just curious, any reason this isn't just using bio_alloc()?
>> The dasd_diag_bio doesn't seem to have anything to do with the
>> block layer struct bio..
> 
> exactly, that's a struct dasd_diag_bio.

Duh, guess I should have looked a bit more closely...

-- 
Jens Axboe

Re: [PATCH 0/2] DASD DIAG patches

[Jens Axboe]

On 7/14/20 2:03 PM, Stefan Haberland wrote:
> Hi Jens,
> 
> please see the following two patches that fix and improve the DASD DIAG driver.
> 
> Regards,
> Stefan
> 
> Gustavo A. R. Silva (1):
>   s390/dasd: Use struct_size() helper
> 
> Stefan Haberland (1):
>   s390/dasd: fix inability to use DASD with DIAG driver
> 
>  drivers/s390/block/dasd_diag.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)

Applied, thanks.

-- 
Jens Axboe