| Message ID | 1e088333-7c7e-8e7b-7b85-12edd4355b92@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: libata-scsi: fix result type of ata_ioc32() | expand |
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
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; > } > > /*
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; } /*
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(-)