Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <5FBCBE569E134E4CA167B91C0A77FD610198F8FA41@EXMBX-SZMAIL022.tencent.com>
In-Reply-To: <5FBCBE569E134E4CA167B91C0A77FD610198F8FA41@EXMBX-SZMAIL022.tencent.com>
From:   Todd Kjos <tkjos@google.com>
Date:   Mon, 12 Nov 2018 08:45:07 -0800
Message-ID: <CAHRSSEwf67AkOtwsAEuqmmwSf2OV8BbqYFiGft9qXcrDQvhzHA@mail.gmail.com>
Subject: Re: [PATCH V4] binder: ipc namespace support for android binder
To:     chouryzhou@tencent.com, christian@brauner.io,
        Martijn Coenen <maco@android.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>, akpm@linux-foundation.org,
        dave@stgolabs.net,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+christian@brauner.io +Martijn Coenen

Christian,

Does this patch work for your container use-cases? If not, please
comment on this thread. Let's discuss at LPC this week.

-Todd

On Mon, Nov 12, 2018 at 1:38 AM chouryzhou(=E5=91=A8=E5=A8=81) <chouryzhou@=
tencent.com> wrote:
>
> Currently android's binder is not isolated by ipc namespace. Since binder
> is a form of IPC and therefore should be tied to ipc namespace. With this
> patch, we can run multiple instances of  android container on one host.
>
> This patch move "binder_procs" and "binder_context" into ipc_namespace,
> driver will find the context from it when opening. For debugfs, binder_pr=
oc
> is namespace-aware, but not for binder dead nodes, binder_stats and
> binder_transaction_log_entry (we added ipc inum to trace it).
>
> Signed-off-by: chouryzhou <chouryzhou@tencent.com>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/android/binder.c      | 133 ++++++++++++++++++++++++++++++++----=
------
>  include/linux/ipc_namespace.h |  15 +++++
>  ipc/namespace.c               |  10 +++-
>  3 files changed, 125 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cb30a524d16d..453265505b04 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -67,6 +67,8 @@
>  #include <linux/sched/mm.h>
>  #include <linux/seq_file.h>
>  #include <linux/uaccess.h>
> +#include <linux/proc_ns.h>
> +#include <linux/ipc_namespace.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/security.h>
>  #include <linux/spinlock.h>
> @@ -80,13 +82,21 @@
>  #include "binder_alloc.h"
>  #include "binder_trace.h"
>
> +
> +#ifndef CONFIG_IPC_NS
> +static struct ipc_namespace binder_ipc_ns =3D {
> +       .ns.inum =3D PROC_IPC_INIT_INO,
> +};
> +
> +#define ipcns  (&binder_ipc_ns)
> +#else
> +#define ipcns  (current->nsproxy->ipc_ns)
> +#endif
> +
>  static HLIST_HEAD(binder_deferred_list);
>  static DEFINE_MUTEX(binder_deferred_lock);
>
>  static HLIST_HEAD(binder_devices);
> -static HLIST_HEAD(binder_procs);
> -static DEFINE_MUTEX(binder_procs_lock);
> -
>  static HLIST_HEAD(binder_dead_nodes);
>  static DEFINE_SPINLOCK(binder_dead_nodes_lock);
>
> @@ -233,6 +243,7 @@ struct binder_transaction_log_entry {
>         uint32_t return_error;
>         uint32_t return_error_param;
>         const char *context_name;
> +       unsigned int ipc_inum;
>  };
>  struct binder_transaction_log {
>         atomic_t cur;
> @@ -263,19 +274,66 @@ static struct binder_transaction_log_entry *binder_=
transaction_log_add(
>  }
>
>  struct binder_context {
> +       struct hlist_node hlist;
>         struct binder_node *binder_context_mgr_node;
>         struct mutex context_mgr_node_lock;
>
>         kuid_t binder_context_mgr_uid;
> +       int   device;
>         const char *name;
>  };
>
>  struct binder_device {
>         struct hlist_node hlist;
>         struct miscdevice miscdev;
> -       struct binder_context context;
>  };
>
> +void binder_exit_ns(struct ipc_namespace *ns)
> +{
> +       struct binder_context *context;
> +       struct hlist_node *tmp;
> +
> +       mutex_destroy(&ns->binder_procs_lock);
> +       hlist_for_each_entry_safe(context, tmp, &ns->binder_contexts, hli=
st) {
> +               mutex_destroy(&context->context_mgr_node_lock);
> +               hlist_del(&context->hlist);
> +               kfree(context);
> +       }
> +}
> +
> +int binder_init_ns(struct ipc_namespace *ns)
> +{
> +       int ret;
> +       struct binder_device *device;
> +
> +       mutex_init(&ns->binder_procs_lock);
> +       INIT_HLIST_HEAD(&ns->binder_procs);
> +       INIT_HLIST_HEAD(&ns->binder_contexts);
> +
> +       hlist_for_each_entry(device, &binder_devices, hlist) {
> +               struct binder_context *context;
> +
> +               context =3D kzalloc(sizeof(*context), GFP_KERNEL);
> +               if (!context) {
> +                       ret =3D -ENOMEM;
> +                       goto err;
> +               }
> +
> +               context->device =3D device->miscdev.minor;
> +               context->name =3D device->miscdev.name;
> +               context->binder_context_mgr_uid =3D INVALID_UID;
> +               mutex_init(&context->context_mgr_node_lock);
> +
> +               hlist_add_head(&context->hlist, &ns->binder_contexts);
> +       }
> +
> +       return 0;
> +err:
> +       binder_exit_ns(ns);
> +       return ret;
> +}
> +
> +
>  /**
>   * struct binder_work - work enqueued on a worklist
>   * @entry:             node enqueued on list
> @@ -2728,6 +2786,7 @@ static void binder_transaction(struct binder_proc *=
proc,
>         e->data_size =3D tr->data_size;
>         e->offsets_size =3D tr->offsets_size;
>         e->context_name =3D proc->context->name;
> +       e->ipc_inum =3D ipcns->ns.inum;
>
>         if (reply) {
>                 binder_inner_proc_lock(proc);
> @@ -4922,6 +4981,7 @@ static int binder_open(struct inode *nodp, struct f=
ile *filp)
>  {
>         struct binder_proc *proc;
>         struct binder_device *binder_dev;
> +       struct binder_context *context;
>
>         binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
>                      current->group_leader->pid, current->pid);
> @@ -4937,7 +4997,15 @@ static int binder_open(struct inode *nodp, struct =
file *filp)
>         proc->default_priority =3D task_nice(current);
>         binder_dev =3D container_of(filp->private_data, struct binder_dev=
ice,
>                                   miscdev);
> -       proc->context =3D &binder_dev->context;
> +       hlist_for_each_entry(context, &ipcns->binder_contexts, hlist) {
> +               if (context->device =3D=3D binder_dev->miscdev.minor) {
> +                       proc->context =3D context;
> +                       break;
> +               }
> +       }
> +       if (!proc->context)
> +               return -ENOENT;
> +
>         binder_alloc_init(&proc->alloc);
>
>         binder_stats_created(BINDER_STAT_PROC);
> @@ -4946,9 +5014,9 @@ static int binder_open(struct inode *nodp, struct f=
ile *filp)
>         INIT_LIST_HEAD(&proc->waiting_threads);
>         filp->private_data =3D proc;
>
> -       mutex_lock(&binder_procs_lock);
> -       hlist_add_head(&proc->proc_node, &binder_procs);
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_lock(&ipcns->binder_procs_lock);
> +       hlist_add_head(&proc->proc_node, &ipcns->binder_procs);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         if (binder_debugfs_dir_entry_proc) {
>                 char strbuf[11];
> @@ -5082,9 +5150,9 @@ static void binder_deferred_release(struct binder_p=
roc *proc)
>         struct rb_node *n;
>         int threads, nodes, incoming_refs, outgoing_refs, active_transact=
ions;
>
> -       mutex_lock(&binder_procs_lock);
> +       mutex_lock(&ipcns->binder_procs_lock);
>         hlist_del(&proc->proc_node);
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         mutex_lock(&context->context_mgr_node_lock);
>         if (context->binder_context_mgr_node &&
> @@ -5623,10 +5691,10 @@ static int binder_state_show(struct seq_file *m, =
void *unused)
>         if (last_node)
>                 binder_put_node(last_node);
>
> -       mutex_lock(&binder_procs_lock);
> -       hlist_for_each_entry(proc, &binder_procs, proc_node)
> +       mutex_lock(&ipcns->binder_procs_lock);
> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>                 print_binder_proc(m, proc, 1);
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         return 0;
>  }
> @@ -5639,10 +5707,10 @@ static int binder_stats_show(struct seq_file *m, =
void *unused)
>
>         print_binder_stats(m, "", &binder_stats);
>
> -       mutex_lock(&binder_procs_lock);
> -       hlist_for_each_entry(proc, &binder_procs, proc_node)
> +       mutex_lock(&ipcns->binder_procs_lock);
> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>                 print_binder_proc_stats(m, proc);
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         return 0;
>  }
> @@ -5652,10 +5720,10 @@ static int binder_transactions_show(struct seq_fi=
le *m, void *unused)
>         struct binder_proc *proc;
>
>         seq_puts(m, "binder transactions:\n");
> -       mutex_lock(&binder_procs_lock);
> -       hlist_for_each_entry(proc, &binder_procs, proc_node)
> +       mutex_lock(&ipcns->binder_procs_lock);
> +       hlist_for_each_entry(proc, &ipcns->binder_procs, proc_node)
>                 print_binder_proc(m, proc, 0);
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         return 0;
>  }
> @@ -5665,14 +5733,14 @@ static int binder_proc_show(struct seq_file *m, v=
oid *unused)
>         struct binder_proc *itr;
>         int pid =3D (unsigned long)m->private;
>
> -       mutex_lock(&binder_procs_lock);
> -       hlist_for_each_entry(itr, &binder_procs, proc_node) {
> +       mutex_lock(&ipcns->binder_procs_lock);
> +       hlist_for_each_entry(itr, &ipcns->binder_procs, proc_node) {
>                 if (itr->pid =3D=3D pid) {
>                         seq_puts(m, "binder proc state:\n");
>                         print_binder_proc(m, itr, 1);
>                 }
>         }
> -       mutex_unlock(&binder_procs_lock);
> +       mutex_unlock(&ipcns->binder_procs_lock);
>
>         return 0;
>  }
> @@ -5687,12 +5755,12 @@ static void print_binder_transaction_log_entry(st=
ruct seq_file *m,
>          */
>         smp_rmb();
>         seq_printf(m,
> -                  "%d: %s from %d:%d to %d:%d context %s node %d handle =
%d size %d:%d ret %d/%d l=3D%d",
> +                  "%d: %s from %d:%d to %d:%d ipc %d context %s node %d =
handle %d size %d:%d ret %d/%d l=3D%d",
>                    e->debug_id, (e->call_type =3D=3D 2) ? "reply" :
>                    ((e->call_type =3D=3D 1) ? "async" : "call "), e->from=
_proc,
> -                  e->from_thread, e->to_proc, e->to_thread, e->context_n=
ame,
> -                  e->to_node, e->target_handle, e->data_size, e->offsets=
_size,
> -                  e->return_error, e->return_error_param,
> +                  e->from_thread, e->to_proc, e->to_thread, e->ipc_inum,
> +                  e->context_name, e->to_node, e->target_handle, e->data=
_size,
> +                  e->offsets_size, e->return_error, e->return_error_para=
m,
>                    e->return_error_line);
>         /*
>          * read-barrier to guarantee read of debug_id_done after
> @@ -5753,10 +5821,6 @@ static int __init init_binder_device(const char *n=
ame)
>         binder_device->miscdev.minor =3D MISC_DYNAMIC_MINOR;
>         binder_device->miscdev.name =3D name;
>
> -       binder_device->context.binder_context_mgr_uid =3D INVALID_UID;
> -       binder_device->context.name =3D name;
> -       mutex_init(&binder_device->context.context_mgr_node_lock);
> -
>         ret =3D misc_register(&binder_device->miscdev);
>         if (ret < 0) {
>                 kfree(binder_device);
> @@ -5831,9 +5895,16 @@ static int __init binder_init(void)
>                 if (ret)
>                         goto err_init_binder_device_failed;
>         }
> +#ifdef CONFIG_IPC_NS
> +       ret =3D binder_init_ns(&init_ipc_ns);
> +#else
> +       ret =3D binder_init_ns(&binder_ipc_ns);
> +#endif
> +       if (ret)
> +               goto err_init_namespace_failed;
>
>         return ret;
> -
> +err_init_namespace_failed:
>  err_init_binder_device_failed:
>         hlist_for_each_entry_safe(device, tmp, &binder_devices, hlist) {
>                 misc_deregister(&device->miscdev);
> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.=
h
> index 6ab8c1bada3f..d7f850a2ded8 100644
> --- a/include/linux/ipc_namespace.h
> +++ b/include/linux/ipc_namespace.h
> @@ -63,6 +63,13 @@ struct ipc_namespace {
>         unsigned int    mq_msg_default;
>         unsigned int    mq_msgsize_default;
>
> +#ifdef CONFIG_ANDROID_BINDER_IPC
> +       /* next fields are for binder */
> +       struct mutex      binder_procs_lock;
> +       struct hlist_head binder_procs;
> +       struct hlist_head binder_contexts;
> +#endif
> +
>         /* user_ns which owns the ipc ns */
>         struct user_namespace *user_ns;
>         struct ucounts *ucounts;
> @@ -118,6 +125,14 @@ extern int mq_init_ns(struct ipc_namespace *ns);
>  static inline int mq_init_ns(struct ipc_namespace *ns) { return 0; }
>  #endif
>
> +#ifdef CONFIG_ANDROID_BINDER_IPC
> +extern int binder_init_ns(struct ipc_namespace *ns);
> +extern void binder_exit_ns(struct ipc_namespace *ns);
> +#else
> +static inline int binder_init_ns(struct ipc_namespace *ns) { return 0; }
> +static inline void binder_exit_ns(struct ipc_namespace *ns) { }
> +#endif
> +
>  #if defined(CONFIG_IPC_NS)
>  extern struct ipc_namespace *copy_ipcs(unsigned long flags,
>         struct user_namespace *user_ns, struct ipc_namespace *ns);
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index 21607791d62c..68c6e983b002 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -57,7 +57,10 @@ static struct ipc_namespace *create_ipc_ns(struct user=
_namespace *user_ns,
>
>         err =3D mq_init_ns(ns);
>         if (err)
> -               goto fail_put;
> +               goto fail_init_mq;
> +       err =3D binder_init_ns(ns);
> +       if (err)
> +               goto fail_init_binder;
>
>         sem_init_ns(ns);
>         msg_init_ns(ns);
> @@ -65,7 +68,9 @@ static struct ipc_namespace *create_ipc_ns(struct user_=
namespace *user_ns,
>
>         return ns;
>
> -fail_put:
> +fail_init_binder:
> +       mq_put_mnt(ns);
> +fail_init_mq:
>         put_user_ns(ns->user_ns);
>         ns_free_inum(&ns->ns);
>  fail_free:
> @@ -120,6 +125,7 @@ static void free_ipc_ns(struct ipc_namespace *ns)
>         sem_exit_ns(ns);
>         msg_exit_ns(ns);
>         shm_exit_ns(ns);
> +       binder_exit_ns(ns);
>
>         dec_ipc_namespaces(ns->ucounts);
>         put_user_ns(ns->user_ns);
> --
> 2.11.0