[PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c

[PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c

[Sergey Shtylyov]

The libata code too often uses the *unsigned long* type for the millisecond
timeouts, while the kernel functions like msecs_to_jiffies() or msleep() only
take *unsigned int* parameters for those. Start fixing the timeout types from
ata_exec_internal[_sg]() that triggered the SVACE static analyzer; there's
going to be a large continuation series somewhat later...

Sergey Shtylyov (2):
  ata: libata-core: make ata_exec_internal_sg() *static*
  ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

 drivers/ata/libata-core.c | 12 ++++++------
 drivers/ata/libata.h      |  6 +-----
 2 files changed, 7 insertions(+), 11 deletions(-)

-- 
2.26.3

[PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static*

[Sergey Shtylyov]

ata_exec_internal_sg() is only called by ata_exec_internal() further in
the same file, so we can make it *static* and remove its prototype from
drivers/ata/libata.h...

Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- new patch.

 drivers/ata/libata-core.c | 8 ++++----
 drivers/ata/libata.h      | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 980328a4b896..3cc1312a2622 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1467,10 +1467,10 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
  *	RETURNS:
  *	Zero on success, AC_ERR_* mask on failure
  */
-unsigned ata_exec_internal_sg(struct ata_device *dev,
-			      struct ata_taskfile *tf, const u8 *cdb,
-			      int dma_dir, struct scatterlist *sgl,
-			      unsigned int n_elem, unsigned long timeout)
+static unsigned ata_exec_internal_sg(struct ata_device *dev,
+				     struct ata_taskfile *tf, const u8 *cdb,
+				     int dma_dir, struct scatterlist *sgl,
+				     unsigned int n_elem, unsigned long timeout)
 {
 	struct ata_link *link = dev->link;
 	struct ata_port *ap = link->ap;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 926a7f41303d..1446a482835d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -53,10 +53,6 @@ extern unsigned ata_exec_internal(struct ata_device *dev,
 				  struct ata_taskfile *tf, const u8 *cdb,
 				  int dma_dir, void *buf, unsigned int buflen,
 				  unsigned long timeout);
-extern unsigned ata_exec_internal_sg(struct ata_device *dev,
-				     struct ata_taskfile *tf, const u8 *cdb,
-				     int dma_dir, struct scatterlist *sg,
-				     unsigned int n_elem, unsigned long timeout);
 extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
 			  int (*check_ready)(struct ata_link *link));
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
-- 
2.26.3

Re: [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static*

[Damien Le Moal]

On 6/16/22 04:38, Sergey Shtylyov wrote:
> ata_exec_internal_sg() is only called by ata_exec_internal() further in
> the same file, so we can make it *static* and remove its prototype from
> drivers/ata/libata.h...
> 
> Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> Changes in version 2:
> - new patch.
> 
>  drivers/ata/libata-core.c | 8 ++++----
>  drivers/ata/libata.h      | 4 ----
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 980328a4b896..3cc1312a2622 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1467,10 +1467,10 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>   *	RETURNS:
>   *	Zero on success, AC_ERR_* mask on failure
>   */
> -unsigned ata_exec_internal_sg(struct ata_device *dev,
> -			      struct ata_taskfile *tf, const u8 *cdb,
> -			      int dma_dir, struct scatterlist *sgl,
> -			      unsigned int n_elem, unsigned long timeout)
> +static unsigned ata_exec_internal_sg(struct ata_device *dev,
> +				     struct ata_taskfile *tf, const u8 *cdb,
> +				     int dma_dir, struct scatterlist *sgl,
> +				     unsigned int n_elem, unsigned long timeout)
>  {
>  	struct ata_link *link = dev->link;
>  	struct ata_port *ap = link->ap;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 926a7f41303d..1446a482835d 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -53,10 +53,6 @@ extern unsigned ata_exec_internal(struct ata_device *dev,
>  				  struct ata_taskfile *tf, const u8 *cdb,
>  				  int dma_dir, void *buf, unsigned int buflen,
>  				  unsigned long timeout);
> -extern unsigned ata_exec_internal_sg(struct ata_device *dev,
> -				     struct ata_taskfile *tf, const u8 *cdb,
> -				     int dma_dir, struct scatterlist *sg,
> -				     unsigned int n_elem, unsigned long timeout);
>  extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
>  			  int (*check_ready)(struct ata_link *link));
>  extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,

Applied this one only to for-5.20. Thanks !

-- 
Damien Le Moal
Western Digital Research

[PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Sergey Shtylyov]

Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
milliseconds. Then follow the suit with ata_exec_internal(), its only
caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
caller  that explicitly passes *unsigned long* variable for timeout...

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

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- rebased atop of the new patch #1.

 drivers/ata/libata-core.c | 6 +++---
 drivers/ata/libata.h      | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3cc1312a2622..03a08d1e666a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1470,7 +1470,7 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
 static unsigned ata_exec_internal_sg(struct ata_device *dev,
 				     struct ata_taskfile *tf, const u8 *cdb,
 				     int dma_dir, struct scatterlist *sgl,
-				     unsigned int n_elem, unsigned long timeout)
+				     unsigned int n_elem, unsigned int timeout)
 {
 	struct ata_link *link = dev->link;
 	struct ata_port *ap = link->ap;
@@ -1645,7 +1645,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
 unsigned ata_exec_internal(struct ata_device *dev,
 			   struct ata_taskfile *tf, const u8 *cdb,
 			   int dma_dir, void *buf, unsigned int buflen,
-			   unsigned long timeout)
+			   unsigned int timeout)
 {
 	struct scatterlist *psg = NULL, sg;
 	unsigned int n_elem = 0;
@@ -4342,7 +4342,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
-	unsigned long timeout = 0;
+	unsigned int timeout = 0;
 
 	/* set up set-features taskfile */
 	ata_dev_dbg(dev, "set features - SATA features\n");
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 1446a482835d..8292d4cdc22b 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -52,7 +52,7 @@ extern u64 ata_tf_read_block(const struct ata_taskfile *tf,
 extern unsigned ata_exec_internal(struct ata_device *dev,
 				  struct ata_taskfile *tf, const u8 *cdb,
 				  int dma_dir, void *buf, unsigned int buflen,
-				  unsigned long timeout);
+				  unsigned int timeout);
 extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
 			  int (*check_ready)(struct ata_link *link));
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
-- 
2.26.3

Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Damien Le Moal]

On 6/16/22 04:38, Sergey Shtylyov wrote:
> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
> milliseconds. Then follow the suit with ata_exec_internal(), its only
> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
> caller  that explicitly passes *unsigned long* variable for timeout...

Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
and ata_internal_cmd_timeout() returns an unsigned long which is stored
into the unsigned int timeout variable. So it may be good to add another
prep patch before this one to cleanup the auto_timeout stuff (struct
ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).

Hmm ? Thoughts ?

> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> Changes in version 2:
> - rebased atop of the new patch #1.
> 
>  drivers/ata/libata-core.c | 6 +++---
>  drivers/ata/libata.h      | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3cc1312a2622..03a08d1e666a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1470,7 +1470,7 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>  static unsigned ata_exec_internal_sg(struct ata_device *dev,
>  				     struct ata_taskfile *tf, const u8 *cdb,
>  				     int dma_dir, struct scatterlist *sgl,
> -				     unsigned int n_elem, unsigned long timeout)
> +				     unsigned int n_elem, unsigned int timeout)
>  {
>  	struct ata_link *link = dev->link;
>  	struct ata_port *ap = link->ap;
> @@ -1645,7 +1645,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
>  unsigned ata_exec_internal(struct ata_device *dev,
>  			   struct ata_taskfile *tf, const u8 *cdb,
>  			   int dma_dir, void *buf, unsigned int buflen,
> -			   unsigned long timeout)
> +			   unsigned int timeout)
>  {
>  	struct scatterlist *psg = NULL, sg;
>  	unsigned int n_elem = 0;
> @@ -4342,7 +4342,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> -	unsigned long timeout = 0;
> +	unsigned int timeout = 0;
>  
>  	/* set up set-features taskfile */
>  	ata_dev_dbg(dev, "set features - SATA features\n");
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 1446a482835d..8292d4cdc22b 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -52,7 +52,7 @@ extern u64 ata_tf_read_block(const struct ata_taskfile *tf,
>  extern unsigned ata_exec_internal(struct ata_device *dev,
>  				  struct ata_taskfile *tf, const u8 *cdb,
>  				  int dma_dir, void *buf, unsigned int buflen,
> -				  unsigned long timeout);
> +				  unsigned int timeout);
>  extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
>  			  int (*check_ready)(struct ata_link *link));
>  extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Sergey Shtylyov]

Hello!

On 6/17/22 10:44 AM, Damien Le Moal wrote:

>> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
>> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
>> milliseconds. Then follow the suit with ata_exec_internal(), its only
>> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
>> caller  that explicitly passes *unsigned long* variable for timeout...
> 
> Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
> and ata_internal_cmd_timeout() returns an unsigned long which is stored
> into the unsigned int timeout variable.

   I know. All timeouts stored in those tables fit into *unsigned int* (except
the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
   Note that *unsigned long* has always been silently truncated to *unsigned int*
when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
my patch, it just gets truncated a bit earlier...

> So it may be good to add another
> prep patch before this one to cleanup the auto_timeout stuff (struct
> ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).

   It shouldn't matter whether we do it before or after this patch, due to
truncation which happens even now (I have such a patch, but no description
yet). I was planning to address that as a part of the large patch series,
most probably next week (and I wrote about that in the cover letter)...

> Hmm ? Thoughts ?

   Was I clear enough? :-)

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

   I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
typing...

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

MBR, Sergey

Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Damien Le Moal]

On 6/18/22 02:27, Sergey Shtylyov wrote:
> Hello!
> 
> On 6/17/22 10:44 AM, Damien Le Moal wrote:
> 
>>> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
>>> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
>>> milliseconds. Then follow the suit with ata_exec_internal(), its only
>>> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
>>> caller  that explicitly passes *unsigned long* variable for timeout...
>>
>> Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
>> and ata_internal_cmd_timeout() returns an unsigned long which is stored
>> into the unsigned int timeout variable.
> 
>    I know. All timeouts stored in those tables fit into *unsigned int* (except
> the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
>    Note that *unsigned long* has always been silently truncated to *unsigned int*
> when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
> my patch, it just gets truncated a bit earlier...
> 
>> So it may be good to add another
>> prep patch before this one to cleanup the auto_timeout stuff (struct
>> ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).
> 
>    It shouldn't matter whether we do it before or after this patch, due to
> truncation which happens even now (I have such a patch, but no description
> yet). I was planning to address that as a part of the large patch series,
> most probably next week (and I wrote about that in the cover letter)...
> 
>> Hmm ? Thoughts ?
> 
>    Was I clear enough? :-)
> 
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
> 
>    I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
> typing...

Fine. But in the spirit of fixing everything with these types, please add
a patch to also convert the timeout tables to unsigned int. It is never a
good idea to have code silently truncate long to int as that can be a pain
to debug. So let's go all the way and fix this.

If you cannot do the additional patch, I will do it.

> 
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Sergey Shtylyov]

Hello!

On 6/18/22 9:45 AM, Damien Le Moal wrote:
[...]
>>>> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
>>>> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
>>>> milliseconds. Then follow the suit with ata_exec_internal(), its only
>>>> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
>>>> caller  that explicitly passes *unsigned long* variable for timeout...
>>>
>>> Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
>>> and ata_internal_cmd_timeout() returns an unsigned long which is stored
>>> into the unsigned int timeout variable.
>>
>>    I know. All timeouts stored in those tables fit into *unsigned int* (except
>> the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
>>    Note that *unsigned long* has always been silently truncated to *unsigned int*
>> when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
>> my patch, it just gets truncated a bit earlier...
>>
>>> So it may be good to add another
>>> prep patch before this one to cleanup the auto_timeout stuff (struct
>>> ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).
>>
>>    It shouldn't matter whether we do it before or after this patch, due to
>> truncation which happens even now (I have such a patch, but no description
>> yet). I was planning to address that as a part of the large patch series,
>> most probably next week (and I wrote about that in the cover letter)...
>>
>>> Hmm ? Thoughts ?
>>
>>    Was I clear enough? :-)
>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>>> analysis tool.
>>
>>    I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
>> typing...
> 
> Fine. But in the spirit of fixing everything with these types, please add
> a patch to also convert the timeout tables to unsigned int. It is never a
> good idea to have code silently truncate long to int

   I even wonder why C allows that... and the static analyzers don't seem to
complain about that too...

> as that can be a pain
> to debug. So let's go all the way and fix this.

   OK.

> If you cannot do the additional patch, I will do it.

   I can. I have the patch already, just without the description yet...

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

MBR, Sergey

Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Damien Le Moal]

On 6/16/22 04:38, Sergey Shtylyov wrote:
> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
> milliseconds. Then follow the suit with ata_exec_internal(), its only
> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
> caller  that explicitly passes *unsigned long* variable for timeout...
> 
> 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. Thanks.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c

[Sergey Shtylyov]

On 6/15/22 10:38 PM, Sergey Shtylyov wrote:

> The libata code too often uses the *unsigned long* type for the millisecond
> timeouts, while the kernel functions like msecs_to_jiffies() or msleep() only
> take *unsigned int* parameters for those. Start fixing the timeout types from
> ata_exec_internal[_sg]() that triggered the SVACE static analyzer; there's
> going to be a large continuation series somewhat later...
> 
> Sergey Shtylyov (2):
>   ata: libata-core: make ata_exec_internal_sg() *static*
>   ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
> 
>  drivers/ata/libata-core.c | 12 ++++++------
>  drivers/ata/libata.h      |  6 +-----
>  2 files changed, 7 insertions(+), 11 deletions(-)

  Forgot to mention the patches are against the 'for-next' branch of Damien's
'libata.git' repo... :-/

MBR, Sergey