* [PATCH 0/2] ipc: allocations cleanup [not found] <e67f2a95-4b01-9db2-fe47-0b2210f0b138@virtuozzo.com> @ 2021-04-26 10:18 ` Vasily Averin 2021-04-28 7:35 ` [PATCH v2 " Vasily Averin ` (2 more replies) 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2 siblings, 3 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-26 10:18 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov Some ipc objects use the wrong allocation functions: small objects can use kmalloc(), and vice versa, potentially large objects can use kmalloc(). I think it's better to handle these patches via cgroups@ to avoid merge conflicts with my patches that include accounting for ipc objects. Vasily Averin (2): ipc sem: use kvmalloc for sem_undo allocation ipc: use kmalloc for msg_queue and shmid_kernel ipc/msg.c | 6 +++--- ipc/sem.c | 10 +++++----- ipc/shm.c | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 0/2] ipc: allocations cleanup 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin @ 2021-04-28 7:35 ` Vasily Averin 2021-04-28 7:35 ` [PATCH v2 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-28 7:35 ` [PATCH v2 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2 siblings, 0 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-28 7:35 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov Some ipc objects use the wrong allocation functions: small objects can use kmalloc(), and vice versa, potentially large objects can use kmalloc(). I think it's better to handle these patches via cgroups@ to avoid merge conflicts with "memcg: enable accounting of ipc resources" patch included to "memcg accounting from OpenVZ" patch set. v2: - improved patch description Vasily Averin (2): ipc sem: use kvmalloc for sem_undo allocation ipc: use kmalloc for msg_queue and shmid_kernel ipc/msg.c | 6 +++--- ipc/sem.c | 10 +++++----- ipc/shm.c | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] ipc sem: use kvmalloc for sem_undo allocation 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin 2021-04-28 7:35 ` [PATCH v2 " Vasily Averin @ 2021-04-28 7:35 ` Vasily Averin 2021-04-28 7:35 ` [PATCH v2 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2 siblings, 0 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-28 7:35 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov size of sem_undo can exceed one page and with the maximum possible nsems = 32000 it can grow up to 64Kb. Let's switch its allocation to kvmalloc to avoid user-triggered disruptive actions like OOM killer in case of high-order memory shortage. User triggerable high order allocations are quite a problem on heavily fragmented systems. They can be a DoS vector. Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Acked-by: Michal Hocko <mhocko@suse.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> Acked-by: Roman Gushchin <guro@fb.com> --- ipc/sem.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 52a6599..93088d6 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1152,7 +1152,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) un->semid = -1; list_del_rcu(&un->list_proc); spin_unlock(&un->ulp->lock); - kfree_rcu(un, rcu); + kvfree_rcu(un, rcu); } /* Wake up all pending processes and let them fail with EIDRM. */ @@ -1935,7 +1935,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) rcu_read_unlock(); /* step 2: allocate new undo structure */ - new = kzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, + new = kvzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, GFP_KERNEL_ACCOUNT); if (!new) { ipc_rcu_putref(&sma->sem_perm, sem_rcu_free); @@ -1948,7 +1948,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) if (!ipc_valid_object(&sma->sem_perm)) { sem_unlock(sma, -1); rcu_read_unlock(); - kfree(new); + kvfree(new); un = ERR_PTR(-EIDRM); goto out; } @@ -1959,7 +1959,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) */ un = lookup_undo(ulp, semid); if (un) { - kfree(new); + kvfree(new); goto success; } /* step 5: initialize & link new undo structure */ @@ -2420,7 +2420,7 @@ void exit_sem(struct task_struct *tsk) rcu_read_unlock(); wake_up_q(&wake_q); - kfree_rcu(un, rcu); + kvfree_rcu(un, rcu); } kfree(ulp); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin 2021-04-28 7:35 ` [PATCH v2 " Vasily Averin 2021-04-28 7:35 ` [PATCH v2 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin @ 2021-04-28 7:35 ` Vasily Averin 2 siblings, 0 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-28 7:35 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov msg_queue and shmid_kernel are quite small objects, no need to use kvmalloc for them. mhocko@: "Both of them are 256B on most 64b systems." Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(), common function for several ipc objects. It had kvmalloc call inside(). Later, this function went away and was finally replaced by direct kvmalloc call, and now we can use more suitable kmalloc/kfree for them. Reported-by: Alexey Dobriyan <adobriyan@gmail.com> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Acked-by: Michal Hocko <mhocko@suse.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> Acked-by: Roman Gushchin <guro@fb.com> --- ipc/msg.c | 6 +++--- ipc/shm.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index 87898cb..79c6625 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -130,7 +130,7 @@ static void msg_rcu_free(struct rcu_head *head) struct msg_queue *msq = container_of(p, struct msg_queue, q_perm); security_msg_queue_free(&msq->q_perm); - kvfree(msq); + kfree(msq); } /** @@ -147,7 +147,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) key_t key = params->key; int msgflg = params->flg; - msq = kvmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); + msq = kmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); if (unlikely(!msq)) return -ENOMEM; @@ -157,7 +157,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) msq->q_perm.security = NULL; retval = security_msg_queue_alloc(&msq->q_perm); if (retval) { - kvfree(msq); + kfree(msq); return retval; } diff --git a/ipc/shm.c b/ipc/shm.c index 7632d72..85da060 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -222,7 +222,7 @@ static void shm_rcu_free(struct rcu_head *head) struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel, shm_perm); security_shm_free(&shp->shm_perm); - kvfree(shp); + kfree(shp); } static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s) @@ -619,7 +619,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) ns->shm_tot + numpages > ns->shm_ctlall) return -ENOSPC; - shp = kvmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); + shp = kmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); if (unlikely(!shp)) return -ENOMEM; @@ -630,7 +630,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) shp->shm_perm.security = NULL; error = security_shm_alloc(&shp->shm_perm); if (error) { - kvfree(shp); + kfree(shp); return error; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation [not found] <e67f2a95-4b01-9db2-fe47-0b2210f0b138@virtuozzo.com> 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin @ 2021-04-26 10:18 ` Vasily Averin 2021-04-26 10:28 ` Michal Hocko ` (2 more replies) 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2 siblings, 3 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-26 10:18 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov size of sem_undo can exceed one page and with the maximum possible nsems = 32000 it can grow up to 64Kb. Let's switch its allocation to kvmalloc to avoid user-triggered disruptive actions like OOM killer in case of high-order memory shortage. Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- ipc/sem.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 52a6599..93088d6 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1152,7 +1152,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) un->semid = -1; list_del_rcu(&un->list_proc); spin_unlock(&un->ulp->lock); - kfree_rcu(un, rcu); + kvfree_rcu(un, rcu); } /* Wake up all pending processes and let them fail with EIDRM. */ @@ -1935,7 +1935,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) rcu_read_unlock(); /* step 2: allocate new undo structure */ - new = kzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, + new = kvzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, GFP_KERNEL_ACCOUNT); if (!new) { ipc_rcu_putref(&sma->sem_perm, sem_rcu_free); @@ -1948,7 +1948,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) if (!ipc_valid_object(&sma->sem_perm)) { sem_unlock(sma, -1); rcu_read_unlock(); - kfree(new); + kvfree(new); un = ERR_PTR(-EIDRM); goto out; } @@ -1959,7 +1959,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) */ un = lookup_undo(ulp, semid); if (un) { - kfree(new); + kvfree(new); goto success; } /* step 5: initialize & link new undo structure */ @@ -2420,7 +2420,7 @@ void exit_sem(struct task_struct *tsk) rcu_read_unlock(); wake_up_q(&wake_q); - kfree_rcu(un, rcu); + kvfree_rcu(un, rcu); } kfree(ulp); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin @ 2021-04-26 10:28 ` Michal Hocko 2021-04-26 16:22 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2021-04-26 10:28 UTC (permalink / raw) To: Vasily Averin Cc: cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon 26-04-21 13:18:09, Vasily Averin wrote: > size of sem_undo can exceed one page and with the maximum possible > nsems = 32000 it can grow up to 64Kb. Let's switch its allocation > to kvmalloc to avoid user-triggered disruptive actions like OOM killer > in case of high-order memory shortage. User triggerable high order allocations are quite a problem on heavily fragmented systems. They can be a DoS vector. > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > ipc/sem.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index 52a6599..93088d6 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -1152,7 +1152,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) > un->semid = -1; > list_del_rcu(&un->list_proc); > spin_unlock(&un->ulp->lock); > - kfree_rcu(un, rcu); > + kvfree_rcu(un, rcu); > } > > /* Wake up all pending processes and let them fail with EIDRM. */ > @@ -1935,7 +1935,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) > rcu_read_unlock(); > > /* step 2: allocate new undo structure */ > - new = kzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, > + new = kvzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, > GFP_KERNEL_ACCOUNT); > if (!new) { > ipc_rcu_putref(&sma->sem_perm, sem_rcu_free); > @@ -1948,7 +1948,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) > if (!ipc_valid_object(&sma->sem_perm)) { > sem_unlock(sma, -1); > rcu_read_unlock(); > - kfree(new); > + kvfree(new); > un = ERR_PTR(-EIDRM); > goto out; > } > @@ -1959,7 +1959,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) > */ > un = lookup_undo(ulp, semid); > if (un) { > - kfree(new); > + kvfree(new); > goto success; > } > /* step 5: initialize & link new undo structure */ > @@ -2420,7 +2420,7 @@ void exit_sem(struct task_struct *tsk) > rcu_read_unlock(); > wake_up_q(&wake_q); > > - kfree_rcu(un, rcu); > + kvfree_rcu(un, rcu); > } > kfree(ulp); > } > -- > 1.8.3.1 -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-26 10:28 ` Michal Hocko @ 2021-04-26 16:22 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 0 replies; 14+ messages in thread From: Shakeel Butt @ 2021-04-26 16:22 UTC (permalink / raw) To: Vasily Averin Cc: Michal Hocko, Cgroups, LKML, Alexey Dobriyan, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon, Apr 26, 2021 at 3:18 AM Vasily Averin <vvs@virtuozzo.com> wrote: > > size of sem_undo can exceed one page and with the maximum possible > nsems = 32000 it can grow up to 64Kb. Let's switch its allocation > to kvmalloc to avoid user-triggered disruptive actions like OOM killer > in case of high-order memory shortage. > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-26 10:28 ` Michal Hocko 2021-04-26 16:22 ` Shakeel Butt @ 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 0 replies; 14+ messages in thread From: Roman Gushchin @ 2021-04-26 20:29 UTC (permalink / raw) To: Vasily Averin Cc: Michal Hocko, cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon, Apr 26, 2021 at 01:18:09PM +0300, Vasily Averin wrote: > size of sem_undo can exceed one page and with the maximum possible > nsems = 32000 it can grow up to 64Kb. Let's switch its allocation > to kvmalloc to avoid user-triggered disruptive actions like OOM killer > in case of high-order memory shortage. > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Acked-by: Roman Gushchin <guro@fb.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel [not found] <e67f2a95-4b01-9db2-fe47-0b2210f0b138@virtuozzo.com> 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin @ 2021-04-26 10:18 ` Vasily Averin 2021-04-26 10:25 ` Michal Hocko ` (2 more replies) 2 siblings, 3 replies; 14+ messages in thread From: Vasily Averin @ 2021-04-26 10:18 UTC (permalink / raw) To: Michal Hocko, cgroups Cc: linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov msg_queue and shmid_kernel are quite small objects, no need to use kvmalloc for them. Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(), common function for several ipc objects. It had kvmalloc call inside(). Later, this function went away and was finally replaced by direct kvmalloc call, and now we can use more suitable kmalloc/kfree for them. Reported-by: Alexey Dobriyan <adobriyan@gmail.com> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- ipc/msg.c | 6 +++--- ipc/shm.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ipc/msg.c b/ipc/msg.c index 87898cb..79c6625 100644 --- a/ipc/msg.c +++ b/ipc/msg.c @@ -130,7 +130,7 @@ static void msg_rcu_free(struct rcu_head *head) struct msg_queue *msq = container_of(p, struct msg_queue, q_perm); security_msg_queue_free(&msq->q_perm); - kvfree(msq); + kfree(msq); } /** @@ -147,7 +147,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) key_t key = params->key; int msgflg = params->flg; - msq = kvmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); + msq = kmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); if (unlikely(!msq)) return -ENOMEM; @@ -157,7 +157,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) msq->q_perm.security = NULL; retval = security_msg_queue_alloc(&msq->q_perm); if (retval) { - kvfree(msq); + kfree(msq); return retval; } diff --git a/ipc/shm.c b/ipc/shm.c index 7632d72..85da060 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -222,7 +222,7 @@ static void shm_rcu_free(struct rcu_head *head) struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel, shm_perm); security_shm_free(&shp->shm_perm); - kvfree(shp); + kfree(shp); } static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s) @@ -619,7 +619,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) ns->shm_tot + numpages > ns->shm_ctlall) return -ENOSPC; - shp = kvmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); + shp = kmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); if (unlikely(!shp)) return -ENOMEM; @@ -630,7 +630,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) shp->shm_perm.security = NULL; error = security_shm_alloc(&shp->shm_perm); if (error) { - kvfree(shp); + kfree(shp); return error; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin @ 2021-04-26 10:25 ` Michal Hocko 2021-04-28 5:15 ` Vasily Averin 2021-04-26 16:23 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 1 reply; 14+ messages in thread From: Michal Hocko @ 2021-04-26 10:25 UTC (permalink / raw) To: Vasily Averin Cc: cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon 26-04-21 13:18:14, Vasily Averin wrote: > msg_queue and shmid_kernel are quite small objects, no need to use > kvmalloc for them. Both of them are 256B on most 64b systems. > Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(), > common function for several ipc objects. It had kvmalloc call inside(). > Later, this function went away and was finally replaced by direct > kvmalloc call, and now we can use more suitable kmalloc/kfree for them. This describes the history of the code but it doesn't really explain why kvmalloc is a bad decision here. I would go with the following: " Using kvmalloc for sub page size objects is suboptimal because kmalloc can easily fallback into vmalloc under memory pressure and smaller objects would fragment memory. Therefore replace kvmalloc by a simple kmalloc. " > > Reported-by: Alexey Dobriyan <adobriyan@gmail.com> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> With a clarified changelog, feel free to add Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > ipc/msg.c | 6 +++--- > ipc/shm.c | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/ipc/msg.c b/ipc/msg.c > index 87898cb..79c6625 100644 > --- a/ipc/msg.c > +++ b/ipc/msg.c > @@ -130,7 +130,7 @@ static void msg_rcu_free(struct rcu_head *head) > struct msg_queue *msq = container_of(p, struct msg_queue, q_perm); > > security_msg_queue_free(&msq->q_perm); > - kvfree(msq); > + kfree(msq); > } > > /** > @@ -147,7 +147,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) > key_t key = params->key; > int msgflg = params->flg; > > - msq = kvmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); > + msq = kmalloc(sizeof(*msq), GFP_KERNEL_ACCOUNT); > if (unlikely(!msq)) > return -ENOMEM; > > @@ -157,7 +157,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params) > msq->q_perm.security = NULL; > retval = security_msg_queue_alloc(&msq->q_perm); > if (retval) { > - kvfree(msq); > + kfree(msq); > return retval; > } > > diff --git a/ipc/shm.c b/ipc/shm.c > index 7632d72..85da060 100644 > --- a/ipc/shm.c > +++ b/ipc/shm.c > @@ -222,7 +222,7 @@ static void shm_rcu_free(struct rcu_head *head) > struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel, > shm_perm); > security_shm_free(&shp->shm_perm); > - kvfree(shp); > + kfree(shp); > } > > static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s) > @@ -619,7 +619,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) > ns->shm_tot + numpages > ns->shm_ctlall) > return -ENOSPC; > > - shp = kvmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); > + shp = kmalloc(sizeof(*shp), GFP_KERNEL_ACCOUNT); > if (unlikely(!shp)) > return -ENOMEM; > > @@ -630,7 +630,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) > shp->shm_perm.security = NULL; > error = security_shm_alloc(&shp->shm_perm); > if (error) { > - kvfree(shp); > + kfree(shp); > return error; > } > > -- > 1.8.3.1 -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-26 10:25 ` Michal Hocko @ 2021-04-28 5:15 ` Vasily Averin 2021-04-28 6:33 ` Michal Hocko 0 siblings, 1 reply; 14+ messages in thread From: Vasily Averin @ 2021-04-28 5:15 UTC (permalink / raw) To: Michal Hocko Cc: cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On 4/26/21 1:25 PM, Michal Hocko wrote: > Using kvmalloc for sub page size objects is suboptimal because kmalloc > can easily fallback into vmalloc under memory pressure and smaller > objects would fragment memory. Therefore replace kvmalloc by a simple > kmalloc. I think you're wrong here: kvmalloc can failback to vmalloc for size > PAGE_SIZE only Please take look at mm/util.c::kvmalloc_node() if (size > PAGE_SIZE) { kmalloc_flags |= __GFP_NOWARN; if (!(kmalloc_flags & __GFP_RETRY_MAYFAIL)) kmalloc_flags |= __GFP_NORETRY; } ret = kmalloc_node(size, kmalloc_flags, node); /* * It doesn't really make sense to fallback to vmalloc for sub page * requests */ if (ret || size <= PAGE_SIZE) return ret; return __vmalloc_node(size, 1, flags, node, __builtin_return_address(0)); For small objects kvmalloc is not much different just from kmalloc, so the patch is mostly cosmetic. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-28 5:15 ` Vasily Averin @ 2021-04-28 6:33 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2021-04-28 6:33 UTC (permalink / raw) To: Vasily Averin Cc: cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Wed 28-04-21 08:15:10, Vasily Averin wrote: > On 4/26/21 1:25 PM, Michal Hocko wrote: > > Using kvmalloc for sub page size objects is suboptimal because kmalloc > > can easily fallback into vmalloc under memory pressure and smaller > > objects would fragment memory. Therefore replace kvmalloc by a simple > > kmalloc. > > I think you're wrong here: > kvmalloc can failback to vmalloc for size > PAGE_SIZE only You are right. My bad. My memory failed on me. Sorry about the confusion. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2021-04-26 10:25 ` Michal Hocko @ 2021-04-26 16:23 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 0 replies; 14+ messages in thread From: Shakeel Butt @ 2021-04-26 16:23 UTC (permalink / raw) To: Vasily Averin Cc: Michal Hocko, Cgroups, LKML, Alexey Dobriyan, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon, Apr 26, 2021 at 3:18 AM Vasily Averin <vvs@virtuozzo.com> wrote: > > msg_queue and shmid_kernel are quite small objects, no need to use > kvmalloc for them. > Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(), > common function for several ipc objects. It had kvmalloc call inside(). > Later, this function went away and was finally replaced by direct > kvmalloc call, and now we can use more suitable kmalloc/kfree for them. > > Reported-by: Alexey Dobriyan <adobriyan@gmail.com> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2021-04-26 10:25 ` Michal Hocko 2021-04-26 16:23 ` Shakeel Butt @ 2021-04-26 20:29 ` Roman Gushchin 2 siblings, 0 replies; 14+ messages in thread From: Roman Gushchin @ 2021-04-26 20:29 UTC (permalink / raw) To: Vasily Averin Cc: Michal Hocko, cgroups, linux-kernel, Alexey Dobriyan, Shakeel Butt, Johannes Weiner, Vladimir Davydov, Andrew Morton, Dmitry Safonov On Mon, Apr 26, 2021 at 01:18:14PM +0300, Vasily Averin wrote: > msg_queue and shmid_kernel are quite small objects, no need to use > kvmalloc for them. > Previously these objects was allocated via ipc_alloc/ipc_rcu_alloc(), > common function for several ipc objects. It had kvmalloc call inside(). > Later, this function went away and was finally replaced by direct > kvmalloc call, and now we can use more suitable kmalloc/kfree for them. > > Reported-by: Alexey Dobriyan <adobriyan@gmail.com> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Acked-by: Roman Gushchin <guro@fb.com> Thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-04-28 7:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <e67f2a95-4b01-9db2-fe47-0b2210f0b138@virtuozzo.com> 2021-04-26 10:18 ` [PATCH 0/2] ipc: allocations cleanup Vasily Averin 2021-04-28 7:35 ` [PATCH v2 " Vasily Averin 2021-04-28 7:35 ` [PATCH v2 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-28 7:35 ` [PATCH v2 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2021-04-26 10:18 ` [PATCH 1/2] ipc sem: use kvmalloc for sem_undo allocation Vasily Averin 2021-04-26 10:28 ` Michal Hocko 2021-04-26 16:22 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin 2021-04-26 10:18 ` [PATCH 2/2] ipc: use kmalloc for msg_queue and shmid_kernel Vasily Averin 2021-04-26 10:25 ` Michal Hocko 2021-04-28 5:15 ` Vasily Averin 2021-04-28 6:33 ` Michal Hocko 2021-04-26 16:23 ` Shakeel Butt 2021-04-26 20:29 ` Roman Gushchin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).