Re: [PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct->comm to use RCU.]
On 14/03/27, Stephen Smalley wrote: > On 03/27/2014 01:20 PM, Richard Guy Briggs wrote: > > On 14/03/12, James Morris wrote: > >> On Tue, 11 Mar 2014, Tetsuo Handa wrote: > >> > >>> And the same phrase goes to James Morris... > >>> > >>> If you are sure that it is safe to use get_task_comm() from > >>> dump_common_audit_data() and you prefer locked version, please pick up > >>> below > >>> patch via your git tree. > >>> > >>> If you are unsure or prefer lockless version, I'll make a lockless version > >>> using do_get_task_comm() proposed in this thread. > >> > >> If you can't understand whether your patch is correct or not, don't ask me > >> to apply it to my tree. > >> > >> If you're unsure, get it reviewed first. > > > > Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, > > > > Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The > > general case gives pid and comm of current. Then the > > LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in > > the struct common_audit_data pointer. They are a duplicate of the > > general case without generating a new message. I expect this will cause > > ausearch to ignore those latter two fields. Should the latter two be > > renamed to something like ad_pid= and ad_comm= ? > > Hmmm..only seems to be used by Smack. > SELinux had a tsk field in common_audit_data that was removed by > b466066. This other tsk field seems to have been added for Smack by > 6e837fb. > > That said, it would be nice to have pid/comm info for the target of a > signal check as well as current. Reviving a bit of an old thread... Probably the appropriate keywords would be opid= and ocomm= for the target (object). - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct-comm to use RCU.]
On 14/03/27, Stephen Smalley wrote: On 03/27/2014 01:20 PM, Richard Guy Briggs wrote: On 14/03/12, James Morris wrote: On Tue, 11 Mar 2014, Tetsuo Handa wrote: And the same phrase goes to James Morris... If you are sure that it is safe to use get_task_comm() from dump_common_audit_data() and you prefer locked version, please pick up below patch via your git tree. If you are unsure or prefer lockless version, I'll make a lockless version using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The general case gives pid and comm of current. Then the LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in the struct common_audit_data pointer. They are a duplicate of the general case without generating a new message. I expect this will cause ausearch to ignore those latter two fields. Should the latter two be renamed to something like ad_pid= and ad_comm= ? Hmmm..only seems to be used by Smack. SELinux had a tsk field in common_audit_data that was removed by b466066. This other tsk field seems to have been added for Smack by 6e837fb. That said, it would be nice to have pid/comm info for the target of a signal check as well as current. Reviving a bit of an old thread... Probably the appropriate keywords would be opid= and ocomm= for the target (object). - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct->comm to use RCU.]
On 03/27/2014 01:20 PM, Richard Guy Briggs wrote: > On 14/03/12, James Morris wrote: >> On Tue, 11 Mar 2014, Tetsuo Handa wrote: >> >>> And the same phrase goes to James Morris... >>> >>> If you are sure that it is safe to use get_task_comm() from >>> dump_common_audit_data() and you prefer locked version, please pick up below >>> patch via your git tree. >>> >>> If you are unsure or prefer lockless version, I'll make a lockless version >>> using do_get_task_comm() proposed in this thread. >> >> If you can't understand whether your patch is correct or not, don't ask me >> to apply it to my tree. >> >> If you're unsure, get it reviewed first. > > Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, > > Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The > general case gives pid and comm of current. Then the > LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in > the struct common_audit_data pointer. They are a duplicate of the > general case without generating a new message. I expect this will cause > ausearch to ignore those latter two fields. Should the latter two be > renamed to something like ad_pid= and ad_comm= ? Hmmm..only seems to be used by Smack. SELinux had a tsk field in common_audit_data that was removed by b466066. This other tsk field seems to have been added for Smack by 6e837fb. That said, it would be nice to have pid/comm info for the target of a signal check as well as current. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct->comm to use RCU.]
On 14/03/12, James Morris wrote: > On Tue, 11 Mar 2014, Tetsuo Handa wrote: > > > And the same phrase goes to James Morris... > > > > If you are sure that it is safe to use get_task_comm() from > > dump_common_audit_data() and you prefer locked version, please pick up below > > patch via your git tree. > > > > If you are unsure or prefer lockless version, I'll make a lockless version > > using do_get_task_comm() proposed in this thread. > > If you can't understand whether your patch is correct or not, don't ask me > to apply it to my tree. > > If you're unsure, get it reviewed first. Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The general case gives pid and comm of current. Then the LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in the struct common_audit_data pointer. They are a duplicate of the general case without generating a new message. I expect this will cause ausearch to ignore those latter two fields. Should the latter two be renamed to something like ad_pid= and ad_comm= ? Tetsuo, this conversation should have been on the linux-au...@redhat.com list the whole time... > - James - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct-comm to use RCU.]
On 14/03/12, James Morris wrote: On Tue, 11 Mar 2014, Tetsuo Handa wrote: And the same phrase goes to James Morris... If you are sure that it is safe to use get_task_comm() from dump_common_audit_data() and you prefer locked version, please pick up below patch via your git tree. If you are unsure or prefer lockless version, I'll make a lockless version using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The general case gives pid and comm of current. Then the LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in the struct common_audit_data pointer. They are a duplicate of the general case without generating a new message. I expect this will cause ausearch to ignore those latter two fields. Should the latter two be renamed to something like ad_pid= and ad_comm= ? Tetsuo, this conversation should have been on the linux-au...@redhat.com list the whole time... - James - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct-comm to use RCU.]
On 03/27/2014 01:20 PM, Richard Guy Briggs wrote: On 14/03/12, James Morris wrote: On Tue, 11 Mar 2014, Tetsuo Handa wrote: And the same phrase goes to James Morris... If you are sure that it is safe to use get_task_comm() from dump_common_audit_data() and you prefer locked version, please pick up below patch via your git tree. If you are unsure or prefer lockless version, I'll make a lockless version using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The general case gives pid and comm of current. Then the LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in the struct common_audit_data pointer. They are a duplicate of the general case without generating a new message. I expect this will cause ausearch to ignore those latter two fields. Should the latter two be renamed to something like ad_pid= and ad_comm= ? Hmmm..only seems to be used by Smack. SELinux had a tsk field in common_audit_data that was removed by b466066. This other tsk field seems to have been added for Smack by 6e837fb. That said, it would be nice to have pid/comm info for the target of a signal check as well as current. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On 14/03/12, James Morris wrote: > On Tue, 11 Mar 2014, Tetsuo Handa wrote: > > And the same phrase goes to James Morris... > > > > If you are sure that it is safe to use get_task_comm() from > > dump_common_audit_data() and you prefer locked version, please pick up below > > patch via your git tree. > > > > If you are unsure or prefer lockless version, I'll make a lockless version > > using do_get_task_comm() proposed in this thread. > > If you can't understand whether your patch is correct or not, don't ask me > to apply it to my tree. > > If you're unsure, get it reviewed first. I looked at the calling function tree back a number of steps and didn't see any other evidence of task locks held, so I'm comfortable with this patch and would prefer it to either memcpy() or do_get_task_comm(), but would still like to get an opinion from others. > - James - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
On 14/03/12, James Morris wrote: On Tue, 11 Mar 2014, Tetsuo Handa wrote: And the same phrase goes to James Morris... If you are sure that it is safe to use get_task_comm() from dump_common_audit_data() and you prefer locked version, please pick up below patch via your git tree. If you are unsure or prefer lockless version, I'll make a lockless version using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. I looked at the calling function tree back a number of steps and didn't see any other evidence of task locks held, so I'm comfortable with this patch and would prefer it to either memcpy() or do_get_task_comm(), but would still like to get an opinion from others. - James - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On Tue, 11 Mar 2014, Tetsuo Handa wrote: > And the same phrase goes to James Morris... > > If you are sure that it is safe to use get_task_comm() from > dump_common_audit_data() and you prefer locked version, please pick up below > patch via your git tree. > > If you are unsure or prefer lockless version, I'll make a lockless version > using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. - James -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
And the same phrase goes to James Morris...
If you are sure that it is safe to use get_task_comm() from
dump_common_audit_data() and you prefer locked version, please pick up below
patch via your git tree.
If you are unsure or prefer lockless version, I'll make a lockless version
using do_get_task_comm() proposed in this thread.
--
>From 1fe3dcd8792bc1296e337daba661200a8feaf706 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa
Date: Tue, 11 Mar 2014 21:09:48 +0900
Subject: [PATCH] LSM: Pass comm name via get_task_comm()
When we pass task->comm to audit_log_untrustedstring(), we need to pass
a snapshot of it using get_task_comm(). Otherwise, we will lose fields
after comm= if we raced with updating task->comm.
Signed-off-by: Tetsuo Handa
---
security/lsm_audit.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 9a62045..6291c67 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -212,6 +212,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
struct common_audit_data *a)
{
struct task_struct *tsk = current;
+ char name[TASK_COMM_LEN];
/*
* To keep stack sizes in check force programers to notice if they
@@ -221,7 +222,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, tsk));
switch (a->type) {
case LSM_AUDIT_DATA_NONE:
@@ -280,7 +281,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
tsk = a->u.tsk;
if (tsk && tsk->pid) {
audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, tsk));
}
break;
case LSM_AUDIT_DATA_NET:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
Richard Guy Briggs wrote:
> > Even if you don't trust the comm= field, it is annoying for me that fields
> > after comm= are missing in the audit log.
>
> More than annoying, that isn't acceptable.
>
OK. If you are sure that it is safe to use get_task_comm() from
audit_log_task() and you prefer locked version, please pick up below patch
via your git tree.
If you are unsure or prefer lockless version, I'll make a lockless version
using do_get_task_comm() proposed in this thread.
--
>From 88c3ff13efa10df6f4d4d0f2c243124ce6a3eaeb Mon Sep 17 00:00:00 2001
From: Tetsuo Handa
Date: Tue, 11 Mar 2014 20:47:29 +0900
Subject: [PATCH] Audit: Pass comm name via get_task_comm()
When we pass task->comm to audit_log_untrustedstring(), we need to pass
a snapshot of it using get_task_comm(). Otherwise, we will lose fields
after comm= if we raced with updating task->comm.
Signed-off-by: Tetsuo Handa
---
kernel/auditsc.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 7aef2f4..ba57993 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2357,6 +2357,7 @@ static void audit_log_task(struct audit_buffer *ab)
kgid_t gid;
unsigned int sessionid;
struct mm_struct *mm = current->mm;
+ char name[TASK_COMM_LEN];
auid = audit_get_loginuid(current);
sessionid = audit_get_sessionid(current);
@@ -2369,7 +2370,7 @@ static void audit_log_task(struct audit_buffer *ab)
sessionid);
audit_log_task_context(ab);
audit_log_format(ab, " pid=%d comm=", current->pid);
- audit_log_untrustedstring(ab, current->comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, current));
if (mm) {
down_read(>mmap_sem);
if (mm->exe_file)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
Richard Guy Briggs wrote:
Even if you don't trust the comm= field, it is annoying for me that fields
after comm= are missing in the audit log.
More than annoying, that isn't acceptable.
OK. If you are sure that it is safe to use get_task_comm() from
audit_log_task() and you prefer locked version, please pick up below patch
via your git tree.
If you are unsure or prefer lockless version, I'll make a lockless version
using do_get_task_comm() proposed in this thread.
--
From 88c3ff13efa10df6f4d4d0f2c243124ce6a3eaeb Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Tue, 11 Mar 2014 20:47:29 +0900
Subject: [PATCH] Audit: Pass comm name via get_task_comm()
When we pass task-comm to audit_log_untrustedstring(), we need to pass
a snapshot of it using get_task_comm(). Otherwise, we will lose fields
after comm= if we raced with updating task-comm.
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
kernel/auditsc.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 7aef2f4..ba57993 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2357,6 +2357,7 @@ static void audit_log_task(struct audit_buffer *ab)
kgid_t gid;
unsigned int sessionid;
struct mm_struct *mm = current-mm;
+ char name[TASK_COMM_LEN];
auid = audit_get_loginuid(current);
sessionid = audit_get_sessionid(current);
@@ -2369,7 +2370,7 @@ static void audit_log_task(struct audit_buffer *ab)
sessionid);
audit_log_task_context(ab);
audit_log_format(ab, pid=%d comm=, current-pid);
- audit_log_untrustedstring(ab, current-comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, current));
if (mm) {
down_read(mm-mmap_sem);
if (mm-exe_file)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
And the same phrase goes to James Morris...
If you are sure that it is safe to use get_task_comm() from
dump_common_audit_data() and you prefer locked version, please pick up below
patch via your git tree.
If you are unsure or prefer lockless version, I'll make a lockless version
using do_get_task_comm() proposed in this thread.
--
From 1fe3dcd8792bc1296e337daba661200a8feaf706 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Tue, 11 Mar 2014 21:09:48 +0900
Subject: [PATCH] LSM: Pass comm name via get_task_comm()
When we pass task-comm to audit_log_untrustedstring(), we need to pass
a snapshot of it using get_task_comm(). Otherwise, we will lose fields
after comm= if we raced with updating task-comm.
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
security/lsm_audit.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 9a62045..6291c67 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -212,6 +212,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
struct common_audit_data *a)
{
struct task_struct *tsk = current;
+ char name[TASK_COMM_LEN];
/*
* To keep stack sizes in check force programers to notice if they
@@ -221,7 +222,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
BUILD_BUG_ON(sizeof(a-u) sizeof(void *)*2);
audit_log_format(ab, pid=%d comm=, tsk-pid);
- audit_log_untrustedstring(ab, tsk-comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, tsk));
switch (a-type) {
case LSM_AUDIT_DATA_NONE:
@@ -280,7 +281,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
tsk = a-u.tsk;
if (tsk tsk-pid) {
audit_log_format(ab, pid=%d comm=, tsk-pid);
- audit_log_untrustedstring(ab, tsk-comm);
+ audit_log_untrustedstring(ab, get_task_comm(name, tsk));
}
break;
case LSM_AUDIT_DATA_NET:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
On Tue, 11 Mar 2014, Tetsuo Handa wrote: And the same phrase goes to James Morris... If you are sure that it is safe to use get_task_comm() from dump_common_audit_data() and you prefer locked version, please pick up below patch via your git tree. If you are unsure or prefer lockless version, I'll make a lockless version using do_get_task_comm() proposed in this thread. If you can't understand whether your patch is correct or not, don't ask me to apply it to my tree. If you're unsure, get it reviewed first. - James -- James Morris jmor...@namei.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On 14/03/08, Tetsuo Handa wrote: > Richard Guy Briggs wrote: > > > > > Likewise, audit_log_untrustedstring(ab, current->comm) is racy. > > > > > If task->comm was "Hello Linux" until > > > > > audit_string_contains_control() in > > > > > audit_log_n_untrustedstring() returns false, and becomes "Penguin" > > > > > before > > > > > memcpy() in audit_log_n_string() is called, memcpy() will emit > > > > > "Penguin\0nux" > > > > > into the audit log, which results in loss of information (e.g. > > > > > SELinux > > > > > context) due to the unexpected '\0' byte. > > > > > > > > I expect the audit people don't like this? Also, how do audit and the > > > > LSM crap things interact? I thought they were both different piles of > > > > ignorable goo? > > > > > > I think the audit people do not like loss of information. Some of LSM > > > modules > > > are using audit subsystem for recording security related events. An > > > example is > > > shown later. > > > > This is true, however since comm it untrusted because it can be modified > > by the user audit doesn't trust it anyways, so who cares? > > Excuse me, but did you understand this side effect correctly? > ), you can see that fields after comm= (e.g. exe= subj= key= ) are missing. Ok, from your desciption and example I had clearly not fully understood the problem. > -- An audit log with race -- > type=SYSCALL msg=audit(1394281498.566:63): arch=4003 syscall=11 > success=yes exit=0 a0=858c9c8 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 > ppid=1747 pid=2662 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 > fsgid=0 tty=pts1 ses=2 comm="truncated > type=EXECVE msg=audit(1394281498.566:63): argc=1 a0="/tmp/printable-comm" > type=CWD msg=audit(1394281498.566:63): cwd="/root" > type=PATH msg=audit(1394281498.566:63): item=0 name="/tmp/printable-comm" > inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00 > obj=system_u:object_r:bin_t:s0 nametype=NORMAL > type=PATH msg=audit(1394281498.566:63): item=1 name=(null) inode=2360187 > dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00 > obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL > -- An audit log with race -- > > Even if you don't trust the comm= field, it is annoying for me that fields > after comm= are missing in the audit log. More than annoying, that isn't acceptable. - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
On 14/03/08, Tetsuo Handa wrote: Richard Guy Briggs wrote: Likewise, audit_log_untrustedstring(ab, current-comm) is racy. If task-comm was Hello Linux until audit_string_contains_control() in audit_log_n_untrustedstring() returns false, and becomes Penguin before memcpy() in audit_log_n_string() is called, memcpy() will emit Penguin\0nux into the audit log, which results in loss of information (e.g. SELinux context) due to the unexpected '\0' byte. I expect the audit people don't like this? Also, how do audit and the LSM crap things interact? I thought they were both different piles of ignorable goo? I think the audit people do not like loss of information. Some of LSM modules are using audit subsystem for recording security related events. An example is shown later. This is true, however since comm it untrusted because it can be modified by the user audit doesn't trust it anyways, so who cares? Excuse me, but did you understand this side effect correctly? snip ), you can see that fields after comm= (e.g. exe= subj= key= ) are missing. Ok, from your desciption and example I had clearly not fully understood the problem. -- An audit log with race -- type=SYSCALL msg=audit(1394281498.566:63): arch=4003 syscall=11 success=yes exit=0 a0=858c9c8 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 ppid=1747 pid=2662 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts1 ses=2 comm=truncated type=EXECVE msg=audit(1394281498.566:63): argc=1 a0=/tmp/printable-comm type=CWD msg=audit(1394281498.566:63): cwd=/root type=PATH msg=audit(1394281498.566:63): item=0 name=/tmp/printable-comm inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:bin_t:s0 nametype=NORMAL type=PATH msg=audit(1394281498.566:63): item=1 name=(null) inode=2360187 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL -- An audit log with race -- Even if you don't trust the comm= field, it is annoying for me that fields after comm= are missing in the audit log. More than annoying, that isn't acceptable. - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
Richard Guy Briggs wrote:
> > > > Likewise, audit_log_untrustedstring(ab, current->comm) is racy.
> > > > If task->comm was "Hello Linux" until audit_string_contains_control()
> > > > in
> > > > audit_log_n_untrustedstring() returns false, and becomes "Penguin"
> > > > before
> > > > memcpy() in audit_log_n_string() is called, memcpy() will emit
> > > > "Penguin\0nux"
> > > > into the audit log, which results in loss of information (e.g. SELinux
> > > > context) due to the unexpected '\0' byte.
> > >
> > > I expect the audit people don't like this? Also, how do audit and the
> > > LSM crap things interact? I thought they were both different piles of
> > > ignorable goo?
> >
> > I think the audit people do not like loss of information. Some of LSM
> > modules
> > are using audit subsystem for recording security related events. An example
> > is
> > shown later.
>
> This is true, however since comm it untrusted because it can be modified
> by the user audit doesn't trust it anyways, so who cares?
Excuse me, but did you understand this side effect correctly?
# ln /bin/true /tmp/printable-comm
# auditctl -a exit,always -S execve -F path=/tmp/printable-comm
# /tmp/printable-comm
# cat /var/log/audit/audit.log
If we didn't race, everything is fine.
-- An audit log without race --
type=SYSCALL msg=audit(1394281486.738:62): arch=4003 syscall=11 success=yes
exit=0 a0=8589c48 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 ppid=1747 pid=2657
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts1 ses=2
comm="printable-comm" exe="/tmp/printable-comm"
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=EXECVE msg=audit(1394281486.738:62): argc=1 a0="/tmp/printable-comm"
type=CWD msg=audit(1394281486.738:62): cwd="/root"
type=PATH msg=audit(1394281486.738:62): item=0 name="/tmp/printable-comm"
inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:bin_t:s0 nametype=NORMAL
type=PATH msg=audit(1394281486.738:62): item=1 name=(null) inode=2360187
dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL
-- An audit log without race --
But if we raced (you can use a (dangerous) SystemTap script shown below for
emulating this race condition
# stap -g -e '
function rewrite_comm(str:long) %{
strlcpy((char *) (long) STAP_ARG_str, "truncated", sizeof(current->comm));
%}
probe kernel.function("audit_log_n_string") {
if ($ab && $slen == 14 && kernel_string($string) == "printable-comm") {
rewrite_comm($string); printf("<%s>\n", kernel_string($string))
};
}
'
), you can see that fields after comm= (e.g. exe= subj= key= ) are missing.
-- An audit log with race --
type=SYSCALL msg=audit(1394281498.566:63): arch=4003 syscall=11 success=yes
exit=0 a0=858c9c8 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 ppid=1747 pid=2662
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts1 ses=2
comm="truncated
type=EXECVE msg=audit(1394281498.566:63): argc=1 a0="/tmp/printable-comm"
type=CWD msg=audit(1394281498.566:63): cwd="/root"
type=PATH msg=audit(1394281498.566:63): item=0 name="/tmp/printable-comm"
inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:bin_t:s0 nametype=NORMAL
type=PATH msg=audit(1394281498.566:63): item=1 name=(null) inode=2360187
dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL
-- An audit log with race --
Even if you don't trust the comm= field, it is annoying for me that fields
after comm= are missing in the audit log.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
Richard Guy Briggs wrote:
Likewise, audit_log_untrustedstring(ab, current-comm) is racy.
If task-comm was Hello Linux until audit_string_contains_control()
in
audit_log_n_untrustedstring() returns false, and becomes Penguin
before
memcpy() in audit_log_n_string() is called, memcpy() will emit
Penguin\0nux
into the audit log, which results in loss of information (e.g. SELinux
context) due to the unexpected '\0' byte.
I expect the audit people don't like this? Also, how do audit and the
LSM crap things interact? I thought they were both different piles of
ignorable goo?
I think the audit people do not like loss of information. Some of LSM
modules
are using audit subsystem for recording security related events. An example
is
shown later.
This is true, however since comm it untrusted because it can be modified
by the user audit doesn't trust it anyways, so who cares?
Excuse me, but did you understand this side effect correctly?
# ln /bin/true /tmp/printable-comm
# auditctl -a exit,always -S execve -F path=/tmp/printable-comm
# /tmp/printable-comm
# cat /var/log/audit/audit.log
If we didn't race, everything is fine.
-- An audit log without race --
type=SYSCALL msg=audit(1394281486.738:62): arch=4003 syscall=11 success=yes
exit=0 a0=8589c48 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 ppid=1747 pid=2657
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts1 ses=2
comm=printable-comm exe=/tmp/printable-comm
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=EXECVE msg=audit(1394281486.738:62): argc=1 a0=/tmp/printable-comm
type=CWD msg=audit(1394281486.738:62): cwd=/root
type=PATH msg=audit(1394281486.738:62): item=0 name=/tmp/printable-comm
inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:bin_t:s0 nametype=NORMAL
type=PATH msg=audit(1394281486.738:62): item=1 name=(null) inode=2360187
dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL
-- An audit log without race --
But if we raced (you can use a (dangerous) SystemTap script shown below for
emulating this race condition
# stap -g -e '
function rewrite_comm(str:long) %{
strlcpy((char *) (long) STAP_ARG_str, truncated, sizeof(current-comm));
%}
probe kernel.function(audit_log_n_string) {
if ($ab $slen == 14 kernel_string($string) == printable-comm) {
rewrite_comm($string); printf(%s\n, kernel_string($string))
};
}
'
), you can see that fields after comm= (e.g. exe= subj= key= ) are missing.
-- An audit log with race --
type=SYSCALL msg=audit(1394281498.566:63): arch=4003 syscall=11 success=yes
exit=0 a0=858c9c8 a1=85a6620 a2=858e4a0 a3=85a6620 items=2 ppid=1747 pid=2662
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts1 ses=2
comm=truncated
type=EXECVE msg=audit(1394281498.566:63): argc=1 a0=/tmp/printable-comm
type=CWD msg=audit(1394281498.566:63): cwd=/root
type=PATH msg=audit(1394281498.566:63): item=0 name=/tmp/printable-comm
inode=1970955 dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:bin_t:s0 nametype=NORMAL
type=PATH msg=audit(1394281498.566:63): item=1 name=(null) inode=2360187
dev=08:01 mode=0100755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL
-- An audit log with race --
Even if you don't trust the comm= field, it is annoying for me that fields
after comm= are missing in the audit log.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On 14/03/07, Tetsuo Handa wrote: > Peter Zijlstra wrote: > > https://lkml.org/lkml/2011/5/17/516 > Thank you for pointing that thread out. I found the following comment in that > thread. > > Linus Torvalds wrote: > | What folks? > | > | I don't think a new lock (or any lock) is at all appropriate. > | > | There's just no point. Just guarantee that the last byte is always > | zero, and you're done. > | > | If you just guarantee that, THERE IS NO RACE. The last byte never > | changes. You may get odd half-way strings, but you've trivially > | guaranteed that they are C NUL-terminated, with no locking, no memory > | ordering, no nothing. > > > Likewise, audit_log_untrustedstring(ab, current->comm) is racy. > > > If task->comm was "Hello Linux" until audit_string_contains_control() in > > > audit_log_n_untrustedstring() returns false, and becomes "Penguin" > > > before > > > memcpy() in audit_log_n_string() is called, memcpy() will emit > > > "Penguin\0nux" > > > into the audit log, which results in loss of information (e.g. SELinux > > > context) due to the unexpected '\0' byte. > > > > I expect the audit people don't like this? Also, how do audit and the > > LSM crap things interact? I thought they were both different piles of > > ignorable goo? > > I think the audit people do not like loss of information. Some of LSM modules > are using audit subsystem for recording security related events. An example is > shown later. This is true, however since comm it untrusted because it can be modified by the user audit doesn't trust it anyways, so who cares? > > How about you do what you're supposed to do when you want a reliable > > ->comm and use get_task_comm()? > > I always want a reliable ->comm . But get_task_comm() is not for calling from > vsnprintf(), for somebody might read task's commname from NMI context. > I tried to use RCU for reading from vsnprintf() but Linus will not accept it. - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
Peter Zijlstra wrote: > I would have actually expected it to stop emitting chars at \0. But > sure. Couldn't care less though; that's what you get, we all know this, > we've all been through this discussion several times. Get over it > already. > > One of the last threads on this is: > > https://lkml.org/lkml/2011/5/17/516 > Yes, I knew the attempts to make comm access consistent. I was wondering why such proposal is not yet accepted. Thank you for pointing that thread out. I found the following comment in that thread. Linus Torvalds wrote: | What folks? | | I don't think a new lock (or any lock) is at all appropriate. | | There's just no point. Just guarantee that the last byte is always | zero, and you're done. | | If you just guarantee that, THERE IS NO RACE. The last byte never | changes. You may get odd half-way strings, but you've trivially | guaranteed that they are C NUL-terminated, with no locking, no memory | ordering, no nothing. He stated that no locks are acceptable. Then, there is no room to insert rcu_read_lock()/rcu_read_unlock() into the reader side. The only way which can be implemented without any locks would be a heuristic atomicity shown later. > > Likewise, audit_log_untrustedstring(ab, current->comm) is racy. > > If task->comm was "Hello Linux" until audit_string_contains_control() in > > audit_log_n_untrustedstring() returns false, and becomes "Penguin" before > > memcpy() in audit_log_n_string() is called, memcpy() will emit > > "Penguin\0nux" > > into the audit log, which results in loss of information (e.g. SELinux > > context) due to the unexpected '\0' byte. > > I expect the audit people don't like this? Also, how do audit and the > LSM crap things interact? I thought they were both different piles of > ignorable goo? > I think the audit people do not like loss of information. Some of LSM modules are using audit subsystem for recording security related events. An example is shown later. > See there's not actually a problem statement here at all, so you can't > go about proposing solutions quite yet. > Please see the patch. > > Proposed solution: > > > > To fix abovementioned problem, I proposed commcpy() and "%pT" format > > specifier which does > > > > char tmp[16]; > > memcpy(tmp, task->comm, 16); > > tmp[15] = '\0'; > > sprintf(buf, "%s", tmp); > > > > instead of > > > > sprintf(buf, "%s", task->comm); > > > > . > > How about you do what you're supposed to do when you want a reliable > ->comm and use get_task_comm()? > I always want a reliable ->comm . But get_task_comm() is not for calling from vsnprintf(), for somebody might read task's commname from NMI context. I tried to use RCU for reading from vsnprintf() but Linus will not accept it. Regards. -- >From 212d983aa143954c089177942b6a7d9ab5393f8f Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Fri, 7 Mar 2014 11:00:24 +0900 Subject: [PATCH] Read/update task's commname sizeof(long) bytes at a time. "struct task_struct"->comm is read from various locations, but it is not updated atomically. As a result, readers may see a mixture of old and new commname. A bogus NUL byte in the mixture of old and new commname caused by TOCTTOU race can have unwanted side effect. For example, a sequence described below could happen when audit_log_untrustedstring(ab, tsk->comm) is called. (1) Initial tsk->comm contains "penguin-kernel\0\0" (2) CPU 0 attempts to audit tsk->comm using audit_log_untrustedstring(ab, tsk->comm) (3) audit_log_untrustedstring(ab, tsk->comm) calls audit_log_n_untrustedstring(ab, tsk->comm, strlen(tsk->comm)) (4) strlen(tsk->comm) is called and it returns 14 (5) audit_log_n_untrustedstring(ab, tsk->comm, 14) calls audit_string_contains_control(tsk->comm, 14) (6) audit_string_contains_control(tsk->comm, 14) returns 0 (7) CPU 1 attempts to write tsk->comm using set_task_comm(tsk, "penguin") (8) CPU 1 holds tsk->alloc_lock (9) CPU 1 calls strlcpy(tsk->comm, "penguin", 16) and now tsk->comm contains "penguin\0kernel\0\0" (10) CPU 1 releases task_struct->alloc_lock (11) audit_log_n_string(ab, tsk->comm, 14) calls memcpy(ptr, tsk->comm, 14) (12) memcpy(ptr, tsk->comm, 14) will copy "penguin\0kernel" (13) CPU 0 appends the rest of information and enqueues the entire log (14) the audit daemon reads the entire log, but the information after "penguin\0" are lost because of erroneously copied '\0' byte. Since system call auditing and LSM modules include task's commname in their record, the straying NUL byte can unexpectedly truncate that record, leading loss of information which should have been recorded. This patch introduces do_get_task_comm() and do_set_task_comm(). The former locklessly reads task's commname using "long" and the latter locklessly updates task's commname using "long". By using "long", we can reduce the possibility of getting the mixture of old and new commname, for
Re: [PATCH] Change task_struct-comm to use RCU.
Peter Zijlstra wrote: I would have actually expected it to stop emitting chars at \0. But sure. Couldn't care less though; that's what you get, we all know this, we've all been through this discussion several times. Get over it already. One of the last threads on this is: https://lkml.org/lkml/2011/5/17/516 Yes, I knew the attempts to make comm access consistent. I was wondering why such proposal is not yet accepted. Thank you for pointing that thread out. I found the following comment in that thread. Linus Torvalds wrote: | What folks? | | I don't think a new lock (or any lock) is at all appropriate. | | There's just no point. Just guarantee that the last byte is always | zero, and you're done. | | If you just guarantee that, THERE IS NO RACE. The last byte never | changes. You may get odd half-way strings, but you've trivially | guaranteed that they are C NUL-terminated, with no locking, no memory | ordering, no nothing. He stated that no locks are acceptable. Then, there is no room to insert rcu_read_lock()/rcu_read_unlock() into the reader side. The only way which can be implemented without any locks would be a heuristic atomicity shown later. Likewise, audit_log_untrustedstring(ab, current-comm) is racy. If task-comm was Hello Linux until audit_string_contains_control() in audit_log_n_untrustedstring() returns false, and becomes Penguin before memcpy() in audit_log_n_string() is called, memcpy() will emit Penguin\0nux into the audit log, which results in loss of information (e.g. SELinux context) due to the unexpected '\0' byte. I expect the audit people don't like this? Also, how do audit and the LSM crap things interact? I thought they were both different piles of ignorable goo? I think the audit people do not like loss of information. Some of LSM modules are using audit subsystem for recording security related events. An example is shown later. See there's not actually a problem statement here at all, so you can't go about proposing solutions quite yet. Please see the patch. Proposed solution: To fix abovementioned problem, I proposed commcpy() and %pT format specifier which does char tmp[16]; memcpy(tmp, task-comm, 16); tmp[15] = '\0'; sprintf(buf, %s, tmp); instead of sprintf(buf, %s, task-comm); . How about you do what you're supposed to do when you want a reliable -comm and use get_task_comm()? I always want a reliable -comm . But get_task_comm() is not for calling from vsnprintf(), for somebody might read task's commname from NMI context. I tried to use RCU for reading from vsnprintf() but Linus will not accept it. Regards. -- From 212d983aa143954c089177942b6a7d9ab5393f8f Mon Sep 17 00:00:00 2001 From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp Date: Fri, 7 Mar 2014 11:00:24 +0900 Subject: [PATCH] Read/update task's commname sizeof(long) bytes at a time. struct task_struct-comm is read from various locations, but it is not updated atomically. As a result, readers may see a mixture of old and new commname. A bogus NUL byte in the mixture of old and new commname caused by TOCTTOU race can have unwanted side effect. For example, a sequence described below could happen when audit_log_untrustedstring(ab, tsk-comm) is called. (1) Initial tsk-comm contains penguin-kernel\0\0 (2) CPU 0 attempts to audit tsk-comm using audit_log_untrustedstring(ab, tsk-comm) (3) audit_log_untrustedstring(ab, tsk-comm) calls audit_log_n_untrustedstring(ab, tsk-comm, strlen(tsk-comm)) (4) strlen(tsk-comm) is called and it returns 14 (5) audit_log_n_untrustedstring(ab, tsk-comm, 14) calls audit_string_contains_control(tsk-comm, 14) (6) audit_string_contains_control(tsk-comm, 14) returns 0 (7) CPU 1 attempts to write tsk-comm using set_task_comm(tsk, penguin) (8) CPU 1 holds tsk-alloc_lock (9) CPU 1 calls strlcpy(tsk-comm, penguin, 16) and now tsk-comm contains penguin\0kernel\0\0 (10) CPU 1 releases task_struct-alloc_lock (11) audit_log_n_string(ab, tsk-comm, 14) calls memcpy(ptr, tsk-comm, 14) (12) memcpy(ptr, tsk-comm, 14) will copy penguin\0kernel (13) CPU 0 appends the rest of information and enqueues the entire log (14) the audit daemon reads the entire log, but the information after penguin\0 are lost because of erroneously copied '\0' byte. Since system call auditing and LSM modules include task's commname in their record, the straying NUL byte can unexpectedly truncate that record, leading loss of information which should have been recorded. This patch introduces do_get_task_comm() and do_set_task_comm(). The former locklessly reads task's commname using long and the latter locklessly updates task's commname using long. By using long, we can reduce the possibility of getting the mixture of old and new commname, for reading/writing of long is atomic. The horizontal axis of below map is the strlen() of
Re: [PATCH] Change task_struct-comm to use RCU.
On 14/03/07, Tetsuo Handa wrote: Peter Zijlstra wrote: https://lkml.org/lkml/2011/5/17/516 Thank you for pointing that thread out. I found the following comment in that thread. Linus Torvalds wrote: | What folks? | | I don't think a new lock (or any lock) is at all appropriate. | | There's just no point. Just guarantee that the last byte is always | zero, and you're done. | | If you just guarantee that, THERE IS NO RACE. The last byte never | changes. You may get odd half-way strings, but you've trivially | guaranteed that they are C NUL-terminated, with no locking, no memory | ordering, no nothing. Likewise, audit_log_untrustedstring(ab, current-comm) is racy. If task-comm was Hello Linux until audit_string_contains_control() in audit_log_n_untrustedstring() returns false, and becomes Penguin before memcpy() in audit_log_n_string() is called, memcpy() will emit Penguin\0nux into the audit log, which results in loss of information (e.g. SELinux context) due to the unexpected '\0' byte. I expect the audit people don't like this? Also, how do audit and the LSM crap things interact? I thought they were both different piles of ignorable goo? I think the audit people do not like loss of information. Some of LSM modules are using audit subsystem for recording security related events. An example is shown later. This is true, however since comm it untrusted because it can be modified by the user audit doesn't trust it anyways, so who cares? How about you do what you're supposed to do when you want a reliable -comm and use get_task_comm()? I always want a reliable -comm . But get_task_comm() is not for calling from vsnprintf(), for somebody might read task's commname from NMI context. I tried to use RCU for reading from vsnprintf() but Linus will not accept it. - RGB -- Richard Guy Briggs rbri...@redhat.com Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On Wed, Feb 26, 2014 at 10:44:32PM +0900, Tetsuo Handa wrote:
> Thank you for reviewing, Paul.
No problem, but you do also need to address Lai Jiangshan's and
Peter Zijlstra's questions about the purpose of this patch. I was
looking at it only from a "does it work?" viewpoint.
> Paul E. McKenney wrote:
> > > +/**
> > > + * set_task_comm - Change task_struct->comm with tracer and perf hooks
> > > called.
> > > + *
> > > + * @tsk: Pointer to "struct task_struct".
> > > + * @buf: New comm name.
> > > + *
> > > + * Returns nothing.
> > > + */
> > > +void set_task_comm(struct task_struct *tsk, char *buf)
> > > +{
> > > + /*
> > > + * Question: Do we need to use task_lock()/task_unlock() ?
> >
> > The have-memory path through do_set_task_comm() does tolerate multiple
> > CPUs concurrently setting the comm of a given task, but the no-memory
> > path does not. I advise keeping the locking, at least unless/until
> > some workload is having lots of CPUs concurrently changing a given
> > task's comm. For some good reason, I hasten to add! ;-)
> >
> That question meant whether trace_task_rename(tsk, buf) needs to be serialized
> by tsk->alloc_lock. If trace_task_rename(tsk, buf) doesn't need to be
> serialized by tsk->alloc_lock, we can remove task_lock()/task_unlock().
I was attempting ot answer that question. ;-)
> > Another reason to get rid of the lock is to allow do_set_task_comm()
> > to sleep waiting for memory to become available, but not sure that
> > this is a good approach.
>
> I used GFP_ATOMIC because I don't know whether there are callers that depend
> on
> "changing task->comm does not sleep", for until today "changing task->comm did
> not sleep".
OK...
> > > +void do_set_task_comm(struct task_struct *tsk, const char *buf)
> > > +{
> > > + struct rcu_comm *comm;
> > > +
> > > + comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
> > > + if (likely(comm)) {
> > > + atomic_set(>usage, 1);
> > > + strncpy(comm->name, buf, TASK_COMM_LEN - 1);
> > > + smp_wmb(); /* Behave like rcu_assign_pointer(). */
> > > + comm = xchg(>rcu_comm, comm);
> >
> > The value-returning xchg() implies a full memory barrier, so the
> > smp_wmb() is not needed.
> >
> I see.
>
> > > + put_commname(comm);
> > > + } else {
> > > + /*
> > > + * Memory allocation failed. We have to tolerate loss of
> > > + * consistency.
> > > + *
> > > + * Question 1: Do we want to reserve some amount of static
> > > + * "struct rcu_comm" arrays for use upon allocation failures?
> > > + *
> > > + * Question 2: Do we perfer unchanged comm name over
> > > + * overwritten comm name because comm name is copy-on-write ?
> > > + */
> > > + WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
> > > + rcu_read_lock();
> > > + do {
> > > + comm = rcu_dereference(tsk->rcu_comm);
> > > + } while (!atomic_read(>usage));
> >
> > So the idea here is to avoid races with someone who might be freeing
> > this same ->rcu_comm structure, right? If we see a non-zero reference,
> > they might still free it, but that would be OK because we are in an
> > RCU read-side critical section, so it won't actually be freed until
> > we get done. And our change is being overwritten by someone else's
> > in that case, but that is OK because it could happen anyway.
> >
> Right.
>
> > So the loop shouldn't go through many cycles, especially if memory
> > remains low.
> >
> > If I am correct, might be worth a comment. Doubly so if I am wrong. ;-)
> >
> You are correct.
>
> What about Q1 and Q2, like below ?
I suggest reviewing the LKML thread that Peter Zijlstra pointed you at
before digging too much further into this. Especially this one:
https://lkml.org/lkml/2011/5/18/408
Thanx, Paul
> /* Usage is initialized with ATOMIC_INIT(-1). */
> static struct rcu_comm static_rcu_comm[CONFIG_RESERVED_COMMBUFFER];
>
> static void free_commname(struct rcu_head *rcu)
> {
> struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
> if (comm < _rcu_comm[0] ||
> comm >= _rcu_comm[CONFIG_RESERVED_COMMBUFFER])
> kmem_cache_free(commname_cachep, comm);
> else
> atomic_set(>usage, -1);
> }
>
> void do_set_task_comm(struct task_struct *tsk, const char *buf)
> {
> struct rcu_comm *comm;
>
> comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
> if (likely(comm))
> atomic_set(>usage, 1);
> else {
> int i;
>
> /* Memory allocation failed. Try static comm name buffer. */
> for (i = 0; i < CONFIG_RESERVED_COMMBUFFER; i++) {
> comm = _rcu_comm[i];
> if (atomic_read(>usage) != -1)
>
Re: [PATCH] Change task_struct->comm to use RCU.
Thank you for reviewing, Paul.
Paul E. McKenney wrote:
> > +/**
> > + * set_task_comm - Change task_struct->comm with tracer and perf hooks
> > called.
> > + *
> > + * @tsk: Pointer to "struct task_struct".
> > + * @buf: New comm name.
> > + *
> > + * Returns nothing.
> > + */
> > +void set_task_comm(struct task_struct *tsk, char *buf)
> > +{
> > + /*
> > +* Question: Do we need to use task_lock()/task_unlock() ?
>
> The have-memory path through do_set_task_comm() does tolerate multiple
> CPUs concurrently setting the comm of a given task, but the no-memory
> path does not. I advise keeping the locking, at least unless/until
> some workload is having lots of CPUs concurrently changing a given
> task's comm. For some good reason, I hasten to add! ;-)
>
That question meant whether trace_task_rename(tsk, buf) needs to be serialized
by tsk->alloc_lock. If trace_task_rename(tsk, buf) doesn't need to be
serialized by tsk->alloc_lock, we can remove task_lock()/task_unlock().
> Another reason to get rid of the lock is to allow do_set_task_comm()
> to sleep waiting for memory to become available, but not sure that
> this is a good approach.
I used GFP_ATOMIC because I don't know whether there are callers that depend on
"changing task->comm does not sleep", for until today "changing task->comm did
not sleep".
> > +void do_set_task_comm(struct task_struct *tsk, const char *buf)
> > +{
> > + struct rcu_comm *comm;
> > +
> > + comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
> > + if (likely(comm)) {
> > + atomic_set(>usage, 1);
> > + strncpy(comm->name, buf, TASK_COMM_LEN - 1);
> > + smp_wmb(); /* Behave like rcu_assign_pointer(). */
> > + comm = xchg(>rcu_comm, comm);
>
> The value-returning xchg() implies a full memory barrier, so the
> smp_wmb() is not needed.
>
I see.
> > + put_commname(comm);
> > + } else {
> > + /*
> > +* Memory allocation failed. We have to tolerate loss of
> > +* consistency.
> > +*
> > +* Question 1: Do we want to reserve some amount of static
> > +* "struct rcu_comm" arrays for use upon allocation failures?
> > +*
> > +* Question 2: Do we perfer unchanged comm name over
> > +* overwritten comm name because comm name is copy-on-write ?
> > +*/
> > + WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
> > + rcu_read_lock();
> > + do {
> > + comm = rcu_dereference(tsk->rcu_comm);
> > + } while (!atomic_read(>usage));
>
> So the idea here is to avoid races with someone who might be freeing
> this same ->rcu_comm structure, right? If we see a non-zero reference,
> they might still free it, but that would be OK because we are in an
> RCU read-side critical section, so it won't actually be freed until
> we get done. And our change is being overwritten by someone else's
> in that case, but that is OK because it could happen anyway.
>
Right.
> So the loop shouldn't go through many cycles, especially if memory
> remains low.
>
> If I am correct, might be worth a comment. Doubly so if I am wrong. ;-)
>
You are correct.
What about Q1 and Q2, like below ?
/* Usage is initialized with ATOMIC_INIT(-1). */
static struct rcu_comm static_rcu_comm[CONFIG_RESERVED_COMMBUFFER];
static void free_commname(struct rcu_head *rcu)
{
struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
if (comm < _rcu_comm[0] ||
comm >= _rcu_comm[CONFIG_RESERVED_COMMBUFFER])
kmem_cache_free(commname_cachep, comm);
else
atomic_set(>usage, -1);
}
void do_set_task_comm(struct task_struct *tsk, const char *buf)
{
struct rcu_comm *comm;
comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
if (likely(comm))
atomic_set(>usage, 1);
else {
int i;
/* Memory allocation failed. Try static comm name buffer. */
for (i = 0; i < CONFIG_RESERVED_COMMBUFFER; i++) {
comm = _rcu_comm[i];
if (atomic_read(>usage) != -1)
continue;
if (atomic_add_return(>usage, 2) == 1)
goto found;
put_commname(comm);
put_commname(comm);
}
/* No comm name buffer available. Keep current comm name. */
WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
return;
}
found:
strncpy(comm->name, buf, TASK_COMM_LEN - 1);
comm = xchg(>rcu_comm, comm);
put_commname(comm);
}
Since we can preallocate rcu_comm using GFP_KERNEL and return -ENOMEM (e.g.
prepare_bprm_creds() for execve() case,
Re: [PATCH] Change task_struct-comm to use RCU.
Thank you for reviewing, Paul.
Paul E. McKenney wrote:
+/**
+ * set_task_comm - Change task_struct-comm with tracer and perf hooks
called.
+ *
+ * @tsk: Pointer to struct task_struct.
+ * @buf: New comm name.
+ *
+ * Returns nothing.
+ */
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+ /*
+* Question: Do we need to use task_lock()/task_unlock() ?
The have-memory path through do_set_task_comm() does tolerate multiple
CPUs concurrently setting the comm of a given task, but the no-memory
path does not. I advise keeping the locking, at least unless/until
some workload is having lots of CPUs concurrently changing a given
task's comm. For some good reason, I hasten to add! ;-)
That question meant whether trace_task_rename(tsk, buf) needs to be serialized
by tsk-alloc_lock. If trace_task_rename(tsk, buf) doesn't need to be
serialized by tsk-alloc_lock, we can remove task_lock()/task_unlock().
Another reason to get rid of the lock is to allow do_set_task_comm()
to sleep waiting for memory to become available, but not sure that
this is a good approach.
I used GFP_ATOMIC because I don't know whether there are callers that depend on
changing task-comm does not sleep, for until today changing task-comm did
not sleep.
+void do_set_task_comm(struct task_struct *tsk, const char *buf)
+{
+ struct rcu_comm *comm;
+
+ comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
+ if (likely(comm)) {
+ atomic_set(comm-usage, 1);
+ strncpy(comm-name, buf, TASK_COMM_LEN - 1);
+ smp_wmb(); /* Behave like rcu_assign_pointer(). */
+ comm = xchg(tsk-rcu_comm, comm);
The value-returning xchg() implies a full memory barrier, so the
smp_wmb() is not needed.
I see.
+ put_commname(comm);
+ } else {
+ /*
+* Memory allocation failed. We have to tolerate loss of
+* consistency.
+*
+* Question 1: Do we want to reserve some amount of static
+* struct rcu_comm arrays for use upon allocation failures?
+*
+* Question 2: Do we perfer unchanged comm name over
+* overwritten comm name because comm name is copy-on-write ?
+*/
+ WARN_ONCE(1, Failed to allocate memory for comm name.\n);
+ rcu_read_lock();
+ do {
+ comm = rcu_dereference(tsk-rcu_comm);
+ } while (!atomic_read(comm-usage));
So the idea here is to avoid races with someone who might be freeing
this same -rcu_comm structure, right? If we see a non-zero reference,
they might still free it, but that would be OK because we are in an
RCU read-side critical section, so it won't actually be freed until
we get done. And our change is being overwritten by someone else's
in that case, but that is OK because it could happen anyway.
Right.
So the loop shouldn't go through many cycles, especially if memory
remains low.
If I am correct, might be worth a comment. Doubly so if I am wrong. ;-)
You are correct.
What about Q1 and Q2, like below ?
/* Usage is initialized with ATOMIC_INIT(-1). */
static struct rcu_comm static_rcu_comm[CONFIG_RESERVED_COMMBUFFER];
static void free_commname(struct rcu_head *rcu)
{
struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
if (comm static_rcu_comm[0] ||
comm = static_rcu_comm[CONFIG_RESERVED_COMMBUFFER])
kmem_cache_free(commname_cachep, comm);
else
atomic_set(comm-usage, -1);
}
void do_set_task_comm(struct task_struct *tsk, const char *buf)
{
struct rcu_comm *comm;
comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
if (likely(comm))
atomic_set(comm-usage, 1);
else {
int i;
/* Memory allocation failed. Try static comm name buffer. */
for (i = 0; i CONFIG_RESERVED_COMMBUFFER; i++) {
comm = static_rcu_comm[i];
if (atomic_read(comm-usage) != -1)
continue;
if (atomic_add_return(comm-usage, 2) == 1)
goto found;
put_commname(comm);
put_commname(comm);
}
/* No comm name buffer available. Keep current comm name. */
WARN_ONCE(1, Failed to allocate memory for comm name.\n);
return;
}
found:
strncpy(comm-name, buf, TASK_COMM_LEN - 1);
comm = xchg(tsk-rcu_comm, comm);
put_commname(comm);
}
Since we can preallocate rcu_comm using GFP_KERNEL and return -ENOMEM (e.g.
prepare_bprm_creds() for execve() case, copy_from_user() for prctl(PR_SET_NAME)
and comm_write() (i.e. /proc/$pid/comm ) cases), there
Re: [PATCH] Change task_struct-comm to use RCU.
On Wed, Feb 26, 2014 at 10:44:32PM +0900, Tetsuo Handa wrote:
Thank you for reviewing, Paul.
No problem, but you do also need to address Lai Jiangshan's and
Peter Zijlstra's questions about the purpose of this patch. I was
looking at it only from a does it work? viewpoint.
Paul E. McKenney wrote:
+/**
+ * set_task_comm - Change task_struct-comm with tracer and perf hooks
called.
+ *
+ * @tsk: Pointer to struct task_struct.
+ * @buf: New comm name.
+ *
+ * Returns nothing.
+ */
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+ /*
+ * Question: Do we need to use task_lock()/task_unlock() ?
The have-memory path through do_set_task_comm() does tolerate multiple
CPUs concurrently setting the comm of a given task, but the no-memory
path does not. I advise keeping the locking, at least unless/until
some workload is having lots of CPUs concurrently changing a given
task's comm. For some good reason, I hasten to add! ;-)
That question meant whether trace_task_rename(tsk, buf) needs to be serialized
by tsk-alloc_lock. If trace_task_rename(tsk, buf) doesn't need to be
serialized by tsk-alloc_lock, we can remove task_lock()/task_unlock().
I was attempting ot answer that question. ;-)
Another reason to get rid of the lock is to allow do_set_task_comm()
to sleep waiting for memory to become available, but not sure that
this is a good approach.
I used GFP_ATOMIC because I don't know whether there are callers that depend
on
changing task-comm does not sleep, for until today changing task-comm did
not sleep.
OK...
+void do_set_task_comm(struct task_struct *tsk, const char *buf)
+{
+ struct rcu_comm *comm;
+
+ comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
+ if (likely(comm)) {
+ atomic_set(comm-usage, 1);
+ strncpy(comm-name, buf, TASK_COMM_LEN - 1);
+ smp_wmb(); /* Behave like rcu_assign_pointer(). */
+ comm = xchg(tsk-rcu_comm, comm);
The value-returning xchg() implies a full memory barrier, so the
smp_wmb() is not needed.
I see.
+ put_commname(comm);
+ } else {
+ /*
+ * Memory allocation failed. We have to tolerate loss of
+ * consistency.
+ *
+ * Question 1: Do we want to reserve some amount of static
+ * struct rcu_comm arrays for use upon allocation failures?
+ *
+ * Question 2: Do we perfer unchanged comm name over
+ * overwritten comm name because comm name is copy-on-write ?
+ */
+ WARN_ONCE(1, Failed to allocate memory for comm name.\n);
+ rcu_read_lock();
+ do {
+ comm = rcu_dereference(tsk-rcu_comm);
+ } while (!atomic_read(comm-usage));
So the idea here is to avoid races with someone who might be freeing
this same -rcu_comm structure, right? If we see a non-zero reference,
they might still free it, but that would be OK because we are in an
RCU read-side critical section, so it won't actually be freed until
we get done. And our change is being overwritten by someone else's
in that case, but that is OK because it could happen anyway.
Right.
So the loop shouldn't go through many cycles, especially if memory
remains low.
If I am correct, might be worth a comment. Doubly so if I am wrong. ;-)
You are correct.
What about Q1 and Q2, like below ?
I suggest reviewing the LKML thread that Peter Zijlstra pointed you at
before digging too much further into this. Especially this one:
https://lkml.org/lkml/2011/5/18/408
Thanx, Paul
/* Usage is initialized with ATOMIC_INIT(-1). */
static struct rcu_comm static_rcu_comm[CONFIG_RESERVED_COMMBUFFER];
static void free_commname(struct rcu_head *rcu)
{
struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
if (comm static_rcu_comm[0] ||
comm = static_rcu_comm[CONFIG_RESERVED_COMMBUFFER])
kmem_cache_free(commname_cachep, comm);
else
atomic_set(comm-usage, -1);
}
void do_set_task_comm(struct task_struct *tsk, const char *buf)
{
struct rcu_comm *comm;
comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
if (likely(comm))
atomic_set(comm-usage, 1);
else {
int i;
/* Memory allocation failed. Try static comm name buffer. */
for (i = 0; i CONFIG_RESERVED_COMMBUFFER; i++) {
comm = static_rcu_comm[i];
if (atomic_read(comm-usage) != -1)
continue;
if (atomic_add_return(comm-usage, 2) == 1)
goto found;
put_commname(comm);
Re: [PATCH] Change task_struct->comm to use RCU.
On Tue, Feb 25, 2014 at 09:54:01PM +0900, Tetsuo Handa wrote:
> Lai Jiangshan wrote:
> > CC scheduler people.
> >
> > I can't figure out what we get with this patch.
> >
> OK. Welcome to this thread. I'll explain you what is going on.
>
> Current problem:
>
> printk("%s\n", task->comm) is racy because "%s" format specifier assumes
> that
> the corresponding argument does not change between strnlen() and the for
> loop
> at string() in lib/vsnprintf.c . If task->comm was "Hello Linux" until
> strnlen() and becomes "Penguin" before the for loop, "%s" will emit
> "Penguin\0nux" (note the unexpected '\0' byte and the garbage bytes).
I would have actually expected it to stop emitting chars at \0. But
sure. Couldn't care less though; that's what you get, we all know this,
we've all been through this discussion several times. Get over it
already.
One of the last threads on this is:
https://lkml.org/lkml/2011/5/17/516
> Likewise, audit_log_untrustedstring(ab, current->comm) is racy.
> If task->comm was "Hello Linux" until audit_string_contains_control() in
> audit_log_n_untrustedstring() returns false, and becomes "Penguin" before
> memcpy() in audit_log_n_string() is called, memcpy() will emit
> "Penguin\0nux"
> into the audit log, which results in loss of information (e.g. SELinux
> context) due to the unexpected '\0' byte.
I expect the audit people don't like this? Also, how do audit and the
LSM crap things interact? I thought they were both different piles of
ignorable goo?
See there's not actually a problem statement here at all, so you can't
go about proposing solutions quite yet.
> Proposed solution:
>
> To fix abovementioned problem, I proposed commcpy() and "%pT" format
> specifier which does
>
> char tmp[16];
> memcpy(tmp, task->comm, 16);
> tmp[15] = '\0';
> sprintf(buf, "%s", tmp);
>
> instead of
>
> sprintf(buf, "%s", task->comm);
>
> .
How about you do what you're supposed to do when you want a reliable
->comm and use get_task_comm()?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
Lai Jiangshan wrote:
> CC scheduler people.
>
> I can't figure out what we get with this patch.
>
OK. Welcome to this thread. I'll explain you what is going on.
Current problem:
printk("%s\n", task->comm) is racy because "%s" format specifier assumes that
the corresponding argument does not change between strnlen() and the for loop
at string() in lib/vsnprintf.c . If task->comm was "Hello Linux" until
strnlen() and becomes "Penguin" before the for loop, "%s" will emit
"Penguin\0nux" (note the unexpected '\0' byte and the garbage bytes).
Likewise, audit_log_untrustedstring(ab, current->comm) is racy.
If task->comm was "Hello Linux" until audit_string_contains_control() in
audit_log_n_untrustedstring() returns false, and becomes "Penguin" before
memcpy() in audit_log_n_string() is called, memcpy() will emit "Penguin\0nux"
into the audit log, which results in loss of information (e.g. SELinux
context) due to the unexpected '\0' byte.
Proposed solution:
To fix abovementioned problem, I proposed commcpy() and "%pT" format
specifier which does
char tmp[16];
memcpy(tmp, task->comm, 16);
tmp[15] = '\0';
sprintf(buf, "%s", tmp);
instead of
sprintf(buf, "%s", task->comm);
.
Remaining problem:
Although the proposed solution will prevent the caller from emitting the
unexpected '\0' byte and the garbage bytes, memcpy(tmp, task->comm, 16) in
the proposed solution is not atomic. That is, "%pT" does not emit the '\0'
byte like "Penguin\0nux" but "%pT" still might emit "Penguininux".
To fix this problem, I proposed protecting memcpy(tmp, task->comm, 16) part
using RCU. This patch is a design for how the update side of task->comm will
look like if we use RCU approach.
Of course, this approach depends on that nobody prefers the speed of reading
task->comm over the atomicity of reading task->comm . If somebody strongly
objects on the cost of calling rcu_read_lock()/rcu_read_unlock() for the
atomicity, I'm fine without this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On Tue, Feb 25, 2014 at 09:49:40AM +0800, Lai Jiangshan wrote: > >>From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001 > > From: Tetsuo Handa > > Date: Mon, 17 Feb 2014 14:32:11 +0900 > > Subject: [PATCH] Change task_struct->comm to use RCU. > > > > This patch changes task_struct->comm to be updated using RCU > > (unless memory allocation fails). This Changelog sucks; it fails to describe a problem; therefore there is no fix and the patch goes away, *poof*. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
On Tue, Feb 25, 2014 at 09:49:40AM +0800, Lai Jiangshan wrote: From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001 From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp Date: Mon, 17 Feb 2014 14:32:11 +0900 Subject: [PATCH] Change task_struct-comm to use RCU. This patch changes task_struct-comm to be updated using RCU (unless memory allocation fails). This Changelog sucks; it fails to describe a problem; therefore there is no fix and the patch goes away, *poof*. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
Lai Jiangshan wrote: CC scheduler people. I can't figure out what we get with this patch. OK. Welcome to this thread. I'll explain you what is going on. Current problem: printk(%s\n, task-comm) is racy because %s format specifier assumes that the corresponding argument does not change between strnlen() and the for loop at string() in lib/vsnprintf.c . If task-comm was Hello Linux until strnlen() and becomes Penguin before the for loop, %s will emit Penguin\0nux (note the unexpected '\0' byte and the garbage bytes). Likewise, audit_log_untrustedstring(ab, current-comm) is racy. If task-comm was Hello Linux until audit_string_contains_control() in audit_log_n_untrustedstring() returns false, and becomes Penguin before memcpy() in audit_log_n_string() is called, memcpy() will emit Penguin\0nux into the audit log, which results in loss of information (e.g. SELinux context) due to the unexpected '\0' byte. Proposed solution: To fix abovementioned problem, I proposed commcpy() and %pT format specifier which does char tmp[16]; memcpy(tmp, task-comm, 16); tmp[15] = '\0'; sprintf(buf, %s, tmp); instead of sprintf(buf, %s, task-comm); . Remaining problem: Although the proposed solution will prevent the caller from emitting the unexpected '\0' byte and the garbage bytes, memcpy(tmp, task-comm, 16) in the proposed solution is not atomic. That is, %pT does not emit the '\0' byte like Penguin\0nux but %pT still might emit Penguininux. To fix this problem, I proposed protecting memcpy(tmp, task-comm, 16) part using RCU. This patch is a design for how the update side of task-comm will look like if we use RCU approach. Of course, this approach depends on that nobody prefers the speed of reading task-comm over the atomicity of reading task-comm . If somebody strongly objects on the cost of calling rcu_read_lock()/rcu_read_unlock() for the atomicity, I'm fine without this patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct-comm to use RCU.
On Tue, Feb 25, 2014 at 09:54:01PM +0900, Tetsuo Handa wrote: Lai Jiangshan wrote: CC scheduler people. I can't figure out what we get with this patch. OK. Welcome to this thread. I'll explain you what is going on. Current problem: printk(%s\n, task-comm) is racy because %s format specifier assumes that the corresponding argument does not change between strnlen() and the for loop at string() in lib/vsnprintf.c . If task-comm was Hello Linux until strnlen() and becomes Penguin before the for loop, %s will emit Penguin\0nux (note the unexpected '\0' byte and the garbage bytes). I would have actually expected it to stop emitting chars at \0. But sure. Couldn't care less though; that's what you get, we all know this, we've all been through this discussion several times. Get over it already. One of the last threads on this is: https://lkml.org/lkml/2011/5/17/516 Likewise, audit_log_untrustedstring(ab, current-comm) is racy. If task-comm was Hello Linux until audit_string_contains_control() in audit_log_n_untrustedstring() returns false, and becomes Penguin before memcpy() in audit_log_n_string() is called, memcpy() will emit Penguin\0nux into the audit log, which results in loss of information (e.g. SELinux context) due to the unexpected '\0' byte. I expect the audit people don't like this? Also, how do audit and the LSM crap things interact? I thought they were both different piles of ignorable goo? See there's not actually a problem statement here at all, so you can't go about proposing solutions quite yet. Proposed solution: To fix abovementioned problem, I proposed commcpy() and %pT format specifier which does char tmp[16]; memcpy(tmp, task-comm, 16); tmp[15] = '\0'; sprintf(buf, %s, tmp); instead of sprintf(buf, %s, task-comm); . How about you do what you're supposed to do when you want a reliable -comm and use get_task_comm()? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
CC scheduler people.
I can't figure out what we get with this patch.
On 02/17/2014 07:27 PM, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
>> This is a draft patch which changes task_struct->comm to use RCU.
>
> Changes from previous draft version:
>
> Changed "struct rcu_comm" to use copy-on-write approach. Those multi-thread
> or multi-process applications which do not change comm name will consume
> memory for only one "struct rcu_comm".
>
> Changed do_commset() not to sleep.
>
> Changed to tolerate loss of consistency when memory allocation failed.
>
> Fixed race condition in copy_process().
>
> Regards.
> --
>>From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa
> Date: Mon, 17 Feb 2014 14:32:11 +0900
> Subject: [PATCH] Change task_struct->comm to use RCU.
>
> This patch changes task_struct->comm to be updated using RCU
> (unless memory allocation fails).
>
> Signed-off-by: Tetsuo Handa
> ---
> fs/exec.c | 145
> +++--
> include/linux/init_task.h |4 +-
> include/linux/sched.h | 37 ++--
> kernel/fork.c |9 +++
> kernel/kthread.c |4 +-
> kernel/sched/core.c |2 +-
> 6 files changed, 173 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a781dec..8a68ab3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
>
> /* Task command name length */
> #define TASK_COMM_LEN 16
> +struct rcu_comm {
> + char name[TASK_COMM_LEN];
> + atomic_t usage;
> + struct rcu_head rcu;
> +};
>
I think the name "rcu_comm" is not good, I suggest that s/rcu_comm/task_comm/g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
On Mon, Feb 17, 2014 at 08:27:31PM +0900, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > This is a draft patch which changes task_struct->comm to use RCU.
>
> Changes from previous draft version:
>
> Changed "struct rcu_comm" to use copy-on-write approach. Those multi-thread
> or multi-process applications which do not change comm name will consume
> memory for only one "struct rcu_comm".
>
> Changed do_commset() not to sleep.
>
> Changed to tolerate loss of consistency when memory allocation failed.
>
> Fixed race condition in copy_process().
>
> Regards.
> --
> >From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa
> Date: Mon, 17 Feb 2014 14:32:11 +0900
> Subject: [PATCH] Change task_struct->comm to use RCU.
>
> This patch changes task_struct->comm to be updated using RCU
> (unless memory allocation fails).
>
> Signed-off-by: Tetsuo Handa
A few questions and comments below. With those addressed,
Reviewed-by: Paul E. McKenney
> ---
> fs/exec.c | 145
> +++--
> include/linux/init_task.h |4 +-
> include/linux/sched.h | 37 ++--
> kernel/fork.c |9 +++
> kernel/kthread.c |4 +-
> kernel/sched/core.c |2 +-
> 6 files changed, 173 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a781dec..8a68ab3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
>
> /* Task command name length */
> #define TASK_COMM_LEN 16
> +struct rcu_comm {
> + char name[TASK_COMM_LEN];
> + atomic_t usage;
> + struct rcu_head rcu;
> +};
>
> #include
>
> @@ -1317,10 +1322,12 @@ struct task_struct {
>* credentials (COW) */
> const struct cred __rcu *cred; /* effective (overridable) subjective
> task
>* credentials (COW) */
> - char comm[TASK_COMM_LEN]; /* executable name excluding path
> - - access with [gs]et_task_comm (which lock
> -it with task_lock())
> - - initialized normally by setup_new_exec */
> + struct rcu_comm __rcu *rcu_comm; /* executable name excluding path (COW)
> + - update with set_task_comm() or
> + do_set_task_comm[_fmt]()
> + - read with commcpy() or %pT
> + - initialized normally by
> + setup_new_exec */
> /* file system info */
> int link_count, total_link_count;
> #ifdef CONFIG_SYSVIPC
> @@ -1792,6 +1799,23 @@ static inline cputime_t task_gtime(struct task_struct
> *t)
> extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut,
> cputime_t *st);
> extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t
> *ut, cputime_t *st);
>
> +/**
> + * commcpy - Copy task_struct->comm to buffer.
> + *
> + * @buf: Buffer to copy @tsk->comm which must be at least TASK_COMM_LEN
> bytes.
> + * @tsk: Pointer to "struct task_struct".
> + *
> + * Returns @buf .
> + */
> +static inline char *commcpy(char *buf, const struct task_struct *tsk)
> +{
> + rcu_read_lock();
> + memcpy(buf, rcu_dereference(tsk->rcu_comm)->name, TASK_COMM_LEN);
> + rcu_read_unlock();
> + buf[TASK_COMM_LEN - 1] = '\0';
> + return buf;
> +}
> +
> /*
> * Per process flags
> */
> @@ -2320,7 +2344,10 @@ struct task_struct *fork_idle(int);
> extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long
> flags);
>
> extern void set_task_comm(struct task_struct *tsk, char *from);
> -extern char *get_task_comm(char *to, struct task_struct *tsk);
> +extern void do_set_task_comm(struct task_struct *tsk, const char *buf);
> +extern void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt,
> ...)
> + __printf(2, 3);
> +extern void put_commname(struct rcu_comm *comm);
>
> #ifdef CONFIG_SMP
> void scheduler_ipi(void);
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 6df7f9f..b217f8c 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -154,7 +154,7 @@ extern struct task_group root_task_group;
> # define INIT_VTIME(tsk)
> #endif
>
> -#define INIT_TAS
Re: [PATCH] Change task_struct-comm to use RCU.
On Mon, Feb 17, 2014 at 08:27:31PM +0900, Tetsuo Handa wrote:
Tetsuo Handa wrote:
This is a draft patch which changes task_struct-comm to use RCU.
Changes from previous draft version:
Changed struct rcu_comm to use copy-on-write approach. Those multi-thread
or multi-process applications which do not change comm name will consume
memory for only one struct rcu_comm.
Changed do_commset() not to sleep.
Changed to tolerate loss of consistency when memory allocation failed.
Fixed race condition in copy_process().
Regards.
--
From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Mon, 17 Feb 2014 14:32:11 +0900
Subject: [PATCH] Change task_struct-comm to use RCU.
This patch changes task_struct-comm to be updated using RCU
(unless memory allocation fails).
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
A few questions and comments below. With those addressed,
Reviewed-by: Paul E. McKenney paul...@linux.vnet.ibm.com
---
fs/exec.c | 145
+++--
include/linux/init_task.h |4 +-
include/linux/sched.h | 37 ++--
kernel/fork.c |9 +++
kernel/kthread.c |4 +-
kernel/sched/core.c |2 +-
6 files changed, 173 insertions(+), 28 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..8a68ab3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
/* Task command name length */
#define TASK_COMM_LEN 16
+struct rcu_comm {
+ char name[TASK_COMM_LEN];
+ atomic_t usage;
+ struct rcu_head rcu;
+};
#include linux/spinlock.h
@@ -1317,10 +1322,12 @@ struct task_struct {
* credentials (COW) */
const struct cred __rcu *cred; /* effective (overridable) subjective
task
* credentials (COW) */
- char comm[TASK_COMM_LEN]; /* executable name excluding path
- - access with [gs]et_task_comm (which lock
-it with task_lock())
- - initialized normally by setup_new_exec */
+ struct rcu_comm __rcu *rcu_comm; /* executable name excluding path (COW)
+ - update with set_task_comm() or
+ do_set_task_comm[_fmt]()
+ - read with commcpy() or %pT
+ - initialized normally by
+ setup_new_exec */
/* file system info */
int link_count, total_link_count;
#ifdef CONFIG_SYSVIPC
@@ -1792,6 +1799,23 @@ static inline cputime_t task_gtime(struct task_struct
*t)
extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut,
cputime_t *st);
extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t
*ut, cputime_t *st);
+/**
+ * commcpy - Copy task_struct-comm to buffer.
+ *
+ * @buf: Buffer to copy @tsk-comm which must be at least TASK_COMM_LEN
bytes.
+ * @tsk: Pointer to struct task_struct.
+ *
+ * Returns @buf .
+ */
+static inline char *commcpy(char *buf, const struct task_struct *tsk)
+{
+ rcu_read_lock();
+ memcpy(buf, rcu_dereference(tsk-rcu_comm)-name, TASK_COMM_LEN);
+ rcu_read_unlock();
+ buf[TASK_COMM_LEN - 1] = '\0';
+ return buf;
+}
+
/*
* Per process flags
*/
@@ -2320,7 +2344,10 @@ struct task_struct *fork_idle(int);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long
flags);
extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern void do_set_task_comm(struct task_struct *tsk, const char *buf);
+extern void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt,
...)
+ __printf(2, 3);
+extern void put_commname(struct rcu_comm *comm);
#ifdef CONFIG_SMP
void scheduler_ipi(void);
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 6df7f9f..b217f8c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -154,7 +154,7 @@ extern struct task_group root_task_group;
# define INIT_VTIME(tsk)
#endif
-#define INIT_TASK_COMM swapper
+extern struct rcu_comm init_rcu_comm;
#ifdef CONFIG_RT_MUTEXES
# define INIT_RT_MUTEXES(tsk)
\
@@ -201,7 +201,7 @@ extern struct task_group root_task_group;
.group_leader = tsk, \
RCU_POINTER_INITIALIZER(real_cred, init_cred), \
RCU_POINTER_INITIALIZER(cred, init_cred), \
- .comm
Re: [PATCH] Change task_struct-comm to use RCU.
CC scheduler people.
I can't figure out what we get with this patch.
On 02/17/2014 07:27 PM, Tetsuo Handa wrote:
Tetsuo Handa wrote:
This is a draft patch which changes task_struct-comm to use RCU.
Changes from previous draft version:
Changed struct rcu_comm to use copy-on-write approach. Those multi-thread
or multi-process applications which do not change comm name will consume
memory for only one struct rcu_comm.
Changed do_commset() not to sleep.
Changed to tolerate loss of consistency when memory allocation failed.
Fixed race condition in copy_process().
Regards.
--
From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Mon, 17 Feb 2014 14:32:11 +0900
Subject: [PATCH] Change task_struct-comm to use RCU.
This patch changes task_struct-comm to be updated using RCU
(unless memory allocation fails).
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
fs/exec.c | 145
+++--
include/linux/init_task.h |4 +-
include/linux/sched.h | 37 ++--
kernel/fork.c |9 +++
kernel/kthread.c |4 +-
kernel/sched/core.c |2 +-
6 files changed, 173 insertions(+), 28 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..8a68ab3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
/* Task command name length */
#define TASK_COMM_LEN 16
+struct rcu_comm {
+ char name[TASK_COMM_LEN];
+ atomic_t usage;
+ struct rcu_head rcu;
+};
I think the name rcu_comm is not good, I suggest that s/rcu_comm/task_comm/g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change task_struct->comm to use RCU.
Tetsuo Handa wrote:
> This is a draft patch which changes task_struct->comm to use RCU.
Changes from previous draft version:
Changed "struct rcu_comm" to use copy-on-write approach. Those multi-thread
or multi-process applications which do not change comm name will consume
memory for only one "struct rcu_comm".
Changed do_commset() not to sleep.
Changed to tolerate loss of consistency when memory allocation failed.
Fixed race condition in copy_process().
Regards.
--
>From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
From: Tetsuo Handa
Date: Mon, 17 Feb 2014 14:32:11 +0900
Subject: [PATCH] Change task_struct->comm to use RCU.
This patch changes task_struct->comm to be updated using RCU
(unless memory allocation fails).
Signed-off-by: Tetsuo Handa
---
fs/exec.c | 145 +++--
include/linux/init_task.h |4 +-
include/linux/sched.h | 37 ++--
kernel/fork.c |9 +++
kernel/kthread.c |4 +-
kernel/sched/core.c |2 +-
6 files changed, 173 insertions(+), 28 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..8a68ab3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
/* Task command name length */
#define TASK_COMM_LEN 16
+struct rcu_comm {
+ char name[TASK_COMM_LEN];
+ atomic_t usage;
+ struct rcu_head rcu;
+};
#include
@@ -1317,10 +1322,12 @@ struct task_struct {
* credentials (COW) */
const struct cred __rcu *cred; /* effective (overridable) subjective
task
* credentials (COW) */
- char comm[TASK_COMM_LEN]; /* executable name excluding path
-- access with [gs]et_task_comm (which lock
- it with task_lock())
-- initialized normally by setup_new_exec */
+ struct rcu_comm __rcu *rcu_comm; /* executable name excluding path (COW)
+ - update with set_task_comm() or
+ do_set_task_comm[_fmt]()
+ - read with commcpy() or %pT
+ - initialized normally by
+ setup_new_exec */
/* file system info */
int link_count, total_link_count;
#ifdef CONFIG_SYSVIPC
@@ -1792,6 +1799,23 @@ static inline cputime_t task_gtime(struct task_struct *t)
extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut,
cputime_t *st);
extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t
*ut, cputime_t *st);
+/**
+ * commcpy - Copy task_struct->comm to buffer.
+ *
+ * @buf: Buffer to copy @tsk->comm which must be at least TASK_COMM_LEN bytes.
+ * @tsk: Pointer to "struct task_struct".
+ *
+ * Returns @buf .
+ */
+static inline char *commcpy(char *buf, const struct task_struct *tsk)
+{
+ rcu_read_lock();
+ memcpy(buf, rcu_dereference(tsk->rcu_comm)->name, TASK_COMM_LEN);
+ rcu_read_unlock();
+ buf[TASK_COMM_LEN - 1] = '\0';
+ return buf;
+}
+
/*
* Per process flags
*/
@@ -2320,7 +2344,10 @@ struct task_struct *fork_idle(int);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern void do_set_task_comm(struct task_struct *tsk, const char *buf);
+extern void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt, ...)
+ __printf(2, 3);
+extern void put_commname(struct rcu_comm *comm);
#ifdef CONFIG_SMP
void scheduler_ipi(void);
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 6df7f9f..b217f8c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -154,7 +154,7 @@ extern struct task_group root_task_group;
# define INIT_VTIME(tsk)
#endif
-#define INIT_TASK_COMM "swapper"
+extern struct rcu_comm init_rcu_comm;
#ifdef CONFIG_RT_MUTEXES
# define INIT_RT_MUTEXES(tsk) \
@@ -201,7 +201,7 @@ extern struct task_group root_task_group;
.group_leader = , \
RCU_POINTER_INITIALIZER(real_cred, _cred), \
RCU_POINTER_INITIALIZER(cred, _cred), \
- .comm = INIT_TASK_COMM, \
+ .rcu_comm = _rcu_comm, \
.thread = INIT_THREAD, \
.fs = _fs, \
.fil
Re: [PATCH] Change task_struct-comm to use RCU.
Tetsuo Handa wrote:
This is a draft patch which changes task_struct-comm to use RCU.
Changes from previous draft version:
Changed struct rcu_comm to use copy-on-write approach. Those multi-thread
or multi-process applications which do not change comm name will consume
memory for only one struct rcu_comm.
Changed do_commset() not to sleep.
Changed to tolerate loss of consistency when memory allocation failed.
Fixed race condition in copy_process().
Regards.
--
From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Mon, 17 Feb 2014 14:32:11 +0900
Subject: [PATCH] Change task_struct-comm to use RCU.
This patch changes task_struct-comm to be updated using RCU
(unless memory allocation fails).
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
fs/exec.c | 145 +++--
include/linux/init_task.h |4 +-
include/linux/sched.h | 37 ++--
kernel/fork.c |9 +++
kernel/kthread.c |4 +-
kernel/sched/core.c |2 +-
6 files changed, 173 insertions(+), 28 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..8a68ab3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
/* Task command name length */
#define TASK_COMM_LEN 16
+struct rcu_comm {
+ char name[TASK_COMM_LEN];
+ atomic_t usage;
+ struct rcu_head rcu;
+};
#include linux/spinlock.h
@@ -1317,10 +1322,12 @@ struct task_struct {
* credentials (COW) */
const struct cred __rcu *cred; /* effective (overridable) subjective
task
* credentials (COW) */
- char comm[TASK_COMM_LEN]; /* executable name excluding path
-- access with [gs]et_task_comm (which lock
- it with task_lock())
-- initialized normally by setup_new_exec */
+ struct rcu_comm __rcu *rcu_comm; /* executable name excluding path (COW)
+ - update with set_task_comm() or
+ do_set_task_comm[_fmt]()
+ - read with commcpy() or %pT
+ - initialized normally by
+ setup_new_exec */
/* file system info */
int link_count, total_link_count;
#ifdef CONFIG_SYSVIPC
@@ -1792,6 +1799,23 @@ static inline cputime_t task_gtime(struct task_struct *t)
extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut,
cputime_t *st);
extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t
*ut, cputime_t *st);
+/**
+ * commcpy - Copy task_struct-comm to buffer.
+ *
+ * @buf: Buffer to copy @tsk-comm which must be at least TASK_COMM_LEN bytes.
+ * @tsk: Pointer to struct task_struct.
+ *
+ * Returns @buf .
+ */
+static inline char *commcpy(char *buf, const struct task_struct *tsk)
+{
+ rcu_read_lock();
+ memcpy(buf, rcu_dereference(tsk-rcu_comm)-name, TASK_COMM_LEN);
+ rcu_read_unlock();
+ buf[TASK_COMM_LEN - 1] = '\0';
+ return buf;
+}
+
/*
* Per process flags
*/
@@ -2320,7 +2344,10 @@ struct task_struct *fork_idle(int);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern void do_set_task_comm(struct task_struct *tsk, const char *buf);
+extern void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt, ...)
+ __printf(2, 3);
+extern void put_commname(struct rcu_comm *comm);
#ifdef CONFIG_SMP
void scheduler_ipi(void);
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 6df7f9f..b217f8c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -154,7 +154,7 @@ extern struct task_group root_task_group;
# define INIT_VTIME(tsk)
#endif
-#define INIT_TASK_COMM swapper
+extern struct rcu_comm init_rcu_comm;
#ifdef CONFIG_RT_MUTEXES
# define INIT_RT_MUTEXES(tsk) \
@@ -201,7 +201,7 @@ extern struct task_group root_task_group;
.group_leader = tsk, \
RCU_POINTER_INITIALIZER(real_cred, init_cred), \
RCU_POINTER_INITIALIZER(cred, init_cred), \
- .comm = INIT_TASK_COMM, \
+ .rcu_comm = init_rcu_comm, \
.thread = INIT_THREAD, \
.fs = init_fs
