On Mon, Sep 16, 2019 at 10:13:41AM -0400, KeMeng Shi wrote:
> Commit f786ecba41588 ("connector: add comm change event report to proc
> connector") added proc_comm_connector to report comm change event, and
> prctl will report comm change event when dealing with PR_SET_NAME case.
>
> prctl can only set the name of the calling thread. In order to set the name
> of other threads in a process, modifying /proc/self/task/tid/comm is a
> general way.It's exactly how pthread_setname_np do to set name of a thread.
>
> It's unable to get comm change event of thread if the name of thread is set
> by other thread via pthread_setname_np. This update provides a chance for
> application to monitor and control threads whose name is set by other
> threads.
>
> Signed-off-by: KeMeng Shi <[email protected]>
> ---
> fs/proc/base.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ebea9501afb8..34ffe572ac69 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -94,6 +94,7 @@
> #include <linux/sched/debug.h>
> #include <linux/sched/stat.h>
> #include <linux/posix-timers.h>
> +#include <linux/cn_proc.h>
> #include <trace/events/oom.h>
> #include "internal.h"
> #include "fd.h"
> @@ -1549,10 +1550,12 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
> if (!p)
> return -ESRCH;
>
> - if (same_thread_group(current, p))
> + if (same_thread_group(current, p)) {
> set_task_comm(p, buffer);
> - else
> + proc_comm_connector(p);
> + } else {
> count = -EINVAL;
> + }
>
> put_task_struct(p);
The rough idea looks ok to me but I have two concerns:
(1) This looks like it will be visible to userspace, and this changes
the behaviour after ~8 years of not reporting this event.
(2) What prevents proc_comm_connector(p) running concurrently with itself
via the prctl()? The locking seems to be confined to set_task_comm().
Will
>(2) What prevents proc_comm_connector(p) running concurrently with itself
> via the prctl()? The locking seems to be confined to set_task_comm().
To be honest, I did not consider the concurrence problem at beginning. And
some comm change events may lost or are reported repeatly as follows:
set name via procfs set name via prctl
set_task_comm
set_task_comm
proc_comm_connector
proc_comm_connector
Comm change event belong to procfs losts and the fresh comm change belong
to prctl is reported twice. Actually, there is also concurrence problem
without this update as follows:
set name via procfs set name via prctl
set_task_comm
set_task_comm
proc_comm_connector
Comm change event from procfs is reported instead of prctl, this may
bothers user who only care the comm change via prctl.
There is no matter if user only care the latest comm change otherwise, put
setting name and reporting event in crtical region may be the only answer.
Thanks for review.
KeMeng Shi
On Tue, Sep 17, 2019 at 09:56:28AM -0400, KeMeng Shi wrote:
> on 2019/9/17 at 5:10, Will Deacon wrote:
> >The rough idea looks ok to me but I have two concerns:
> >
> > (1) This looks like it will be visible to userspace, and this changes
> > the behaviour after ~8 years of not reporting this event.
> This do bother for users who only care the comm change via prctl, but it
> also benefits users who want all comm changes. Maybe the best way is add
> something like config or switch to meet the both conditions above. In my
> opinion, users cares comm change event rather than how it change.
I was really just looking for some intuition as to how this event is
currently used and why extending it like this is unlikely to break those
existing users.
> >(2) What prevents proc_comm_connector(p) running concurrently with itself
> > via the prctl()? The locking seems to be confined to set_task_comm().
> To be honest, I did not consider the concurrence problem at beginning. And
> some comm change events may lost or are reported repeatly as follows:
> set name via procfs set name via prctl
> set_task_comm
> set_task_comm
> proc_comm_connector
> proc_comm_connector
> Comm change event belong to procfs losts and the fresh comm change belong
> to prctl is reported twice. Actually, there is also concurrence problem
> without this update as follows:
> set name via procfs set name via prctl
> set_task_comm
> set_task_comm
> proc_comm_connector
> Comm change event from procfs is reported instead of prctl, this may
> bothers user who only care the comm change via prctl.
Perhaps, although given that proc_comm_connector() is currently only called
on the prctl() path, then it does at least provide the comm from the most
recent prctl() invocation. With your path, the calls can go out of order,
so I think that probably needs fixing.
Will
on 2019/9/17 at 5:10, Will Deacon wrote:
>The rough idea looks ok to me but I have two concerns:
>
> (1) This looks like it will be visible to userspace, and this changes
> the behaviour after ~8 years of not reporting this event.
This do bother for users who only care the comm change via prctl, but it
also benefits users who want all comm changes. Maybe the best way is add
something like config or switch to meet the both conditions above. In my
opinion, users cares comm change event rather than how it change.
>(2) What prevents proc_comm_connector(p) running concurrently with itself
> via the prctl()? The locking seems to be confined to set_task_comm().
To be honest, I did not consider the concurrence problem at beginning. And
some comm change events may lost or are reported repeatly as follows:
set name via procfs set name via prctl
set_task_comm
set_task_comm
proc_comm_connector
proc_comm_connector
Comm change event belong to procfs losts and the fresh comm change belong
to prctl is reported twice. Actually, there is also concurrence problem
without this update as follows:
set name via procfs set name via prctl
set_task_comm
set_task_comm
proc_comm_connector
Comm change event from procfs is reported instead of prctl, this may
bothers user who only care the comm change via prctl.
There is no matter if user only care the latest comm change otherwise, put
setting name and reporting event in crtical region may be the only answer.
I'm sorry the response to (1) concern is missing in last mail. This is a
full version. Apologize again for my mistake.
Thanks for review.
KeMeng Shi
On 2019/9/18 at 1:08, Will Deacon wrote:
>On Tue, Sep 17, 2019 at 09:56:28AM -0400, KeMeng Shi wrote:
>>on 2019/9/17 at 5:10, Will Deacon wrote:
>>>The rough idea looks ok to me but I have two concerns:
>>>
>>> (1) This looks like it will be visible to userspace, and this changes
>>> the behaviour after ~8 years of not reporting this event.
>>This do bother for users who only care the comm change via prctl, but
>>it also benefits users who want all comm changes. Maybe the best way
>>is add something like config or switch to meet the both conditions
>>above. In my opinion, users cares comm change event rather than how it
>>change.
>
>I was really just looking for some intuition as to how this event is currently
>used and why extending it like this is unlikely to break those existing users.
By listening these comm change events, user is able to monitor and control
specific threads that they are interested. For instance, a process control
daemon listening to proc connector and following comm value policies can
place specific threads to assigned cgroup partitions (quota from commit
f786ecba415888 ("connector: add comm change event report to proc
connector")). It's harmless as user ignore the threads with names that
they are not interested.
>>>(2) What prevents proc_comm_connector(p) running concurrently with
>>>itself via the prctl()? The locking seems to be confined to set_task_comm().
>>To be honest, I did not consider the concurrence problem at beginning.
>>And some comm change events may lost or are reported repeatly as follows:
>>set name via procfs set name via prctl
>>set_task_comm
>> set_task_comm
>>proc_comm_connector
>> proc_comm_connector
>>Comm change event belong to procfs losts and the fresh comm change
>>belong to prctl is reported twice. Actually, there is also concurrence
>>problem without this update as follows:
>>set name via procfs set name via prctl
>> set_task_comm
>>set_task_comm
>> proc_comm_connector
>>Comm change event from procfs is reported instead of prctl, this may
>>bothers user who only care the comm change via prctl.
>
>Perhaps, although given that proc_comm_connector() is currently only
>called on the prctl() path, then it does at least provide the comm
>from the most recent prctl() invocation. With your path, the calls
>can go out of order, so I think that probably needs fixing.
It's indeed necessary to fix the concurrence problem. I will submit
a v2 patch when I fix this.
Thanks for your review and advise.
KeMeng Shi
On Wed, 18 Sep 2019 22:43:21 -0400 KeMeng Shi <[email protected]> wrote:
> It's indeed necessary to fix the concurrence problem. I will submit
> a v2 patch when I fix this.
Even though this is a procfs change, connector patches are usually
handled by davem. So please cc himself, Evgeniy Polyakov and
[email protected] on the next version, thanks.