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

[PATCH] 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 -- fix that by returning
'bool' instead (actually, the object code doesn't seem to change, probably
because the function is always inlined).

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 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] ata: libata-scsi: fix result type of ata_ioc32()

[Damien Le Moal]

On 6/23/22 05:57, 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 -- fix that by returning
> 'bool' instead (actually, the object code doesn't seem to change, probably
> because the function is always inlined).

Looks good. I would though prefer to change this commit message to simply
say that ata_ioc32() return value is clearly a bool. The implicit cast to
unsigned long from int is not really an issue at all (the reverse cast
would be an issue). And keep mentioning that the object code does not change.

By the way, does your statis analyzer stop complaining after this change ?
Because we still have an implicit cast in the user site.

> 
> 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 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

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

[Sergey Shtylyov]

Hello!

On 6/23/22 2:45 AM, Damien Le Moal 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 -- fix that by returning
>> 'bool' instead (actually, the object code doesn't seem to change, probably
>> because the function is always inlined).
> 
> Looks good. I would though prefer to change this commit message to simply
> say that ata_ioc32() return value is clearly a bool.

   No, just changing *int* to 'bool' wasn't the purpose of this patch --
I would not have called it a fix if it was so.

> The implicit cast to
> unsigned long from int is not really an issue at all (the reverse cast

   In general case, it is an issue -- as it involves a sign extension (and
thus a needless extra instruction on an x86_64 kernel)! However, with the
possible *int* values just being 0 and 1, it's not much of issue indeed
(except an extra instruction just being there)...  In reality , that extra
instruction is not there, probably due to ata_ioc32() being inlined...

> would be an issue). And keep mentioning that the object code does not change.
> 
> By the way, does your statis analyzer stop complaining after this change ?

   I don't really know -- all I have is the online report generated for the
whole 5.10 kernel. I still can't re-run SVACE but maybe that will change soon...

> Because we still have an implicit cast in the user site.

   A cast from 'bool' should be OK... :-)

>> 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] ata: libata-scsi: fix result type of ata_ioc32()

[Damien Le Moal]

On 6/24/22 03:33, Sergey Shtylyov wrote:
> Hello!
> 
> On 6/23/22 2:45 AM, Damien Le Moal 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 -- fix that by returning
>>> 'bool' instead (actually, the object code doesn't seem to change, probably
>>> because the function is always inlined).
>>
>> Looks good. I would though prefer to change this commit message to simply
>> say that ata_ioc32() return value is clearly a bool.
> 
>    No, just changing *int* to 'bool' wasn't the purpose of this patch --
> I would not have called it a fix if it was so.
> 
>> The implicit cast to
>> unsigned long from int is not really an issue at all (the reverse cast
> 
>    In general case, it is an issue -- as it involves a sign extension (and
> thus a needless extra instruction on an x86_64 kernel)! However, with the
> possible *int* values just being 0 and 1, it's not much of issue indeed
> (except an extra instruction just being there)...  In reality , that extra
> instruction is not there, probably due to ata_ioc32() being inlined...
> 
>> would be an issue). And keep mentioning that the object code does not change.
>>
>> By the way, does your statis analyzer stop complaining after this change ?
> 
>    I don't really know -- all I have is the online report generated for the
> whole 5.10 kernel. I still can't re-run SVACE but maybe that will change soon...
> 
>> Because we still have an implicit cast in the user site.
> 
>    A cast from 'bool' should be OK... :-)

Yes, I am aware of that. My point is that the commit message does not say
WHY it is OK. Need to mention that the cast is between unsigned types so
is fine, or something like that.

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


-- 
Damien Le Moal
Western Digital Research

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

[Sergey Shtylyov]

On 6/24/22 1:52 AM, Damien Le Moal 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 -- fix that by returning
>>>> 'bool' instead (actually, the object code doesn't seem to change, probably
>>>> because the function is always inlined).
>>>
>>> Looks good. I would though prefer to change this commit message to simply
>>> say that ata_ioc32() return value is clearly a bool.
>>
>>    No, just changing *int* to 'bool' wasn't the purpose of this patch --
>> I would not have called it a fix if it was so.
>>
>>> The implicit cast to
>>> unsigned long from int is not really an issue at all (the reverse cast
>>
>>    In general case, it is an issue -- as it involves a sign extension (and
>> thus a needless extra instruction on an x86_64 kernel)! However, with the
>> possible *int* values just being 0 and 1, it's not much of issue indeed
>> (except an extra instruction just being there)...  In reality , that extra
>> instruction is not there, probably due to ata_ioc32() being inlined...

   Yes, inlining is to blame here -- luckily we have 'noinline'! :-)

>>> would be an issue). And keep mentioning that the object code does not change.
>>>
>>> By the way, does your statis analyzer stop complaining after this change ?
>>
>>    I don't really know -- all I have is the online report generated for the
>> whole 5.10 kernel. I still can't re-run SVACE but maybe that will change soon...
>>
>>> Because we still have an implicit cast in the user site.
>>
>>    A cast from 'bool' should be OK... :-)
> 
> Yes, I am aware of that. My point is that the commit message does not say
> WHY it is OK. Need to mention that the cast is between unsigned types so
> is fine, or something like that.

   OK, I'll try to come up with something... didn't quite expect that this
patch would be so tough to get merged... :-)

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

MBR, Sergey