diff mbox series

[net-next,1/8] atm: use min() to simplify the code

Message ID 20240822133908.1042240-2-lizetao1@huawei.com (mailing list archive)
State New, archived
Headers show
Series Some modifications to optimize code readability | expand

Commit Message

Li Zetao Aug. 22, 2024, 1:39 p.m. UTC
When copying data to user, it needs to determine the copy length.
It is easier to understand using min() here.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/atm/addr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dr. David Alan Gilbert Aug. 22, 2024, 1:39 p.m. UTC | #1
* Li Zetao (lizetao1@huawei.com) wrote:
> When copying data to user, it needs to determine the copy length.
> It is easier to understand using min() here.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/atm/addr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/atm/addr.c b/net/atm/addr.c
> index 0530b63f509a..6c4c942b2cb9 100644
> --- a/net/atm/addr.c
> +++ b/net/atm/addr.c
> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>  	unsigned long flags;
>  	struct atm_dev_addr *this;
>  	struct list_head *head;
> -	int total = 0, error;
> +	size_t total = 0, error;

Aren't you accidentally changing the type of 'error' there, and the function
returns 'int'.

Dave


>  	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>  
>  	spin_lock_irqsave(&dev->lock, flags);
> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>  	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  	error = total > size ? -E2BIG : total;
> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>  		error = -EFAULT;
>  	kfree(tmp_buf);
>  	return error;
> -- 
> 2.34.1
>
Li Zetao Aug. 22, 2024, 1:50 p.m. UTC | #2
Hi,

在 2024/8/22 21:39, Dr. David Alan Gilbert 写道:
> * Li Zetao (lizetao1@huawei.com) wrote:
>> When copying data to user, it needs to determine the copy length.
>> It is easier to understand using min() here.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   net/atm/addr.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/atm/addr.c b/net/atm/addr.c
>> index 0530b63f509a..6c4c942b2cb9 100644
>> --- a/net/atm/addr.c
>> +++ b/net/atm/addr.c
>> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>   	unsigned long flags;
>>   	struct atm_dev_addr *this;
>>   	struct list_head *head;
>> -	int total = 0, error;
>> +	size_t total = 0, error;
> 
> Aren't you accidentally changing the type of 'error' there, and the function
> returns 'int'.
This is intentionally modified because the input parameter size is of 
type size_t. If total is of type int, the compiler will report an error 
when the min() is called.
> 
> Dav
> 
> 
>>   	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>>   
>>   	spin_lock_irqsave(&dev->lock, flags);
>> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>   	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>>   	spin_unlock_irqrestore(&dev->lock, flags);
>>   	error = total > size ? -E2BIG : total;
>> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
>> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>>   		error = -EFAULT;
>>   	kfree(tmp_buf);
>>   	return error;
>> -- 
>> 2.34.1
>>

Thanks,
Li Zetao.
Jacob Keller Aug. 22, 2024, 8:20 p.m. UTC | #3
On 8/22/2024 6:50 AM, Li Zetao wrote:
> Hi,
> 
> 在 2024/8/22 21:39, Dr. David Alan Gilbert 写道:
>> * Li Zetao (lizetao1@huawei.com) wrote:
>>> When copying data to user, it needs to determine the copy length.
>>> It is easier to understand using min() here.
>>>
>>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>>> ---
>>>   net/atm/addr.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/atm/addr.c b/net/atm/addr.c
>>> index 0530b63f509a..6c4c942b2cb9 100644
>>> --- a/net/atm/addr.c
>>> +++ b/net/atm/addr.c
>>> @@ -136,7 +136,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>>   	unsigned long flags;
>>>   	struct atm_dev_addr *this;
>>>   	struct list_head *head;
>>> -	int total = 0, error;
>>> +	size_t total = 0, error;
>>
>> Aren't you accidentally changing the type of 'error' there, and the function
>> returns 'int'.
> This is intentionally modified because the input parameter size is of 
> type size_t. If total is of type int, the compiler will report an error 
> when the min() is called.
>>

Yea, but what you're missing is that error was an int before and is now
a size_t which can't be negative.

I think this either needs to be:

size_t total = 0;
int error

or better yet....

>> Dav
>>
>>
>>>   	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
>>>   
>>>   	spin_lock_irqsave(&dev->lock, flags);
>>> @@ -155,7 +155,7 @@ int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
>>>   	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
>>>   	spin_unlock_irqrestore(&dev->lock, flags);
>>>   	error = total > size ? -E2BIG : total;
>>> -	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
>>> +	if (copy_to_user(buf, tmp_buf, min(total, size)))
>>>   		error = -EFAULT;


Couldn't you just use min_t here instead of changing the variable sizes?

Thanks,
Jake
diff mbox series

Patch

diff --git a/net/atm/addr.c b/net/atm/addr.c
index 0530b63f509a..6c4c942b2cb9 100644
--- a/net/atm/addr.c
+++ b/net/atm/addr.c
@@ -136,7 +136,7 @@  int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
 	unsigned long flags;
 	struct atm_dev_addr *this;
 	struct list_head *head;
-	int total = 0, error;
+	size_t total = 0, error;
 	struct sockaddr_atmsvc *tmp_buf, *tmp_bufp;
 
 	spin_lock_irqsave(&dev->lock, flags);
@@ -155,7 +155,7 @@  int atm_get_addr(struct atm_dev *dev, struct sockaddr_atmsvc __user * buf,
 	    memcpy(tmp_bufp++, &this->addr, sizeof(struct sockaddr_atmsvc));
 	spin_unlock_irqrestore(&dev->lock, flags);
 	error = total > size ? -E2BIG : total;
-	if (copy_to_user(buf, tmp_buf, total < size ? total : size))
+	if (copy_to_user(buf, tmp_buf, min(total, size)))
 		error = -EFAULT;
 	kfree(tmp_buf);
 	return error;