[PATCH] tcp: use kmalloc() than kmalloc_array().

[PATCH] tcp: use kmalloc() than kmalloc_array().

[Tetsuo Handa]

Commit 095dc8e0c3686d58 ("tcp: fix/cleanup inet_ehash_locks_alloc()")
silently changed from kmalloc() to kmalloc_array(). The latter has
overflow check whereas the former doesn't have.

If nblocks * locksz might overflow, we need to do like

  -  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
  +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
       hashinfo->ehash_locks = vmalloc(nblocks * locksz);

because kmalloc_array() detects overflow and returns NULL.
But if nblocks * locksz is guaranteed not to overflow, there is
no need to use kmalloc_array().

Since I assume it won't overflow, use kmalloc() than kmalloc_array().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/ipv4/inet_hashtables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ccc5980..8f4ab27 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -648,8 +648,8 @@ int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
 		/* no more locks than number of hash buckets */
 		nblocks = min(nblocks, hashinfo->ehash_mask + 1);
 
-		hashinfo->ehash_locks =	kmalloc_array(nblocks, locksz,
-						      GFP_KERNEL | __GFP_NOWARN);
+		hashinfo->ehash_locks =	kmalloc(nblocks * locksz,
+						GFP_KERNEL | __GFP_NOWARN);
 		if (!hashinfo->ehash_locks)
 			hashinfo->ehash_locks = vmalloc(nblocks * locksz);
 
-- 
1.8.3.1

Re: [PATCH] tcp: use kmalloc() than kmalloc_array().

[Tetsuo Handa]

Tetsuo Handa wrote:
> Commit 095dc8e0c3686d58 ("tcp: fix/cleanup inet_ehash_locks_alloc()")
> silently changed from kmalloc() to kmalloc_array(). The latter has
> overflow check whereas the former doesn't have.
> 
> If nblocks * locksz might overflow, we need to do like
> 
>   -  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
>   +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)

Oops, I meant

   -  if (!hashinfo->ehash_locks)
   +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)

here.

>        hashinfo->ehash_locks = vmalloc(nblocks * locksz);
> 
> because kmalloc_array() detects overflow and returns NULL.
> But if nblocks * locksz is guaranteed not to overflow, there is
> no need to use kmalloc_array().
> 
> Since I assume it won't overflow, use kmalloc() than kmalloc_array().

I don't know about possible value range.
Please confirm that it can't overflow.

Re: [PATCH] tcp: use kmalloc() than kmalloc_array().

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sun, 8 Nov 2015 00:58:50 +0900

> Tetsuo Handa wrote:
>> Commit 095dc8e0c3686d58 ("tcp: fix/cleanup inet_ehash_locks_alloc()")
>> silently changed from kmalloc() to kmalloc_array(). The latter has
>> overflow check whereas the former doesn't have.
>> 
>> If nblocks * locksz might overflow, we need to do like
>> 
>>   -  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
>>   +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
> 
> Oops, I meant
> 
>    -  if (!hashinfo->ehash_locks)
>    +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
> 
> here.
> 
>>        hashinfo->ehash_locks = vmalloc(nblocks * locksz);
>> 
>> because kmalloc_array() detects overflow and returns NULL.
>> But if nblocks * locksz is guaranteed not to overflow, there is
>> no need to use kmalloc_array().
>> 
>> Since I assume it won't overflow, use kmalloc() than kmalloc_array().
> 
> I don't know about possible value range.
> Please confirm that it can't overflow.

Whether it can overflow or not, I don't like your change at all.

If kmalloc_array() provides overflow protection, we want to keep using
it, rather than reverting back to not checking for overflow.

kmalloc_array() can fail for us at this stage for two reasons, either:

1) overflow

2) allocation size exceeds kmalloc() maximum alloc size

so if you want to be completely perfect about all of this you need
to add code to distinguish between these two cases as you suggest
above.

Re: [PATCH] tcp: use kmalloc() than kmalloc_array().

[Eric Dumazet]

On Sun, 2015-11-08 at 00:50 +0900, Tetsuo Handa wrote:
> Commit 095dc8e0c3686d58 ("tcp: fix/cleanup inet_ehash_locks_alloc()")
> silently changed from kmalloc() to kmalloc_array(). The latter has
> overflow check whereas the former doesn't have.
> 
> If nblocks * locksz might overflow, we need to do like
> 
>   -  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
>   +  if (!hashinfo->ehash_locks && nblocks > SIZE_MAX / locksz)
>        hashinfo->ehash_locks = vmalloc(nblocks * locksz);
> 
> because kmalloc_array() detects overflow and returns NULL.
> But if nblocks * locksz is guaranteed not to overflow, there is
> no need to use kmalloc_array().
> 
> Since I assume it won't overflow, use kmalloc() than kmalloc_array().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  net/ipv4/inet_hashtables.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index ccc5980..8f4ab27 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -648,8 +648,8 @@ int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
>  		/* no more locks than number of hash buckets */
>  		nblocks = min(nblocks, hashinfo->ehash_mask + 1);
>  
> -		hashinfo->ehash_locks =	kmalloc_array(nblocks, locksz,
> -						      GFP_KERNEL | __GFP_NOWARN);
> +		hashinfo->ehash_locks =	kmalloc(nblocks * locksz,
> +						GFP_KERNEL | __GFP_NOWARN);
>  		if (!hashinfo->ehash_locks)
>  			hashinfo->ehash_locks = vmalloc(nblocks * locksz);
>  

I remember that my initial attempt had been to use size_t for nblocks,
but I realized roundup_pow_of_two() only accepted an 'unsigned long'

Then, presumably I just gave up.

I do not feel we should go back to kmalloc() just because
vmalloc_array() does not exist yet.

Maybe the following would clear things for you guys ?

If it is OK, please Tetsuo submit this patch formally.

Thanks !

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index ccc5980797fc..8f7c71e20089 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -638,15 +638,15 @@ EXPORT_SYMBOL_GPL(inet_hashinfo_init);
 int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
 {
 	unsigned int locksz = sizeof(spinlock_t);
-	unsigned int i, nblocks = 1;
+	size_t i, nblocks = 1;
 
 	if (locksz != 0) {
 		/* allocate 2 cache lines or at least one spinlock per cpu */
-		nblocks = max(2U * L1_CACHE_BYTES / locksz, 1U);
+		nblocks = max_t(size_t, 2 * L1_CACHE_BYTES / locksz, 1);
 		nblocks = roundup_pow_of_two(nblocks * num_possible_cpus());
 
 		/* no more locks than number of hash buckets */
-		nblocks = min(nblocks, hashinfo->ehash_mask + 1);
+		nblocks = min_t(size_t, nblocks, hashinfo->ehash_mask + 1);
 
 		hashinfo->ehash_locks =	kmalloc_array(nblocks, locksz,
 						      GFP_KERNEL | __GFP_NOWARN);

Re: [PATCH] tcp: use kmalloc() than kmalloc_array().

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 07 Nov 2015 10:50:07 -0800

> I do not feel we should go back to kmalloc() just because
> vmalloc_array() does not exist yet.

Agreed.

Re: [PATCH] tcp: use kmalloc() than kmalloc_array().

[Tetsuo Handa]

David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 07 Nov 2015 10:50:07 -0800
> 
> > I do not feel we should go back to kmalloc() just because
> > vmalloc_array() does not exist yet.
> 
> Agreed.
> 

Please change as you like.

I was thinking to introduce a helper that does vmalloc() when
kmalloc() failed because locations that do

  ptr = kmalloc(size, GFP_NOFS);
  if (!ptr)
      ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */

are found. I just noticed that inet_ehash_locks_alloc() is doing

  ptr = kmalloc_array(count, size, GFP_KERNEL);
  if (!ptr)
      ptr = vmalloc(count * size); /* Wrong because overflow is not checked */

and wanted to know your intent.