Re: [PATCH] audit: clear thread flag for new children
On Thursday 01 November 2007 01:23:24 pm Tony Jones wrote:
> > We are looking into this - at one time it did. Someone should follow up
> > with a path correcting this soon. But I doubt the audit system will work
> > correctly if the flag gets removed as there is no good way to add it
> > again later.
>
> So on the syscall path you're now going to something like (pseudocode):
> if (unlikely(current->audit_context)) {audit_syscall_entry()}
> else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}
Something like that. The TIF flag is to pick off processes that we are
interested in auditing, for performance reasons if audit is disabled the
context is not created. But if auditing is re-enabled, processes can
be "repaired" so that the are auditable again by allocating the context on
the fly the first time we see a process. Without the flag, they never get
steered into the audit system where this can happen.
> I agree, if this is what you want to do, then clearing the thread flag
> would be a bad idea.
If any other performance improvements jumps out at you, please bring them up.
> I'd assumed the current behaviour was by design as allocating contexts at
> syscall time doesn't seem a great idea but if you need the functionality,
> you need the functionality.
Yep
-Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Thu, Nov 01, 2007 at 10:33:52AM -0400, Steve Grubb wrote:
> On Monday 29 October 2007 07:15:30 pm Tony Jones wrote:
> > On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote:
> > > So when audit is re-enabled, how do you make that task auditable?
> >
> > No idea. How do you do it currently? HINT: current->audit_context == NULL
> > for these tasks. If !audit_enabled, then audit_alloc() is not going to
> > allocate an audit_context for the task.
>
> We are looking into this - at one time it did. Someone should follow up with
> a path correcting this soon. But I doubt the audit system will work correctly
> if the flag gets removed as there is no good way to add it again later.
So on the syscall path you're now going to something like (pseudocode):
if (unlikely(current->audit_context)) {audit_syscall_entry()}
else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}
I agree, if this is what you want to do, then clearing the thread flag would
be a bad idea. I'd assumed the current behaviour was by design as allocating
contexts at syscall time doesn't seem a great idea but if you need the
functionality, you need the functionality.
Tony
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 07:15:30 pm Tony Jones wrote: > On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote: > > So when audit is re-enabled, how do you make that task auditable? > > No idea. How do you do it currently? HINT: current->audit_context == NULL > for these tasks. If !audit_enabled, then audit_alloc() is not going to > allocate an audit_context for the task. We are looking into this - at one time it did. Someone should follow up with a path correcting this soon. But I doubt the audit system will work correctly if the flag gets removed as there is no good way to add it again later. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 07:15:30 pm Tony Jones wrote: On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote: So when audit is re-enabled, how do you make that task auditable? No idea. How do you do it currently? HINT: current-audit_context == NULL for these tasks. If !audit_enabled, then audit_alloc() is not going to allocate an audit_context for the task. We are looking into this - at one time it did. Someone should follow up with a path correcting this soon. But I doubt the audit system will work correctly if the flag gets removed as there is no good way to add it again later. -Steve - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Thu, Nov 01, 2007 at 10:33:52AM -0400, Steve Grubb wrote:
On Monday 29 October 2007 07:15:30 pm Tony Jones wrote:
On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote:
So when audit is re-enabled, how do you make that task auditable?
No idea. How do you do it currently? HINT: current-audit_context == NULL
for these tasks. If !audit_enabled, then audit_alloc() is not going to
allocate an audit_context for the task.
We are looking into this - at one time it did. Someone should follow up with
a path correcting this soon. But I doubt the audit system will work correctly
if the flag gets removed as there is no good way to add it again later.
So on the syscall path you're now going to something like (pseudocode):
if (unlikely(current-audit_context)) {audit_syscall_entry()}
else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}
I agree, if this is what you want to do, then clearing the thread flag would
be a bad idea. I'd assumed the current behaviour was by design as allocating
contexts at syscall time doesn't seem a great idea but if you need the
functionality, you need the functionality.
Tony
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Thursday 01 November 2007 01:23:24 pm Tony Jones wrote:
We are looking into this - at one time it did. Someone should follow up
with a path correcting this soon. But I doubt the audit system will work
correctly if the flag gets removed as there is no good way to add it
again later.
So on the syscall path you're now going to something like (pseudocode):
if (unlikely(current-audit_context)) {audit_syscall_entry()}
else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}
Something like that. The TIF flag is to pick off processes that we are
interested in auditing, for performance reasons if audit is disabled the
context is not created. But if auditing is re-enabled, processes can
be repaired so that the are auditable again by allocating the context on
the fly the first time we see a process. Without the flag, they never get
steered into the audit system where this can happen.
I agree, if this is what you want to do, then clearing the thread flag
would be a bad idea.
If any other performance improvements jumps out at you, please bring them up.
I'd assumed the current behaviour was by design as allocating contexts at
syscall time doesn't seem a great idea but if you need the functionality,
you need the functionality.
Yep
-Steve
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote: > If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into > audit_syscall_entry. It becomes unauditable. True but a task where current->audit_context == NULL is going to immediately BUG out in audit_syscall_entry. This is why the invocations of audit_syscall_entry() are conditional on current->audit_context. > So when audit is re-enabled, how do you make that task auditable? No idea. How do you do it currently? HINT: current->audit_context == NULL for these tasks. If !audit_enabled, then audit_alloc() is not going to allocate an audit_context for the task. I'm very curious how you think one of these tasks becomes auditable later on once audit is re-enabled, regardless of the value of TIF_SYSCALL_AUDIT. Tony - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 01:20:58 pm Tony Jones wrote: > > The problem is that removing that flag makes the children unauditable in > > the future. The only place that flag gets set is during fork. > > I don't see this. If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into audit_syscall_entry. It becomes unauditable. > The case that would be undesirable would be for a task to have an audit > context but to not have the thread flag enabled. That would just be a small allocation of memory that will be returned when the process exits. From an auditing PoV, something that is undesirable is the inability to audit a process that you want to audit. > > Unless I'm missing something, to make all children auditable again would > > mean stopping all processes and or'ing that flag into all thread info > > areas. > > I think you are. Or maybe the code was different two years ago so that the > above made sense. > > In the above scenario, audit is disabled, a new child is forked, we bail > early so there is no audit context (and now there is no flag in the thread > area). Currently there is no way this task is ever going to be audited as > there is no audit context. So when audit is re-enabled, how do you make that task auditable? > If this task forks a new child, at this point the value of audit enabled > will determine if there should be a context allocated and it will allocate > the TIF flag also. In the new child, but not the parent. > I don't see your stopping all processes scenario. That is so you can walk the process table and "or" the bit in unconditionally. All processes need to be auditable or you've got a security hole. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Sat, Oct 27, 2007 at 10:21:39AM -0400, Steve Grubb wrote: > On Friday 26 October 2007 04:42:28 pm Tony Jones wrote: > > Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit > > context creation has been disabled (auditctl -e0). This can cause new > > children forked from a parent created when audit was enabled to not take > > the fastest syscall path thru entry.S > > This came up almost 2 years ago: > > https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html I was not aware of this, thanks. > The problem is that removing that flag makes the children unauditable in the > future. The only place that flag gets set is during fork. I don't see this. The case that would be undesirable would be for a task to have an audit context but to not have the thread flag enabled. That isn't the case. This was the point Chris made in his Ack, although perhaps somewhat tersely. > Unless I'm missing something, to make all children auditable again would > mean stopping all processes and or'ing that flag into all thread info areas. I think you are. Or maybe the code was different two years ago so that the above made sense. In the above scenario, audit is disabled, a new child is forked, we bail early so there is no audit context (and now there is no flag in the thread area). Currently there is no way this task is ever going to be audited as there is no audit context.If this task forks a new child, at this point the value of audit enabled will determine if there should be a context allocated and it will allocate the TIF flag also. I don't see your stopping all processes scenario. > I do not want to propose that patch to LKML. :) ;-) Tony - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Sat, Oct 27, 2007 at 10:21:39AM -0400, Steve Grubb wrote: On Friday 26 October 2007 04:42:28 pm Tony Jones wrote: Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit context creation has been disabled (auditctl -e0). This can cause new children forked from a parent created when audit was enabled to not take the fastest syscall path thru entry.S This came up almost 2 years ago: https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html I was not aware of this, thanks. The problem is that removing that flag makes the children unauditable in the future. The only place that flag gets set is during fork. I don't see this. The case that would be undesirable would be for a task to have an audit context but to not have the thread flag enabled. That isn't the case. This was the point Chris made in his Ack, although perhaps somewhat tersely. Unless I'm missing something, to make all children auditable again would mean stopping all processes and or'ing that flag into all thread info areas. I think you are. Or maybe the code was different two years ago so that the above made sense. In the above scenario, audit is disabled, a new child is forked, we bail early so there is no audit context (and now there is no flag in the thread area). Currently there is no way this task is ever going to be audited as there is no audit context.If this task forks a new child, at this point the value of audit enabled will determine if there should be a context allocated and it will allocate the TIF flag also. I don't see your stopping all processes scenario. I do not want to propose that patch to LKML. :) ;-) Tony - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 01:20:58 pm Tony Jones wrote: The problem is that removing that flag makes the children unauditable in the future. The only place that flag gets set is during fork. I don't see this. If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into audit_syscall_entry. It becomes unauditable. The case that would be undesirable would be for a task to have an audit context but to not have the thread flag enabled. That would just be a small allocation of memory that will be returned when the process exits. From an auditing PoV, something that is undesirable is the inability to audit a process that you want to audit. Unless I'm missing something, to make all children auditable again would mean stopping all processes and or'ing that flag into all thread info areas. I think you are. Or maybe the code was different two years ago so that the above made sense. In the above scenario, audit is disabled, a new child is forked, we bail early so there is no audit context (and now there is no flag in the thread area). Currently there is no way this task is ever going to be audited as there is no audit context. So when audit is re-enabled, how do you make that task auditable? If this task forks a new child, at this point the value of audit enabled will determine if there should be a context allocated and it will allocate the TIF flag also. In the new child, but not the parent. I don't see your stopping all processes scenario. That is so you can walk the process table and or the bit in unconditionally. All processes need to be auditable or you've got a security hole. -Steve - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote: If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into audit_syscall_entry. It becomes unauditable. True but a task where current-audit_context == NULL is going to immediately BUG out in audit_syscall_entry. This is why the invocations of audit_syscall_entry() are conditional on current-audit_context. So when audit is re-enabled, how do you make that task auditable? No idea. How do you do it currently? HINT: current-audit_context == NULL for these tasks. If !audit_enabled, then audit_alloc() is not going to allocate an audit_context for the task. I'm very curious how you think one of these tasks becomes auditable later on once audit is re-enabled, regardless of the value of TIF_SYSCALL_AUDIT. Tony - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Friday 26 October 2007 04:42:28 pm Tony Jones wrote: > Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit > context creation has been disabled (auditctl -e0). This can cause new > children forked from a parent created when audit was enabled to not take > the fastest syscall path thru entry.S This came up almost 2 years ago: https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html The problem is that removing that flag makes the children unauditable in the future. The only place that flag gets set is during fork. Unless I'm missing something, to make all children auditable again would mean stopping all processes and or'ing that flag into all thread info areas. I do not want to propose that patch to LKML. :) -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Friday 26 October 2007 04:42:28 pm Tony Jones wrote: Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit context creation has been disabled (auditctl -e0). This can cause new children forked from a parent created when audit was enabled to not take the fastest syscall path thru entry.S This came up almost 2 years ago: https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html The problem is that removing that flag makes the children unauditable in the future. The only place that flag gets set is during fork. Unless I'm missing something, to make all children auditable again would mean stopping all processes and or'ing that flag into all thread info areas. I do not want to propose that patch to LKML. :) -Steve - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
* Tony Jones ([EMAIL PROTECTED]) wrote: > From: Tony Jones <[EMAIL PROTECTED]> > Minor performance enhancement. > > Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit > context creation has been disabled (auditctl -e0). This can cause new > children > forked from a parent created when audit was enabled to not take the fastest > syscall path thru entry.S > > Signed-off-by: Tony Jones <[EMAIL PROTECTED]> Yeah, I think that's the right thing to do. Doesn't have an audit_context anyway. Acked-by: Chris Wright <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] audit: clear thread flag for new children
From: Tony Jones <[EMAIL PROTECTED]>
Minor performance enhancement.
Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit
context creation has been disabled (auditctl -e0). This can cause new children
forked from a parent created when audit was enabled to not take the fastest
syscall path thru entry.S
Signed-off-by: Tony Jones <[EMAIL PROTECTED]>
---
kernel/auditsc.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -814,8 +814,10 @@ int audit_alloc(struct task_struct *tsk)
struct audit_context *context;
enum audit_state state;
- if (likely(!audit_enabled))
+ if (likely(!audit_enabled)) {
+ clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
return 0; /* Return if not auditing. */
+ }
state = audit_filter_task(tsk);
if (likely(state == AUDIT_DISABLED))
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] audit: clear thread flag for new children
From: Tony Jones [EMAIL PROTECTED]
Minor performance enhancement.
Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit
context creation has been disabled (auditctl -e0). This can cause new children
forked from a parent created when audit was enabled to not take the fastest
syscall path thru entry.S
Signed-off-by: Tony Jones [EMAIL PROTECTED]
---
kernel/auditsc.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -814,8 +814,10 @@ int audit_alloc(struct task_struct *tsk)
struct audit_context *context;
enum audit_state state;
- if (likely(!audit_enabled))
+ if (likely(!audit_enabled)) {
+ clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
return 0; /* Return if not auditing. */
+ }
state = audit_filter_task(tsk);
if (likely(state == AUDIT_DISABLED))
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
* Tony Jones ([EMAIL PROTECTED]) wrote: From: Tony Jones [EMAIL PROTECTED] Minor performance enhancement. Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit context creation has been disabled (auditctl -e0). This can cause new children forked from a parent created when audit was enabled to not take the fastest syscall path thru entry.S Signed-off-by: Tony Jones [EMAIL PROTECTED] Yeah, I think that's the right thing to do. Doesn't have an audit_context anyway. Acked-by: Chris Wright [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
