[PATCH] scsi: don't panic host on invalid sgtable count

[PATCH] scsi: don't panic host on invalid sgtable count

[Johannes Thumshirn]

If we have an invalid number of entries mapped an sg table, there's no
need to panic the host, instead we can spit out a warning in dmesg and
gracefully return an I/O error.

While we're at it fix a trailing whitespace in the comment above.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/scsi/scsi_lib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 3e7a45d0daca..9bddf54e3def 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -992,12 +992,15 @@ static blk_status_t scsi_init_sgtable(struct request *req,
 			SCSI_INLINE_SG_CNT)))
 		return BLK_STS_RESOURCE;
 
-	/* 
+	/*
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
 	count = blk_rq_map_sg(req->q, req, sdb->table.sgl);
-	BUG_ON(count > sdb->table.nents);
+	if (WARN_ON_ONCE(count > sdb->table.nents)) {
+		sg_free_table_chained(&sdb->table, SCSI_INLINE_SG_CNT);
+		return BLK_STS_IOERR;
+	}
 	sdb->table.nents = count;
 	sdb->length = blk_rq_payload_bytes(req);
 	return BLK_STS_OK;
-- 
2.24.1

Re: [PATCH] scsi: don't panic host on invalid sgtable count

[James Bottomley]

On Sat, 2020-01-25 at 00:16 +0900, Johannes Thumshirn wrote:
> If we have an invalid number of entries mapped an sg table, there's
> no need to panic the host, instead we can spit out a warning in dmesg
> and gracefully return an I/O error.

Can we?  This is an assertion failure which should never happen.  If it
does, it's likely an indicator that a system has gone seriously out of
spec for some reason, like internal compromise, CPU/Memory failure or
something else.

The HA view is that panic is appropriate for conditions that should
never happen because it helps the machine fail fast.

James

> While we're at it fix a trailing whitespace in the comment above.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  drivers/scsi/scsi_lib.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 3e7a45d0daca..9bddf54e3def 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -992,12 +992,15 @@ static blk_status_t scsi_init_sgtable(struct
> request *req,
>  			SCSI_INLINE_SG_CNT)))
>  		return BLK_STS_RESOURCE;
>  
> -	/* 
> +	/*
>  	 * Next, walk the list, and fill in the addresses and sizes
> of
>  	 * each segment.
>  	 */
>  	count = blk_rq_map_sg(req->q, req, sdb->table.sgl);
> -	BUG_ON(count > sdb->table.nents);
> +	if (WARN_ON_ONCE(count > sdb->table.nents)) {
> +		sg_free_table_chained(&sdb->table,
> SCSI_INLINE_SG_CNT);
> +		return BLK_STS_IOERR;
> +	}
>  	sdb->table.nents = count;
>  	sdb->length = blk_rq_payload_bytes(req);
>  	return BLK_STS_OK;

Re: [PATCH] scsi: don't panic host on invalid sgtable count

[Johannes Thumshirn]

On 24/01/2020 16:23, James Bottomley wrote:
> On Sat, 2020-01-25 at 00:16 +0900, Johannes Thumshirn wrote:
>> If we have an invalid number of entries mapped an sg table, there's
>> no need to panic the host, instead we can spit out a warning in dmesg
>> and gracefully return an I/O error.
> 
> Can we?  This is an assertion failure which should never happen.  If it
> does, it's likely an indicator that a system has gone seriously out of
> spec for some reason, like internal compromise, CPU/Memory failure or
> something else.
> 
> The HA view is that panic is appropriate for conditions that should
> never happen because it helps the machine fail fast.

Yes but an HA setup could still set panic_on_oops and retain the fail 
fast portion.

Anyway it's just something that popped up when I was looking up 
something unrelated in scsi_lib.c. It's not that I'm married to this 
cleanup.