Re: [PATCH] audit: listen in all network namespaces

Re: [PATCH] audit: listen in all network namespaces

[Gao feng]

On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
> Convert audit from only listening in init_net to use register_pernet_subsys()
> to dynamically manage the netlink socket list.
> 
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---

I think it's the time for us to discuss if we should revert this
commit, since this one prevent me from continuing to achieve
audit namespace.


The major problem is in kaudit_send_skb, we have no idea which
audit sock the skb should send to.

in this patch, there only is one auditd proecess, so the
audit_sock is the only one. but when we have audit namespace.
there will be multi audit socks. we have to store audit_sock
into auditns(auditns will be passed to kauditd_send_skb),
this will cause auditns have to get a reference of netns.
and for some reason(netfilter audit target), netns will
get reference of auditns too. this is terrible...

So why not we revert this one, and use a very simple one to
replace it? the below patch will save us from the refer to
each other case, achieve the same effect.

what's your opinion?


Add a compare function which always return true for
audit netlink socket, this will cause audit netlink
sockets netns unaware, and no matter which netns the
user space audit netlink sockets belong to, they all
can find out and communicate with audit_sock.

This gets rid of the necessary to create per-netns
audit kernel side socket(audit_sock), it's pain to
depend on and get reference of netns for auditns.

Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 kernel/audit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..468950b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -886,12 +886,18 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }

+static bool audit_compare(struct net *net, struct sock *sk)
+{
+	return true;
+}
+
 /* Initialize audit support at boot time. */
 static int __init audit_init(void)
 {
 	int i;
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.compare = audit_compare,
 	};

 	if (audit_initialized == AUDIT_DISABLED)

Re: [PATCH] audit: listen in all network namespaces

[Eric Paris]

On Thu, 2013-12-19 at 11:59 +0800, Gao feng wrote:
> On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
> > Convert audit from only listening in init_net to use register_pernet_subsys()
> > to dynamically manage the netlink socket list.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> 
> I think it's the time for us to discuss if we should revert this
> commit, since this one prevent me from continuing to achieve
> audit namespace.
> 
> 
> The major problem is in kaudit_send_skb, we have no idea which
> audit sock the skb should send to.

right, we have problems here no matter what...

If we stick with the current approach you will need to know socket +
portid.  With your approach one only needs to know portid.  Since these
are can both be part of the audit_ns structure I don't see a huge
difference...

> we have to store audit_sock
> into auditns(auditns will be passed to kauditd_send_skb),
> this will cause auditns have to get a reference of netns.
> and for some reason(netfilter audit target), netns will
> get reference of auditns too. this is terrible...

I'm not sure I agree/understand this entirely...

> So why not we revert this one, and use a very simple one to
> replace it? the below patch will save us from the refer to
> each other case, achieve the same effect.
> 
> what's your opinion?

Help me go all the way back to the beginning.  What's our end goal here
again?

When thinking about this I realized we have another problem that I don't
think we've considered.  Which makes me lean away from the single
socket/kauditd  :(

I we have one socket and one kauditd ANY auditd can completely freeze
the audit system.  Which seems problematic, especially if there isn't
equal levels of trust between the different namespaces...  If one auditd
gets hung (intentionally or not) the kernel will never send another
audit message....

Makes me think we really need a kauditd thread per namespace, possibly
an skb queue per namespace.  At which point an audit socket per
namespace makes a lot of sense too....

Re: [PATCH] audit: listen in all network namespaces

[Gao feng]

On 12/20/2013 02:40 AM, Eric Paris wrote:
> On Thu, 2013-12-19 at 11:59 +0800, Gao feng wrote:
>> On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
>>> Convert audit from only listening in init_net to use register_pernet_subsys()
>>> to dynamically manage the netlink socket list.
>>>
>>> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>
>> I think it's the time for us to discuss if we should revert this
>> commit, since this one prevent me from continuing to achieve
>> audit namespace.
>>
>>
>> The major problem is in kaudit_send_skb, we have no idea which
>> audit sock the skb should send to.
> 
> right, we have problems here no matter what...
> 
> If we stick with the current approach you will need to know socket +
> portid.  With your approach one only needs to know portid.  Since these
> are can both be part of the audit_ns structure I don't see a huge
> difference...
> 
>> we have to store audit_sock
>> into auditns(auditns will be passed to kauditd_send_skb),
>> this will cause auditns have to get a reference of netns.
>> and for some reason(netfilter audit target), netns will
>> get reference of auditns too. this is terrible...
> 
> I'm not sure I agree/understand this entirely...
> 

My brain must be destroyed, I need to think about if auditns
should get reference of netns. it's not clear to me now. :(
but I intend to think you are right.

>> So why not we revert this one, and use a very simple one to
>> replace it? the below patch will save us from the refer to
>> each other case, achieve the same effect.
>>
>> what's your opinion?
> 
> Help me go all the way back to the beginning.  What's our end goal here
> again?
> 
> When thinking about this I realized we have another problem that I don't
> think we've considered.  Which makes me lean away from the single
> socket/kauditd  :(
> 
> I we have one socket and one kauditd ANY auditd can completely freeze
> the audit system.  Which seems problematic, especially if there isn't
> equal levels of trust between the different namespaces...  If one auditd
> gets hung (intentionally or not) the kernel will never send another
> audit message....
> 
> Makes me think we really need a kauditd thread per namespace, possibly
> an skb queue per namespace.  At which point an audit socket per
> namespace makes a lot of sense too....
> 

You are right, and My prototype supports per kauditd/auditd/sbk queue per
audit namespace. one auditd freeze in one auditns will not affect audit
subsystem in another auditns.

Thanks!

Re: [PATCH] audit: listen in all network namespaces

[Gao feng]

On 12/20/2013 02:40 AM, Eric Paris wrote:
> On Thu, 2013-12-19 at 11:59 +0800, Gao feng wrote:
>> On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
>>> Convert audit from only listening in init_net to use register_pernet_subsys()
>>> to dynamically manage the netlink socket list.
>>>
>>> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>
>> I think it's the time for us to discuss if we should revert this
>> commit, since this one prevent me from continuing to achieve
>> audit namespace.
>>
>>
>> The major problem is in kaudit_send_skb, we have no idea which
>> audit sock the skb should send to.
> 
> right, we have problems here no matter what...
> 
> If we stick with the current approach you will need to know socket +
> portid.  With your approach one only needs to know portid.  Since these
> are can both be part of the audit_ns structure I don't see a huge
> difference...
> 
>> we have to store audit_sock
>> into auditns(auditns will be passed to kauditd_send_skb),
>> this will cause auditns have to get a reference of netns.
>> and for some reason(netfilter audit target), netns will
>> get reference of auditns too. this is terrible...
> 
> I'm not sure I agree/understand this entirely...
> 

Yes, the audit_sock is created and destroyed by net namespace,
so if auditns wants to use audit_sock, it must prevent netns
from being destroyed. so auditns has to get reference of netns.

and in some case, netns will get reference of auditns too. this
is complex than making audit_sock global and getting rid of this
reference.

Re: [PATCH] audit: listen in all network namespaces

[Eric Paris]

On Fri, 2013-12-20 at 10:46 +0800, Gao feng wrote:
> On 12/20/2013 02:40 AM, Eric Paris wrote:
> > On Thu, 2013-12-19 at 11:59 +0800, Gao feng wrote:
> >> On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:

> >> we have to store audit_sock
> >> into auditns(auditns will be passed to kauditd_send_skb),
> >> this will cause auditns have to get a reference of netns.
> >> and for some reason(netfilter audit target), netns will
> >> get reference of auditns too. this is terrible...
> > 
> > I'm not sure I agree/understand this entirely...
> > 
> 
> Yes, the audit_sock is created and destroyed by net namespace,
> so if auditns wants to use audit_sock, it must prevent netns
> from being destroyed. so auditns has to get reference of netns.

Namespace == mind blown.  Ok, so:

 auditd in audit_ns2 and net_ns2. <--- ONLY process in net_ns2
 some process in audit_ns2 and net_ns3

Lets assume that auditd is killed improperly/dies.  Because the last
process in net_ns2 is gone net_ns2 is invalid/freed.

Today in the kernel the way we detect auditd is gone is by using the
socket and getting ECONNREFUSSED.  So here you think that audit_ns2
should hold a reference on net_ns2, to make sure that socket is always
valid.

I instead propose that we could run all audit_ns and reset the audit_pid
in that namespace and the audit_sock in the namespace to 0/null inside
audit_net_exit.  Since obviously if the net_ns disappeared, the auditd
which was running in any audit namespace in that net_ns isn't running
any more.  We didn't need to hold a reference on the net_ns.  We just
have to clear the skb_queue, reset the audit_pid to 0, and reset the
socket to NULL...

Maybe the one magic socket is the right answer.  I'm not arguing against
your solution.  I'm really trying to understand why we are going that
way...

> and in some case, netns will get reference of auditns too. this
> is complex than making audit_sock global and getting rid of this
> reference.

This I haven't even started to try to wrap my head around...

Re: [PATCH] audit: listen in all network namespaces

[Gao feng]

On 12/20/2013 11:11 AM, Eric Paris wrote:
> On Fri, 2013-12-20 at 10:46 +0800, Gao feng wrote:
>> On 12/20/2013 02:40 AM, Eric Paris wrote:
>>> On Thu, 2013-12-19 at 11:59 +0800, Gao feng wrote:
>>>> On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
> 
>>>> we have to store audit_sock
>>>> into auditns(auditns will be passed to kauditd_send_skb),
>>>> this will cause auditns have to get a reference of netns.
>>>> and for some reason(netfilter audit target), netns will
>>>> get reference of auditns too. this is terrible...
>>>
>>> I'm not sure I agree/understand this entirely...
>>>
>>
>> Yes, the audit_sock is created and destroyed by net namespace,
>> so if auditns wants to use audit_sock, it must prevent netns
>> from being destroyed. so auditns has to get reference of netns.
> 
> Namespace == mind blown.  Ok, so:
> 
>  auditd in audit_ns2 and net_ns2. <--- ONLY process in net_ns2
>  some process in audit_ns2 and net_ns3
> 
> Lets assume that auditd is killed improperly/dies.  Because the last
> process in net_ns2 is gone net_ns2 is invalid/freed.
> 
> Today in the kernel the way we detect auditd is gone is by using the
> socket and getting ECONNREFUSSED.  So here you think that audit_ns2
> should hold a reference on net_ns2, to make sure that socket is always
> valid.
> 
> I instead propose that we could run all audit_ns and reset the audit_pid
> in that namespace and the audit_sock in the namespace to 0/null inside
> audit_net_exit.  Since obviously if the net_ns disappeared, the auditd
> which was running in any audit namespace in that net_ns isn't running
> any more.  We didn't need to hold a reference on the net_ns.  We just
> have to clear the skb_queue, reset the audit_pid to 0, and reset the
> socket to NULL...

multi auditns can share the same netns. it happens if you unshare
auditns. if you want to reset audit_sock to null inside audit_net_exit,
you have to maintain a list in netns, this list contains the auditnss
whose audit_sock is created in this netns. so you can foreach this
list and reset the audit socks of audit nss.

Above is unsharing auditns, consider unsharing netns. auditd is running
in auditns1 and netns1, and then who-know-why the auditd call unshare(CLONE_NEWNET)
to change it's netns from netns1 to new netns2. so the netns1 is released
and auditns->audit_sock being reset to NULL. the auditd cannot receive
the audit log. auditd will in chaos, "I'm still alive, why kernel think
I'm die?"

So maybe you will say, we can reset the audit_sock of netns2 to auditns.
ok, this is a way. but how can we decide if we should reset the auditns->audit_sock?
when we create the new netns, the old netns is still alive, so the auditns->audit_sock
is still valid in that time.

I don't know if there are some other problems we should consider.
it is too complex..

> 
> Maybe the one magic socket is the right answer.  I'm not arguing against
> your solution.  I'm really trying to understand why we are going that
> way...
> 

That's why we should discuss :)