* [PATCH] tcp: use kmalloc() than kmalloc_array().
@ 2015-11-07 15:50 Tetsuo Handa
2015-11-07 15:58 ` Tetsuo Handa
2015-11-07 18:50 ` Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Tetsuo Handa @ 2015-11-07 15:50 UTC (permalink / raw)
To: edumazet; +Cc: netdev, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tcp: use kmalloc() than kmalloc_array(). 2015-11-07 15:50 [PATCH] tcp: use kmalloc() than kmalloc_array() Tetsuo Handa @ 2015-11-07 15:58 ` Tetsuo Handa 2015-11-07 18:02 ` David Miller 2015-11-07 18:50 ` Eric Dumazet 1 sibling, 1 reply; 6+ messages in thread From: Tetsuo Handa @ 2015-11-07 15:58 UTC (permalink / raw) To: edumazet; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: use kmalloc() than kmalloc_array(). 2015-11-07 15:58 ` Tetsuo Handa @ 2015-11-07 18:02 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2015-11-07 18:02 UTC (permalink / raw) To: penguin-kernel; +Cc: edumazet, netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: use kmalloc() than kmalloc_array(). 2015-11-07 15:50 [PATCH] tcp: use kmalloc() than kmalloc_array() Tetsuo Handa 2015-11-07 15:58 ` Tetsuo Handa @ 2015-11-07 18:50 ` Eric Dumazet 2015-11-07 19:21 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2015-11-07 18:50 UTC (permalink / raw) To: Tetsuo Handa; +Cc: edumazet, netdev 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: use kmalloc() than kmalloc_array(). 2015-11-07 18:50 ` Eric Dumazet @ 2015-11-07 19:21 ` David Miller 2015-11-08 4:43 ` Tetsuo Handa 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2015-11-07 19:21 UTC (permalink / raw) To: eric.dumazet; +Cc: penguin-kernel, edumazet, netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: use kmalloc() than kmalloc_array(). 2015-11-07 19:21 ` David Miller @ 2015-11-08 4:43 ` Tetsuo Handa 0 siblings, 0 replies; 6+ messages in thread From: Tetsuo Handa @ 2015-11-08 4:43 UTC (permalink / raw) To: davem, eric.dumazet; +Cc: edumazet, netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-08 4:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-07 15:50 [PATCH] tcp: use kmalloc() than kmalloc_array() Tetsuo Handa 2015-11-07 15:58 ` Tetsuo Handa 2015-11-07 18:02 ` David Miller 2015-11-07 18:50 ` Eric Dumazet 2015-11-07 19:21 ` David Miller 2015-11-08 4:43 ` Tetsuo Handa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.