[PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[Chengguang Xu]

sizeof() will return unsigned value so in the error check
negative error code will be always larger than sizeof().

Fixes: a0d8e02c35ff ("nfp: add support for reading nffw info")

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
v2:
- Add more information to patch subject and commit log.

 drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c
index cd34097b79f1..37a6d7822a38 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c
@@ -232,7 +232,7 @@ struct nfp_nffw_info *nfp_nffw_info_open(struct nfp_cpp *cpp)
 	err = nfp_cpp_read(cpp, nfp_resource_cpp_id(state->res),
 			   nfp_resource_address(state->res),
 			   fwinf, sizeof(*fwinf));
-	if (err < sizeof(*fwinf))
+	if (err < (int)sizeof(*fwinf))
 		goto err_release;
 
 	if (!nffw_res_flg_init_get(fwinf))
-- 
2.17.1

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[Joe Perches]

On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> sizeof() will return unsigned value so in the error check
> negative error code will be always larger than sizeof().

This looks like a general class of error in the kernel
where a signed result that could be returning a -errno
is tested against < or <= sizeof()

A couple examples:

drivers/input/mouse/elan_i2c_smbus.c:

		len = i2c_smbus_read_block_data(client,
						ETP_SMBUS_IAP_PASSWORD_READ,
						val);
		if (len < sizeof(u16)) {

i2c_smbus_read_block_data can return a negative errno


net/smc/smc_clc.c:

	len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
			     sizeof(struct smc_clc_msg_decline));
	if (len < sizeof(struct smc_clc_msg_decline))

where kernel_sendmsg can return a negative errno

There are probably others, I didn't look hard.

Perhaps a cocci script to find these could be generated?

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[Julia Lawall]

On Mon, 25 Jun 2018, Joe Perches wrote:

> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> > sizeof() will return unsigned value so in the error check
> > negative error code will be always larger than sizeof().
>
> This looks like a general class of error in the kernel
> where a signed result that could be returning a -errno
> is tested against < or <= sizeof()
>
> A couple examples:
>
> drivers/input/mouse/elan_i2c_smbus.c:
>
> 		len = i2c_smbus_read_block_data(client,
> 						ETP_SMBUS_IAP_PASSWORD_READ,
> 						val);
> 		if (len < sizeof(u16)) {
>
> i2c_smbus_read_block_data can return a negative errno
>
>
> net/smc/smc_clc.c:
>
> 	len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> 			     sizeof(struct smc_clc_msg_decline));
> 	if (len < sizeof(struct smc_clc_msg_decline))
>
> where kernel_sendmsg can return a negative errno
>
> There are probably others, I didn't look hard.
>
> Perhaps a cocci script to find these could be generated?

Currently there is a rule for comparison of unsigneds to 0.  It would be
reasonable to extend it for sizes.  I will see what it gives.

julia

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[Julia Lawall]

On Mon, 25 Jun 2018, Joe Perches wrote:

> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> > sizeof() will return unsigned value so in the error check
> > negative error code will be always larger than sizeof().
>
> This looks like a general class of error in the kernel
> where a signed result that could be returning a -errno
> is tested against < or <= sizeof()
>
> A couple examples:
>
> drivers/input/mouse/elan_i2c_smbus.c:
>
> 		len = i2c_smbus_read_block_data(client,
> 						ETP_SMBUS_IAP_PASSWORD_READ,
> 						val);
> 		if (len < sizeof(u16)) {
>
> i2c_smbus_read_block_data can return a negative errno
>
>
> net/smc/smc_clc.c:
>
> 	len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> 			     sizeof(struct smc_clc_msg_decline));
> 	if (len < sizeof(struct smc_clc_msg_decline))
>
> where kernel_sendmsg can return a negative errno
>
> There are probably others, I didn't look hard.
>
> Perhaps a cocci script to find these could be generated?

Here's another one:

drivers/usb/serial/ir-usb.c
@@ -126,13 +126,8 @@ irda_usb_find_class_desc(struct usb_seri
 	if (!desc)
 		return NULL;

-	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-			USB_REQ_CS_IRDA_GET_CLASS_DESC,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			0, ifnum, desc, sizeof(*desc), 1000);

 	dev_dbg(&serial->dev->dev, "%s -  ret=%d\n", __func__, ret);
-	if (ret < sizeof(*desc)) {
 		dev_dbg(&serial->dev->dev,
 			"%s - class descriptor read %s (%d)\n", __func__,
 			(ret < 0) ? "failed" : "too short", ret);

There are other results, but I haven't checked all of them.

julia

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[cgxu519]

On 06/26/2018 04:06 PM, Julia Lawall wrote:
>
> On Mon, 25 Jun 2018, Joe Perches wrote:
>
>> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
>>> sizeof() will return unsigned value so in the error check
>>> negative error code will be always larger than sizeof().
>> This looks like a general class of error in the kernel
>> where a signed result that could be returning a -errno
>> is tested against < or <= sizeof()
>>
>> A couple examples:
>>
>> drivers/input/mouse/elan_i2c_smbus.c:
>>
>> 		len = i2c_smbus_read_block_data(client,
>> 						ETP_SMBUS_IAP_PASSWORD_READ,
>> 						val);
>> 		if (len < sizeof(u16)) {
>>
>> i2c_smbus_read_block_data can return a negative errno
>>
>>
>> net/smc/smc_clc.c:
>>
>> 	len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
>> 			     sizeof(struct smc_clc_msg_decline));
>> 	if (len < sizeof(struct smc_clc_msg_decline))
>>
>> where kernel_sendmsg can return a negative errno
>>
>> There are probably others, I didn't look hard.
>>
>> Perhaps a cocci script to find these could be generated?
> Here's another one:
>
> drivers/usb/serial/ir-usb.c
> @@ -126,13 +126,8 @@ irda_usb_find_class_desc(struct usb_seri
>   	if (!desc)
>   		return NULL;
>
> -	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
> -			USB_REQ_CS_IRDA_GET_CLASS_DESC,
> -			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -			0, ifnum, desc, sizeof(*desc), 1000);
>
>   	dev_dbg(&serial->dev->dev, "%s -  ret=%d\n", __func__, ret);
> -	if (ret < sizeof(*desc)) {
>   		dev_dbg(&serial->dev->dev,
>   			"%s - class descriptor read %s (%d)\n", __func__,
>   			(ret < 0) ? "failed" : "too short", ret);
>
> There are other results, but I haven't checked all of them.

Hi Julia,

Thanks for your check. I posted a patch yesterday to fix three places in 
usb subsystem
and the patch is just in queue now, so you can skip these places.

The detail of patch.
---

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 24b06c7e5e2d..7643716b5299 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -132,7 +132,7 @@ irda_usb_find_class_desc(struct usb_serial *serial, unsigned int ifnum)
  			0, ifnum, desc, sizeof(*desc), 1000);
  
  	dev_dbg(&serial->dev->dev, "%s -  ret=%d\n", __func__, ret);
-	if (ret < sizeof(*desc)) {
+	if (ret < (int)sizeof(*desc)) {
  		dev_dbg(&serial->dev->dev,
  			"%s - class descriptor read %s (%d)\n", __func__,
  			(ret < 0) ? "failed" : "too short", ret);
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index 958e12e1e7c7..ff2322ea5e14 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -194,7 +194,7 @@ static inline int qt2_getregister(struct usb_device *dev,
  	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
  			      QT_SET_GET_REGISTER, 0xc0, reg,
  			      uart, data, sizeof(*data), QT2_USB_TIMEOUT);
-	if (ret < sizeof(*data)) {
+	if (ret < (int)sizeof(*data)) {
  		if (ret >= 0)
  			ret = -EIO;
  	}
diff --git a/drivers/usb/serial/ssu100.c b/drivers/usb/serial/ssu100.c
index 2083c267787b..0900b47b5f57 100644
--- a/drivers/usb/serial/ssu100.c
+++ b/drivers/usb/serial/ssu100.c
@@ -104,7 +104,7 @@ static inline int ssu100_getregister(struct usb_device *dev,
  	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
  			      QT_SET_GET_REGISTER, 0xc0, reg,
  			      uart, data, sizeof(*data), 300);
-	if (ret < sizeof(*data)) {
+	if (ret < (int)sizeof(*data)) {
  		if (ret >= 0)
  			ret = -EIO;
  	}
---




Thanks,
Chengguang.

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[Joe Perches]

On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
> sizeof() will return unsigned value so in the error check
> negative error code will be always larger than sizeof().
[]
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c
[]
> @@ -232,7 +232,7 @@ struct nfp_nffw_info *nfp_nffw_info_open(struct nfp_cpp *cpp)
>  	err = nfp_cpp_read(cpp, nfp_resource_cpp_id(state->res),
>  			   nfp_resource_address(state->res),
>  			   fwinf, sizeof(*fwinf));
> -	if (err < sizeof(*fwinf))
> +	if (err < (int)sizeof(*fwinf))
>  		goto err_release;
>  
>  	if (!nffw_res_flg_init_get(fwinf))

The way this is done in several places in the kernel is
to test first for < 0 and then test for < sizeof

	if (err < 0 || err < sizeof(etc...)

see net/ceph/ceph_common.c etc...

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[cgxu519]

On 06/26/2018 10:29 AM, Joe Perches wrote:
> On Tue, 2018-06-26 at 09:16 +0800, Chengguang Xu wrote:
>> sizeof() will return unsigned value so in the error check
>> negative error code will be always larger than sizeof().
> []
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nffw.c
> []
>> @@ -232,7 +232,7 @@ struct nfp_nffw_info *nfp_nffw_info_open(struct nfp_cpp *cpp)
>>   	err = nfp_cpp_read(cpp, nfp_resource_cpp_id(state->res),
>>   			   nfp_resource_address(state->res),
>>   			   fwinf, sizeof(*fwinf));
>> -	if (err < sizeof(*fwinf))
>> +	if (err < (int)sizeof(*fwinf))
>>   		goto err_release;
>>   
>>   	if (!nffw_res_flg_init_get(fwinf))
> The way this is done in several places in the kernel is
> to test first for < 0 and then test for < sizeof
>
> 	if (err < 0 || err < sizeof(etc...)
>
> see net/ceph/ceph_common.c etc...
If we need to distinguish the cases <0 and >0 && <sizeof() then that 
approach is better.
If not I think cast to int will be enough.

Thanks,
Chengguang.

Re: [PATCH v2 net] nfp: cast sizeof() to int when comparing with error code

[David Miller]

From: Chengguang Xu <cgxu519@gmx.com>
Date: Tue, 26 Jun 2018 09:16:31 +0800

> sizeof() will return unsigned value so in the error check
> negative error code will be always larger than sizeof().
> 
> Fixes: a0d8e02c35ff ("nfp: add support for reading nffw info")
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> v2:
> - Add more information to patch subject and commit log.

I guess the:

	if (x < 0 || x < sizeof(foo))

better self-documents the situation, but this patch is fine too
so I have applied it.

Thanks.