linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: simo@redhat.com, carlos@redhat.com, netdev@vger.kernel.org,
	containers@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, dhowells@redhat.com,
	linux-audit@redhat.com, netfilter-devel@vger.kernel.org,
	ebiederm@xmission.com, luto@kernel.org,
	Eric Paris <eparis@parisplace.org>,
	Serge Hallyn <serge@hallyn.com>,
	viro@zeniv.linux.org.uk
Subject: Re: [PATCH ghak90 (was ghak32) V4 09/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
Date: Thu, 27 Dec 2018 17:54:09 -0500	[thread overview]
Message-ID: <CAHC9VhSw1Aqi7BTh1CEfsWOYPQ-t8uiT6ic91bggxrwhYOi3_A@mail.gmail.com> (raw)
In-Reply-To: <20181227153354.k2vd4be2c6t2kqe4@madcap2.tricolour.ca>

On Thu, Dec 27, 2018 at 10:34 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-31 15:30, Richard Guy Briggs wrote:
> > On 2018-10-19 19:18, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> > > > event standalone records.  Iterate through all potential audit container
> > > > identifiers associated with a network namespace.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  include/linux/audit.h    |  5 +++++
> > > >  kernel/audit.c           | 26 ++++++++++++++++++++++++++
> > > >  net/netfilter/xt_AUDIT.c | 12 ++++++++++--
> > > >  3 files changed, 41 insertions(+), 2 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 9a02095..8755f4d 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -169,6 +169,8 @@ extern int audit_log_contid(struct audit_context *context,
> > > >  extern void audit_netns_contid_add(struct net *net, u64 contid);
> > > >  extern void audit_netns_contid_del(struct net *net, u64 contid);
> > > >  extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
> > > > +extern void audit_log_netns_contid_list(struct net *net,
> > > > +                                struct audit_context *context);
> > > >
> > > >  extern int                 audit_update_lsm_rules(void);
> > > >
> > > > @@ -228,6 +230,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
> > > >  { }
> > > >  static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > > >  { }
> > > > +static inline void audit_log_netns_contid_list(struct net *net,
> > > > +                                       struct audit_context *context)
> > > > +{ }
> > > >
> > > >  #define audit_enabled AUDIT_OFF
> > > >  #endif /* CONFIG_AUDIT */
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index c5fed3b..b23711c 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -392,6 +392,32 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > > >                 audit_netns_contid_add(new->net_ns, contid);
> > > >  }
> > > >
> > > > +void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> > > > +{
> > > > +       spinlock_t *lock = audit_get_netns_contid_list_lock(net);
> > > > +       struct audit_buffer *ab;
> > > > +       struct audit_contid *cont;
> > > > +       bool first = true;
> > > > +
> > > > +       /* Generate AUDIT_CONTAINER record with container ID CSV list */
> > > > +       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_CONTAINER);
> > > > +       if (!ab) {
> > > > +               audit_log_lost("out of memory in audit_log_netns_contid_list");
> > > > +               return;
> > > > +       }
> > > > +       audit_log_format(ab, "contid=");
> > > > +       spin_lock(lock);
> > > > +       list_for_each_entry(cont, audit_get_netns_contid_list(net), list) {
> > > > +               if (!first)
> > > > +                       audit_log_format(ab, ",");
> > > > +               audit_log_format(ab, "%llu", cont->id);
> > > > +               first = false;
> > > > +       }
> > > > +       spin_unlock(lock);
> > >
> > > This is looking like potentially a lot of work to be doing under a
> > > spinlock, not to mention a single spinlock that is shared across CPUs.
> > > Considering that I expect changes to the list to be somewhat
> > > infrequent, this might be a good candidate for a RCU based locking
> > > scheme.
> >
> > Would something like this look reasonable?
> > (This is on top of a patch to make contid list lock and unlock
> > functions.)
>
> Paul, could I please get your review on this locking approach I proposed
> almost two months ago so I can be more reassured that it won't be an
> issue in v5?  Thanks!

I see that not much was learned from our last exchange.  This is disappointing.

At this point you've exhausted my goodwill, and the "This is on top of
a patch to make contid list lock and unlock
functions" comment isn't clear to me at this moment so I'm going to
suggest you just post it as part of your next patchset revision so it
can be seen in the proper context.  Based on a quick inspection it
doesn't seems like there is any mutual exclusion for the writers, but
perhaps this is part of the of "... on top of a patch ..." hand
waving; another reason to see the patch in proper context.

If you've read and understand everything under Documentation/RCU (the
checklist.txt can be helpful), it should be fine.  If you haven't, you
should do so before posting the next iteration.

> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index be5d6eb..9428fc3 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -92,6 +92,7 @@ struct audit_contid {
> >       struct list_head        list;
> >       u64                     id;
> >       refcount_t              refcount;
> > +     struct rcu_head         rcu;
> >  };
> >
> >  extern int is_audit_feature_set(int which);
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index d5b58163..6f84c25 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -106,7 +106,6 @@
> >  struct audit_net {
> >       struct sock *sk;
> >       struct list_head contid_list;
> > -     spinlock_t contid_list_lock;
> >  };
> >
> >  /**
> > @@ -327,26 +326,6 @@ struct list_head *audit_get_netns_contid_list(const struct net *net)
> >       return &aunet->contid_list;
> >  }
> >
> > -static int audit_netns_contid_lock(const struct net *net)
> > -{
> > -     struct audit_net *aunet = net_generic(net, audit_net_id);
> > -
> > -     if (!aunet)
> > -             return -EINVAL;
> > -     spin_lock(aunet->contid_list_lock);
> > -     return 0;
> > -}
> > -
> > -static int audit_netns_contid_unlock(const struct net *net)
> > -{
> > -     struct audit_net *aunet = net_generic(net, audit_net_id);
> > -
> > -     if (!aunet)
> > -             return -EINVAL;
> > -     spin_unlock(aunet->contid_list_lock);
> > -     return 0;
> > -}
> > -
> >  void audit_netns_contid_add(struct net *net, u64 contid)
> >  {
> >       struct list_head *contid_list = audit_get_netns_contid_list(net);
> > @@ -354,10 +333,9 @@ void audit_netns_contid_add(struct net *net, u64 contid)
> >
> >       if (!audit_contid_valid(contid))
> >               return;
> > -     if (audit_netns_contid_lock(net))
> > -             return;
> > +     rcu_read_lock();
> >       if (!list_empty(contid_list))
> > -             list_for_each_entry(cont, contid_list, list)
> > +             list_for_each_entry_rcu(cont, contid_list, list)
> >                       if (cont->id == contid) {
> >                               refcount_inc(&cont->refcount);
> >                               goto out;
> > @@ -367,10 +345,16 @@ void audit_netns_contid_add(struct net *net, u64 contid)
> >               INIT_LIST_HEAD(&cont->list);
> >               cont->id = contid;
> >               refcount_set(&cont->refcount, 1);
> > -             list_add(&cont->list, contid_list);
> > +             list_add_rcu(&cont->list, contid_list);
> >       }
> >  out:
> > -     audit_netns_contid_unlock(net);
> > +     rcu_read_unlock();
> > +}
> > +
> > +audit_free_contid_rcu(struct rcu_head *head) {
> > +     struct audit_contid *contid = container_of(head, struct audit_contid, rcu);
> > +
> > +     kfree(contid);
> >  }
> >
> >  void audit_netns_contid_del(struct net *net, u64 contid)
> > @@ -380,17 +364,16 @@ void audit_netns_contid_del(struct net *net, u64 contid)
> >
> >       if (!audit_contid_valid(contid))
> >               return;
> > -     if (audit_netns_contid_lock(net))
> > -             return;
> > +     rcu_read_lock();
> >       if (!list_empty(contid_list))
> > -             list_for_each_entry(cont, contid_list, list)
> > +             list_for_each_entry_rcu(cont, contid_list, list)
> >                       if (cont->id == contid) {
> > -                             list_del(&cont->list);
> > +                             list_del_rcu(&cont->list);
> >                               if (refcount_dec_and_test(&cont->refcount))
> > -                                     kfree(cont);
> > +                                     call_rcu(&cont->rcu, audit_free_contid_rcu);
> >                               break;
> >                       }
> > -     audit_netns_contid_unlock(net);
> > +     rcu_read_unlock();
> >  }
> >
> >  void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > @@ -418,15 +401,14 @@ void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> >               return;
> >       }
> >       audit_log_format(ab, "ref=net contid=");
> > -     if (audit_netns_contid_lock(net))
> > -             return;
> > -     list_for_each_entry(cont, audit_get_netns_contid_list(net), list) {
> > +     rcu_read_lock();
> > +     list_for_each_entry_rcu(cont, audit_get_netns_contid_list(net), list) {
> >               if (!first)
> >                       audit_log_format(ab, ",");
> >               audit_log_format(ab, "%llu", cont->id);
> >               first = false;
> >       }
> > -     audit_netns_contid_unlock(net);
> > +     rcu_read_unlock();
> >       audit_log_end(ab);
> >  }
> >  EXPORT_SYMBOL(audit_log_netns_contid_list);
> > @@ -1674,7 +1656,6 @@ static int __net_init audit_net_init(struct net *net)
> >               .flags  = NL_CFG_F_NONROOT_RECV,
> >               .groups = AUDIT_NLGRP_MAX,
> >       };
> > -
> >       struct audit_net *aunet = net_generic(net, audit_net_id);
> >
> >       aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> > @@ -1684,8 +1665,6 @@ static int __net_init audit_net_init(struct net *net)
> >       }
> >       aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> >       INIT_LIST_HEAD(&aunet->contid_list);
> > -     spin_lock_init(&aunet->contid_list_lock);
> > -
> >       return 0;
> >  }
> >
> > >
> > > > +       audit_log_end(ab);
> > > > +}
> > > > +EXPORT_SYMBOL(audit_log_netns_contid_list);
> > > >
> > > >  void audit_panic(const char *message)
> > > >  {
> > > >         switch (audit_failure) {
> > > > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> > > > index af883f1..44fac3f 100644
> > > > --- a/net/netfilter/xt_AUDIT.c
> > > > +++ b/net/netfilter/xt_AUDIT.c
> > > > @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> > > >  {
> > > >         struct audit_buffer *ab;
> > > >         int fam = -1;
> > > > +       struct audit_context *context;
> > > > +       struct net *net;
> > > >
> > > >         if (audit_enabled == AUDIT_OFF)
> > > > -               goto errout;
> > > > -       ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > > > +               goto out;
> > > > +       context = audit_alloc_local(GFP_ATOMIC);
> > > > +       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > > >         if (ab == NULL)
> > > >                 goto errout;
> > > >
> > > > @@ -104,7 +107,12 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> > > >
> > > >         audit_log_end(ab);
> > > >
> > > > +       net = xt_net(par);
> > > > +       audit_log_netns_contid_list(net, context);
> > > > +
> > > >  errout:
> > > > +       audit_free_context(context);
> > > > +out:
> > > >         return XT_CONTINUE;
> > > >  }
> > > >
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
paul moore
www.paul-moore.com

  reply	other threads:[~2018-12-27 22:54 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-31 20:07 [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier Richard Guy Briggs
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters Richard Guy Briggs
2018-10-19 23:15   ` Paul Moore
2018-11-01 22:07     ` Richard Guy Briggs
2019-01-03 20:10       ` Paul Moore
2019-01-03 20:29         ` Richard Guy Briggs
2019-01-03 20:33           ` Paul Moore
2019-01-03 20:38             ` Richard Guy Briggs
2019-01-24 20:36         ` Richard Guy Briggs
2019-01-04  2:50   ` Guenter Roeck
2019-01-04 14:57     ` Richard Guy Briggs
2019-01-04 22:04       ` Guenter Roeck
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 02/10] audit: add container id Richard Guy Briggs
2018-08-24 16:01   ` Steve Grubb
2018-10-19 19:38   ` Paul Moore
2018-10-19 19:40     ` Paul Moore
2018-10-19 21:50     ` Richard Guy Briggs
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls Richard Guy Briggs
2018-08-24 16:01   ` Steve Grubb
2018-10-19 23:16   ` Paul Moore
2018-10-24 15:14     ` Richard Guy Briggs
2018-10-24 20:55       ` Paul Moore
2018-10-25  0:42         ` Richard Guy Briggs
2018-10-25  6:06           ` Steve Grubb
2018-10-25 10:49             ` Paul Moore
2018-10-25 12:27               ` Richard Guy Briggs
2018-10-25 15:57                 ` Steve Grubb
2018-10-25 17:38                   ` Richard Guy Briggs
2018-10-25 20:40                     ` Paul Moore
2018-10-25 21:55                       ` Steve Grubb
2018-10-26  8:09                         ` Casey Schaufler
2018-10-28  7:53                           ` Paul Moore
2018-10-25  6:13           ` Paul Moore
2018-10-25 12:22             ` Richard Guy Briggs
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 04/10] audit: add containerid support for ptrace and signals Richard Guy Briggs
2018-10-19 23:16   ` Paul Moore
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 05/10] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2018-10-19 23:17   ` Paul Moore
2018-11-01 18:48     ` Richard Guy Briggs
2019-01-03 20:10       ` Paul Moore
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 06/10] audit: add containerid support for tty_audit Richard Guy Briggs
2018-10-19 23:17   ` Paul Moore
2018-10-31 21:17     ` Richard Guy Briggs
2019-01-03 20:11       ` Paul Moore
2019-01-10 22:58         ` Richard Guy Briggs
2019-01-11  1:12           ` Paul Moore
2019-01-11  3:38             ` Richard Guy Briggs
2019-01-11 23:16               ` Paul Moore
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 07/10] audit: add containerid filtering Richard Guy Briggs
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 08/10] audit: add support for containerid to network namespaces Richard Guy Briggs
2018-10-19 23:18   ` Paul Moore
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 09/10] audit: NETFILTER_PKT: record each container ID associated with a netNS Richard Guy Briggs
2018-10-19 23:18   ` Paul Moore
2018-10-31 19:30     ` Richard Guy Briggs
2018-12-27 15:33       ` Richard Guy Briggs
2018-12-27 22:54         ` Paul Moore [this message]
2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 10/10] debug audit: read container ID of a process Richard Guy Briggs
2019-01-03 16:15 ` [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier Guenter Roeck
2019-01-03 17:36   ` Richard Guy Briggs
2019-01-03 18:58     ` Guenter Roeck
2019-01-03 20:20       ` Richard Guy Briggs
2019-01-03 20:12     ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHC9VhSw1Aqi7BTh1CEfsWOYPQ-t8uiT6ic91bggxrwhYOi3_A@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=carlos@redhat.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=eparis@parisplace.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=rgb@redhat.com \
    --cc=serge@hallyn.com \
    --cc=simo@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).