[PATCH v3] ata: libata-scsi: fix result type of ata_ioc32()

[PATCH v3] ata: libata-scsi: fix result type of ata_ioc32()

[Sergey Shtylyov]

While ata_ioc32() returns 'int', its result gets assigned to and compared
with the 'unsigned long' variable 'val' in ata_sas_scsi_ioctl(), its only
caller, which implies a problematic implicit cast (with sign extension).
Fix this by returning 'bool' instead -- the implicit cast then implies
zero extension which is OK.  Note that actually the object code doesn't
change because ata_ioc32() is always inlined -- I can see the expected
code changes with 'noinline')...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'for-next' branch of Damien's 'libata.git' repo.

Changes in version 3:
- refined the patch description.

Changes in version 2:
- changed the result type of ata_ioc32() to 'bool', updating the 'return'
  statements as well;
- dropped "sloppy" from the patch subject;
- added a note about the object code to the patch description;
- changed * to ' in the patch description.

 drivers/ata/libata-scsi.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: libata/drivers/ata/libata-scsi.c
===================================================================
--- libata.orig/drivers/ata/libata-scsi.c
+++ libata/drivers/ata/libata-scsi.c
@@ -539,13 +539,13 @@ int ata_task_ioctl(struct scsi_device *s
 	return rc;
 }
 
-static int ata_ioc32(struct ata_port *ap)
+static bool ata_ioc32(struct ata_port *ap)
 {
 	if (ap->flags & ATA_FLAG_PIO_DMA)
-		return 1;
+		return true;
 	if (ap->pflags & ATA_PFLAG_PIO32)
-		return 1;
-	return 0;
+		return true;
+	return false;
 }
 
 /*

Re: [PATCH v3] ata: libata-scsi: fix result type of ata_ioc32()

[Sergey Shtylyov]

On 6/24/22 11:39 PM, Sergey Shtylyov wrote:

> While ata_ioc32() returns 'int', its result gets assigned to and compared
> with the 'unsigned long' variable 'val' in ata_sas_scsi_ioctl(), its only
> caller, which implies a problematic implicit cast (with sign extension).
> Fix this by returning 'bool' instead -- the implicit cast then implies
> zero extension which is OK.  Note that actually the object code doesn't
> change because ata_ioc32() is always inlined -- I can see the expected
> code changes with 'noinline')...

   Leftover paren, could this be fixed while applying?

> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH v3] ata: libata-scsi: fix result type of ata_ioc32()

[Damien Le Moal]

On 6/25/22 05:39, Sergey Shtylyov wrote:
> While ata_ioc32() returns 'int', its result gets assigned to and compared
> with the 'unsigned long' variable 'val' in ata_sas_scsi_ioctl(), its only
> caller, which implies a problematic implicit cast (with sign extension).
> Fix this by returning 'bool' instead -- the implicit cast then implies
> zero extension which is OK.  Note that actually the object code doesn't
> change because ata_ioc32() is always inlined -- I can see the expected
> code changes with 'noinline')...
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Applied to for-5.20 with the commit message fix. Thanks !

> 
> ---
> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
> 
> Changes in version 3:
> - refined the patch description.
> 
> Changes in version 2:
> - changed the result type of ata_ioc32() to 'bool', updating the 'return'
>   statements as well;
> - dropped "sloppy" from the patch subject;
> - added a note about the object code to the patch description;
> - changed * to ' in the patch description.
> 
>  drivers/ata/libata-scsi.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Index: libata/drivers/ata/libata-scsi.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-scsi.c
> +++ libata/drivers/ata/libata-scsi.c
> @@ -539,13 +539,13 @@ int ata_task_ioctl(struct scsi_device *s
>  	return rc;
>  }
>  
> -static int ata_ioc32(struct ata_port *ap)
> +static bool ata_ioc32(struct ata_port *ap)
>  {
>  	if (ap->flags & ATA_FLAG_PIO_DMA)
> -		return 1;
> +		return true;
>  	if (ap->pflags & ATA_PFLAG_PIO32)
> -		return 1;
> -	return 0;
> +		return true;
> +	return false;
>  }
>  
>  /*


-- 
Damien Le Moal
Western Digital Research