* [patch] mm, oom: remove task_lock protecting comm printing
@ 2015-09-22 23:30 David Rientjes
2015-09-23 7:44 ` Michal Hocko
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: David Rientjes @ 2015-09-22 23:30 UTC (permalink / raw)
To: Andrew Morton, Oleg Nesterov
Cc: Michal Hocko, Linus Torvalds, Kyle Walker, Christoph Lameter,
Johannes Weiner, Vladimir Davydov, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
The oom killer takes task_lock() in a couple of places solely to protect
printing the task's comm.
A process's comm, including current's comm, may change due to
/proc/pid/comm or PR_SET_NAME.
The comm will always be NULL-terminated, so the worst race scenario would
only be during update. We can tolerate a comm being printed that is in
the middle of an update to avoid taking the lock.
Other locations in the kernel have already dropped task_lock() when
printing comm, so this is consistent.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
include/linux/cpuset.h | 4 ++--
kernel/cpuset.c | 14 +++++++-------
mm/oom_kill.c | 8 +-------
3 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -93,7 +93,7 @@ extern int current_cpuset_is_being_rebound(void);
extern void rebuild_sched_domains(void);
-extern void cpuset_print_task_mems_allowed(struct task_struct *p);
+extern void cpuset_print_current_mems_allowed(void);
/*
* read_mems_allowed_begin is required when making decisions involving
@@ -219,7 +219,7 @@ static inline void rebuild_sched_domains(void)
partition_sched_domains(1, NULL, NULL);
}
-static inline void cpuset_print_task_mems_allowed(struct task_struct *p)
+static inline void cpuset_print_current_mems_allowed(void)
{
}
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2599,22 +2599,22 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
}
/**
- * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
- * @tsk: pointer to task_struct of some task.
+ * cpuset_print_current_mems_allowed - prints current's cpuset and mems_allowed
*
- * Description: Prints @task's name, cpuset name, and cached copy of its
+ * Description: Prints current's name, cpuset name, and cached copy of its
* mems_allowed to the kernel log.
*/
-void cpuset_print_task_mems_allowed(struct task_struct *tsk)
+void cpuset_print_current_mems_allowed(void)
{
struct cgroup *cgrp;
rcu_read_lock();
- cgrp = task_cs(tsk)->css.cgroup;
- pr_info("%s cpuset=", tsk->comm);
+ cgrp = task_cs(current)->css.cgroup;
+ pr_info("%s cpuset=", current->comm);
pr_cont_cgroup_name(cgrp);
- pr_cont(" mems_allowed=%*pbl\n", nodemask_pr_args(&tsk->mems_allowed));
+ pr_cont(" mems_allowed=%*pbl\n",
+ nodemask_pr_args(¤t->mems_allowed));
rcu_read_unlock();
}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -386,13 +386,11 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask)
static void dump_header(struct oom_control *oc, struct task_struct *p,
struct mem_cgroup *memcg)
{
- task_lock(current);
pr_warning("%s invoked oom-killer: gfp_mask=0x%x, order=%d, "
"oom_score_adj=%hd\n",
current->comm, oc->gfp_mask, oc->order,
current->signal->oom_score_adj);
- cpuset_print_task_mems_allowed(current);
- task_unlock(current);
+ cpuset_print_current_mems_allowed();
dump_stack();
if (memcg)
mem_cgroup_print_oom_info(memcg, p);
@@ -518,10 +516,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
if (__ratelimit(&oom_rs))
dump_header(oc, p, memcg);
- task_lock(p);
pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
message, task_pid_nr(p), p->comm, points);
- task_unlock(p);
/*
* If any of p's children has a different mm and is eligible for kill,
@@ -586,10 +582,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
continue;
- task_lock(p); /* Protect ->comm from prctl() */
pr_err("Kill process %d (%s) sharing same memory\n",
task_pid_nr(p), p->comm);
- task_unlock(p);
do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
}
rcu_read_unlock();
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-22 23:30 [patch] mm, oom: remove task_lock protecting comm printing David Rientjes
@ 2015-09-23 7:44 ` Michal Hocko
2015-09-23 8:06 ` Vladimir Davydov
2015-09-24 19:45 ` Johannes Weiner
2 siblings, 0 replies; 11+ messages in thread
From: Michal Hocko @ 2015-09-23 7:44 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Oleg Nesterov, Linus Torvalds, Kyle Walker,
Christoph Lameter, Johannes Weiner, Vladimir Davydov, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
On Tue 22-09-15 16:30:13, David Rientjes wrote:
> The oom killer takes task_lock() in a couple of places solely to protect
> printing the task's comm.
>
> A process's comm, including current's comm, may change due to
> /proc/pid/comm or PR_SET_NAME.
>
> The comm will always be NULL-terminated, so the worst race scenario would
> only be during update. We can tolerate a comm being printed that is in
> the middle of an update to avoid taking the lock.
>
> Other locations in the kernel have already dropped task_lock() when
> printing comm, so this is consistent.
cpuset_print_task_mems_allowed seems unrelated and it would probably
deserve a separate pach.
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: David Rientjes <rientjes@google.com>
For the task_lock part
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> include/linux/cpuset.h | 4 ++--
> kernel/cpuset.c | 14 +++++++-------
> mm/oom_kill.c | 8 +-------
> 3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -93,7 +93,7 @@ extern int current_cpuset_is_being_rebound(void);
>
> extern void rebuild_sched_domains(void);
>
> -extern void cpuset_print_task_mems_allowed(struct task_struct *p);
> +extern void cpuset_print_current_mems_allowed(void);
>
> /*
> * read_mems_allowed_begin is required when making decisions involving
> @@ -219,7 +219,7 @@ static inline void rebuild_sched_domains(void)
> partition_sched_domains(1, NULL, NULL);
> }
>
> -static inline void cpuset_print_task_mems_allowed(struct task_struct *p)
> +static inline void cpuset_print_current_mems_allowed(void)
> {
> }
>
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -2599,22 +2599,22 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
> }
>
> /**
> - * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
> - * @tsk: pointer to task_struct of some task.
> + * cpuset_print_current_mems_allowed - prints current's cpuset and mems_allowed
> *
> - * Description: Prints @task's name, cpuset name, and cached copy of its
> + * Description: Prints current's name, cpuset name, and cached copy of its
> * mems_allowed to the kernel log.
> */
> -void cpuset_print_task_mems_allowed(struct task_struct *tsk)
> +void cpuset_print_current_mems_allowed(void)
> {
> struct cgroup *cgrp;
>
> rcu_read_lock();
>
> - cgrp = task_cs(tsk)->css.cgroup;
> - pr_info("%s cpuset=", tsk->comm);
> + cgrp = task_cs(current)->css.cgroup;
> + pr_info("%s cpuset=", current->comm);
> pr_cont_cgroup_name(cgrp);
> - pr_cont(" mems_allowed=%*pbl\n", nodemask_pr_args(&tsk->mems_allowed));
> + pr_cont(" mems_allowed=%*pbl\n",
> + nodemask_pr_args(¤t->mems_allowed));
>
> rcu_read_unlock();
> }
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -386,13 +386,11 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask)
> static void dump_header(struct oom_control *oc, struct task_struct *p,
> struct mem_cgroup *memcg)
> {
> - task_lock(current);
> pr_warning("%s invoked oom-killer: gfp_mask=0x%x, order=%d, "
> "oom_score_adj=%hd\n",
> current->comm, oc->gfp_mask, oc->order,
> current->signal->oom_score_adj);
> - cpuset_print_task_mems_allowed(current);
> - task_unlock(current);
> + cpuset_print_current_mems_allowed();
> dump_stack();
> if (memcg)
> mem_cgroup_print_oom_info(memcg, p);
> @@ -518,10 +516,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> if (__ratelimit(&oom_rs))
> dump_header(oc, p, memcg);
>
> - task_lock(p);
> pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
> message, task_pid_nr(p), p->comm, points);
> - task_unlock(p);
>
> /*
> * If any of p's children has a different mm and is eligible for kill,
> @@ -586,10 +582,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> continue;
>
> - task_lock(p); /* Protect ->comm from prctl() */
> pr_err("Kill process %d (%s) sharing same memory\n",
> task_pid_nr(p), p->comm);
> - task_unlock(p);
> do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
> }
> rcu_read_unlock();
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-22 23:30 [patch] mm, oom: remove task_lock protecting comm printing David Rientjes
2015-09-23 7:44 ` Michal Hocko
@ 2015-09-23 8:06 ` Vladimir Davydov
2015-09-23 9:13 ` Sergey Senozhatsky
2015-09-24 19:45 ` Johannes Weiner
2 siblings, 1 reply; 11+ messages in thread
From: Vladimir Davydov @ 2015-09-23 8:06 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Oleg Nesterov, Michal Hocko, Linus Torvalds,
Kyle Walker, Christoph Lameter, Johannes Weiner, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
Hi,
On Tue, Sep 22, 2015 at 04:30:13PM -0700, David Rientjes wrote:
> The oom killer takes task_lock() in a couple of places solely to protect
> printing the task's comm.
>
> A process's comm, including current's comm, may change due to
> /proc/pid/comm or PR_SET_NAME.
>
> The comm will always be NULL-terminated, so the worst race scenario would
> only be during update. We can tolerate a comm being printed that is in
> the middle of an update to avoid taking the lock.
>
> Other locations in the kernel have already dropped task_lock() when
> printing comm, so this is consistent.
Without the protection, can't reading task->comm race with PR_SET_NAME
as described below?
Let T->comm[16] = "name\0rubbish1234"
CPU1 CPU2
---- ----
set_task_comm(T, "longname\0")
T->comm[0] = 'l'
T->comm[1] = 'o'
T->comm[2] = 'n'
T->comm[3] = 'g'
T->comm[4] = 'n'
printk("%s\n", T->comm)
T->comm = "longnrubbish1234"
OOPS: the string is not
nil-terminated!
T->comm[5] = 'a'
T->comm[6] = 'm'
T->comm[7] = 'e'
T->comm[8] = '\0'
Thanks,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 8:06 ` Vladimir Davydov
@ 2015-09-23 9:13 ` Sergey Senozhatsky
2015-09-23 9:30 ` Vladimir Davydov
0 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2015-09-23 9:13 UTC (permalink / raw)
To: Vladimir Davydov
Cc: David Rientjes, Andrew Morton, Oleg Nesterov, Michal Hocko,
Linus Torvalds, Kyle Walker, Christoph Lameter, Johannes Weiner,
linux-mm, Linux Kernel Mailing List, Stanislav Kozina,
Tetsuo Handa
On (09/23/15 11:06), Vladimir Davydov wrote:
> Hi,
>
> On Tue, Sep 22, 2015 at 04:30:13PM -0700, David Rientjes wrote:
> > The oom killer takes task_lock() in a couple of places solely to protect
> > printing the task's comm.
> >
> > A process's comm, including current's comm, may change due to
> > /proc/pid/comm or PR_SET_NAME.
> >
> > The comm will always be NULL-terminated, so the worst race scenario would
> > only be during update. We can tolerate a comm being printed that is in
> > the middle of an update to avoid taking the lock.
> >
> > Other locations in the kernel have already dropped task_lock() when
> > printing comm, so this is consistent.
>
> Without the protection, can't reading task->comm race with PR_SET_NAME
> as described below?
the previous name was already null terminated, so it should be
[name\0old_name\0]
-ss
>
> Let T->comm[16] = "name\0rubbish1234"
>
> CPU1 CPU2
> ---- ----
> set_task_comm(T, "longname\0")
> T->comm[0] = 'l'
> T->comm[1] = 'o'
> T->comm[2] = 'n'
> T->comm[3] = 'g'
> T->comm[4] = 'n'
> printk("%s\n", T->comm)
> T->comm = "longnrubbish1234"
> OOPS: the string is not
> nil-terminated!
> T->comm[5] = 'a'
> T->comm[6] = 'm'
> T->comm[7] = 'e'
> T->comm[8] = '\0'
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 9:13 ` Sergey Senozhatsky
@ 2015-09-23 9:30 ` Vladimir Davydov
2015-09-23 9:43 ` Michal Hocko
0 siblings, 1 reply; 11+ messages in thread
From: Vladimir Davydov @ 2015-09-23 9:30 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: David Rientjes, Andrew Morton, Oleg Nesterov, Michal Hocko,
Linus Torvalds, Kyle Walker, Christoph Lameter, Johannes Weiner,
linux-mm, Linux Kernel Mailing List, Stanislav Kozina,
Tetsuo Handa
On Wed, Sep 23, 2015 at 06:13:54PM +0900, Sergey Senozhatsky wrote:
> On (09/23/15 11:06), Vladimir Davydov wrote:
> > Hi,
> >
> > On Tue, Sep 22, 2015 at 04:30:13PM -0700, David Rientjes wrote:
> > > The oom killer takes task_lock() in a couple of places solely to protect
> > > printing the task's comm.
> > >
> > > A process's comm, including current's comm, may change due to
> > > /proc/pid/comm or PR_SET_NAME.
> > >
> > > The comm will always be NULL-terminated, so the worst race scenario would
> > > only be during update. We can tolerate a comm being printed that is in
> > > the middle of an update to avoid taking the lock.
> > >
> > > Other locations in the kernel have already dropped task_lock() when
> > > printing comm, so this is consistent.
> >
> > Without the protection, can't reading task->comm race with PR_SET_NAME
> > as described below?
>
> the previous name was already null terminated,
Yeah, but if the old name is shorter than the new one, set_task_comm()
overwrites the terminating null of the old name before writing the new
terminating null, so there is a short time window during which tsk->comm
might be not null-terminated, no?
Thanks,
Vladimir
> so it should be
>
> [name\0old_name\0]
>
> -ss
>
> >
> > Let T->comm[16] = "name\0rubbish1234"
> >
> > CPU1 CPU2
> > ---- ----
> > set_task_comm(T, "longname\0")
> > T->comm[0] = 'l'
> > T->comm[1] = 'o'
> > T->comm[2] = 'n'
> > T->comm[3] = 'g'
> > T->comm[4] = 'n'
> > printk("%s\n", T->comm)
> > T->comm = "longnrubbish1234"
> > OOPS: the string is not
> > nil-terminated!
> > T->comm[5] = 'a'
> > T->comm[6] = 'm'
> > T->comm[7] = 'e'
> > T->comm[8] = '\0'
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 9:30 ` Vladimir Davydov
@ 2015-09-23 9:43 ` Michal Hocko
2015-09-23 9:50 ` Sergey Senozhatsky
0 siblings, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2015-09-23 9:43 UTC (permalink / raw)
To: Vladimir Davydov
Cc: Sergey Senozhatsky, David Rientjes, Andrew Morton, Oleg Nesterov,
Linus Torvalds, Kyle Walker, Christoph Lameter, Johannes Weiner,
linux-mm, Linux Kernel Mailing List, Stanislav Kozina,
Tetsuo Handa
On Wed 23-09-15 12:30:22, Vladimir Davydov wrote:
> On Wed, Sep 23, 2015 at 06:13:54PM +0900, Sergey Senozhatsky wrote:
> > On (09/23/15 11:06), Vladimir Davydov wrote:
> > > Hi,
> > >
> > > On Tue, Sep 22, 2015 at 04:30:13PM -0700, David Rientjes wrote:
> > > > The oom killer takes task_lock() in a couple of places solely to protect
> > > > printing the task's comm.
> > > >
> > > > A process's comm, including current's comm, may change due to
> > > > /proc/pid/comm or PR_SET_NAME.
> > > >
> > > > The comm will always be NULL-terminated, so the worst race scenario would
> > > > only be during update. We can tolerate a comm being printed that is in
> > > > the middle of an update to avoid taking the lock.
> > > >
> > > > Other locations in the kernel have already dropped task_lock() when
> > > > printing comm, so this is consistent.
> > >
> > > Without the protection, can't reading task->comm race with PR_SET_NAME
> > > as described below?
> >
> > the previous name was already null terminated,
>
> Yeah, but if the old name is shorter than the new one, set_task_comm()
> overwrites the terminating null of the old name before writing the new
> terminating null, so there is a short time window during which tsk->comm
> might be not null-terminated, no?
Not really:
case PR_SET_NAME:
comm[sizeof(me->comm) - 1] = 0;
if (strncpy_from_user(comm, (char __user *)arg2,
sizeof(me->comm) - 1) < 0)
return -EFAULT;
So it first writes the terminating 0 and only then starts copying.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 9:43 ` Michal Hocko
@ 2015-09-23 9:50 ` Sergey Senozhatsky
2015-09-23 9:57 ` Sergey Senozhatsky
2015-09-23 10:07 ` Vladimir Davydov
0 siblings, 2 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2015-09-23 9:50 UTC (permalink / raw)
To: Michal Hocko
Cc: Vladimir Davydov, Sergey Senozhatsky, David Rientjes,
Andrew Morton, Oleg Nesterov, Linus Torvalds, Kyle Walker,
Christoph Lameter, Johannes Weiner, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
On (09/23/15 11:43), Michal Hocko wrote:
[..]
> > > the previous name was already null terminated,
> >
> > Yeah, but if the old name is shorter than the new one, set_task_comm()
> > overwrites the terminating null of the old name before writing the new
> > terminating null, so there is a short time window during which tsk->comm
> > might be not null-terminated, no?
>
> Not really:
> case PR_SET_NAME:
> comm[sizeof(me->comm) - 1] = 0;
> if (strncpy_from_user(comm, (char __user *)arg2,
> sizeof(me->comm) - 1) < 0)
> return -EFAULT;
>
> So it first writes the terminating 0 and only then starts copying.
right.
hm, shouldn't set_task_comm()->__set_task_comm() do the same?
-ss
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 9:50 ` Sergey Senozhatsky
@ 2015-09-23 9:57 ` Sergey Senozhatsky
2015-09-23 10:07 ` Vladimir Davydov
1 sibling, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2015-09-23 9:57 UTC (permalink / raw)
To: Michal Hocko
Cc: Vladimir Davydov, Sergey Senozhatsky, David Rientjes,
Andrew Morton, Oleg Nesterov, Linus Torvalds, Kyle Walker,
Christoph Lameter, Johannes Weiner, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
On (09/23/15 18:50), Sergey Senozhatsky wrote:
> On (09/23/15 11:43), Michal Hocko wrote:
> [..]
> > > > the previous name was already null terminated,
> > >
> > > Yeah, but if the old name is shorter than the new one, set_task_comm()
> > > overwrites the terminating null of the old name before writing the new
> > > terminating null, so there is a short time window during which tsk->comm
> > > might be not null-terminated, no?
> >
> > Not really:
> > case PR_SET_NAME:
> > comm[sizeof(me->comm) - 1] = 0;
> > if (strncpy_from_user(comm, (char __user *)arg2,
> > sizeof(me->comm) - 1) < 0)
> > return -EFAULT;
> >
> > So it first writes the terminating 0 and only then starts copying.
>
> right.
... no right. that should have been
me->comm[sizeof(me->comm) - 1] = 0;
to be save. no?
> hm, shouldn't set_task_comm()->__set_task_comm() do the same?
or something like this instead
---
fs/exec.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/exec.c b/fs/exec.c
index b06623a..d7d2de0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1072,6 +1072,7 @@ EXPORT_SYMBOL_GPL(get_task_comm);
void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
+ tsk->comm[sizeof(tsk->comm) - 1] = 0;
task_lock(tsk);
trace_task_rename(tsk, buf);
strlcpy(tsk->comm, buf, sizeof(tsk->comm));
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 9:50 ` Sergey Senozhatsky
2015-09-23 9:57 ` Sergey Senozhatsky
@ 2015-09-23 10:07 ` Vladimir Davydov
2015-09-23 10:41 ` Michal Hocko
1 sibling, 1 reply; 11+ messages in thread
From: Vladimir Davydov @ 2015-09-23 10:07 UTC (permalink / raw)
To: Michal Hocko, Sergey Senozhatsky
Cc: David Rientjes, Andrew Morton, Oleg Nesterov, Linus Torvalds,
Kyle Walker, Christoph Lameter, Johannes Weiner, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
On Wed, Sep 23, 2015 at 06:50:22PM +0900, Sergey Senozhatsky wrote:
> On (09/23/15 11:43), Michal Hocko wrote:
> [..]
> > > > the previous name was already null terminated,
> > >
> > > Yeah, but if the old name is shorter than the new one, set_task_comm()
> > > overwrites the terminating null of the old name before writing the new
> > > terminating null, so there is a short time window during which tsk->comm
> > > might be not null-terminated, no?
> >
> > Not really:
> > case PR_SET_NAME:
> > comm[sizeof(me->comm) - 1] = 0;
> > if (strncpy_from_user(comm, (char __user *)arg2,
> > sizeof(me->comm) - 1) < 0)
> > return -EFAULT;
> >
> > So it first writes the terminating 0 and only then starts copying.
It writes 0 to a temporary buffer, not to tsk->comm, so I don't think
it's related. However, reading tsk->comm w/o locking must be safe
anyway, because tsk->comm[TASK_COMM_LEN-1] is always 0 (inherited from
init_task) and it never gets overwritten, because __set_task_comm() uses
strlcpy().
>
> right.
>
> hm, shouldn't set_task_comm()->__set_task_comm() do the same?
I don't think so - see above.
Thanks,
Vladimir
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-23 10:07 ` Vladimir Davydov
@ 2015-09-23 10:41 ` Michal Hocko
0 siblings, 0 replies; 11+ messages in thread
From: Michal Hocko @ 2015-09-23 10:41 UTC (permalink / raw)
To: Vladimir Davydov
Cc: Sergey Senozhatsky, David Rientjes, Andrew Morton, Oleg Nesterov,
Linus Torvalds, Kyle Walker, Christoph Lameter, Johannes Weiner,
linux-mm, Linux Kernel Mailing List, Stanislav Kozina,
Tetsuo Handa
On Wed 23-09-15 13:07:40, Vladimir Davydov wrote:
> On Wed, Sep 23, 2015 at 06:50:22PM +0900, Sergey Senozhatsky wrote:
> > On (09/23/15 11:43), Michal Hocko wrote:
> > [..]
> > > > > the previous name was already null terminated,
> > > >
> > > > Yeah, but if the old name is shorter than the new one, set_task_comm()
> > > > overwrites the terminating null of the old name before writing the new
> > > > terminating null, so there is a short time window during which tsk->comm
> > > > might be not null-terminated, no?
> > >
> > > Not really:
> > > case PR_SET_NAME:
> > > comm[sizeof(me->comm) - 1] = 0;
> > > if (strncpy_from_user(comm, (char __user *)arg2,
> > > sizeof(me->comm) - 1) < 0)
> > > return -EFAULT;
> > >
> > > So it first writes the terminating 0 and only then starts copying.
>
> It writes 0 to a temporary buffer, not to tsk->comm, so I don't think
> it's related. However, reading tsk->comm w/o locking must be safe
> anyway, because tsk->comm[TASK_COMM_LEN-1] is always 0 (inherited from
> init_task) and it never gets overwritten, because __set_task_comm() uses
> strlcpy().
Right you are! I am blind obviously... Thought we are copying directly
to the task->comm.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] mm, oom: remove task_lock protecting comm printing
2015-09-22 23:30 [patch] mm, oom: remove task_lock protecting comm printing David Rientjes
2015-09-23 7:44 ` Michal Hocko
2015-09-23 8:06 ` Vladimir Davydov
@ 2015-09-24 19:45 ` Johannes Weiner
2 siblings, 0 replies; 11+ messages in thread
From: Johannes Weiner @ 2015-09-24 19:45 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Oleg Nesterov, Michal Hocko, Linus Torvalds,
Kyle Walker, Christoph Lameter, Vladimir Davydov, linux-mm,
Linux Kernel Mailing List, Stanislav Kozina, Tetsuo Handa
On Tue, Sep 22, 2015 at 04:30:13PM -0700, David Rientjes wrote:
> The oom killer takes task_lock() in a couple of places solely to protect
> printing the task's comm.
>
> A process's comm, including current's comm, may change due to
> /proc/pid/comm or PR_SET_NAME.
>
> The comm will always be NULL-terminated, so the worst race scenario would
> only be during update. We can tolerate a comm being printed that is in
> the middle of an update to avoid taking the lock.
>
> Other locations in the kernel have already dropped task_lock() when
> printing comm, so this is consistent.
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: David Rientjes <rientjes@google.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-09-24 19:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-22 23:30 [patch] mm, oom: remove task_lock protecting comm printing David Rientjes
2015-09-23 7:44 ` Michal Hocko
2015-09-23 8:06 ` Vladimir Davydov
2015-09-23 9:13 ` Sergey Senozhatsky
2015-09-23 9:30 ` Vladimir Davydov
2015-09-23 9:43 ` Michal Hocko
2015-09-23 9:50 ` Sergey Senozhatsky
2015-09-23 9:57 ` Sergey Senozhatsky
2015-09-23 10:07 ` Vladimir Davydov
2015-09-23 10:41 ` Michal Hocko
2015-09-24 19:45 ` Johannes Weiner
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).