Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 9:43 PM chouryzhou(周威) wrote: > > > > > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it > > > will be a static > > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > > me) with > > > no namespace-ization. You will get the same one in all processes, > > > everything is > > > the same as without this patch. > > > > except, as far as I can tell, binder_init_ns() would never have been > > called on it so the mutex and list heads are not initialized so its > > completely broken. Am I missing something? How do those fields get > > initialized in this case? > > > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > > goto err_init_binder_device_failed; > > } > > > > - return ret; > > + ret = binder_init_ns(_ipc_ns); > > + if (ret) > > + goto err_init_namespace_failed; > > > > + return ret; > > They are initialized here. Ok, This init_ipc_ns is a global declared in msgutil.c if SYSVIPC || POSIX_MQUEUE. This seems kinda hacky, but now I finally see why the dependancy... msgutil.c is the only file we can count on if !IPC_NS && (SYSVIPC || POSIX_MQUEUE). There must be a cleaner way to do this, I really don't like this dependency... wouldn't it be cleaner to do: #ifndef CONFIG_IPC_NS static struct ipc_namespace binder_ipc_ns; #define ipcns (_ipc_ns) #else #define ipcns (current->nsproxy->ipc_ns) #endif (and make the initialization of binder_ipc_ns conditional on IPC_NS) This gets us the same thing without the incestuous dependency on the msgutil.c version of init_ipc_ns...and then binder doesn't rely on SYSVIPC or POSIX_MQUEUE directly. > > - choury - >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 9:43 PM chouryzhou(周威) wrote: > > > > > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it > > > will be a static > > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > > me) with > > > no namespace-ization. You will get the same one in all processes, > > > everything is > > > the same as without this patch. > > > > except, as far as I can tell, binder_init_ns() would never have been > > called on it so the mutex and list heads are not initialized so its > > completely broken. Am I missing something? How do those fields get > > initialized in this case? > > > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > > goto err_init_binder_device_failed; > > } > > > > - return ret; > > + ret = binder_init_ns(_ipc_ns); > > + if (ret) > > + goto err_init_namespace_failed; > > > > + return ret; > > They are initialized here. Ok, This init_ipc_ns is a global declared in msgutil.c if SYSVIPC || POSIX_MQUEUE. This seems kinda hacky, but now I finally see why the dependancy... msgutil.c is the only file we can count on if !IPC_NS && (SYSVIPC || POSIX_MQUEUE). There must be a cleaner way to do this, I really don't like this dependency... wouldn't it be cleaner to do: #ifndef CONFIG_IPC_NS static struct ipc_namespace binder_ipc_ns; #define ipcns (_ipc_ns) #else #define ipcns (current->nsproxy->ipc_ns) #endif (and make the initialization of binder_ipc_ns conditional on IPC_NS) This gets us the same thing without the incestuous dependency on the msgutil.c version of init_ipc_ns...and then binder doesn't rely on SYSVIPC or POSIX_MQUEUE directly. > > - choury - >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > > > It seems like this mechanism would work even if both are disabled -- > > > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > > > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > > > "#ifndef CONFIG_IPC_NS" > > > > Let me explain it in detail. If SYSIPC and IPC_NS are both defined, > > current->nsproxy->ipc_ns will save the ipc namespace variables. We just use > > it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, > > current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, > > which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set > > (IPC_NS can't be set in this situation), there is no > > current->nsproxy->ipc_ns. > > We make a fack init_ipc_ns here and use it. > > Yes, I can read the code. I'm wondering specifically about SYSVIPC and > POSIX_MQUEUE. Even with your code changes, binder has no dependency on > these configs. Why are you creating one? The actual dependency with > your changes is on "current->nsproxy->ipc_ns" being initialized for > binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it? > > If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this > work? If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will be a static reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) with no namespace-ization. You will get the same one in all processes, everything is the same as without this patch. - choury -
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > > > It seems like this mechanism would work even if both are disabled -- > > > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > > > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > > > "#ifndef CONFIG_IPC_NS" > > > > Let me explain it in detail. If SYSIPC and IPC_NS are both defined, > > current->nsproxy->ipc_ns will save the ipc namespace variables. We just use > > it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, > > current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, > > which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set > > (IPC_NS can't be set in this situation), there is no > > current->nsproxy->ipc_ns. > > We make a fack init_ipc_ns here and use it. > > Yes, I can read the code. I'm wondering specifically about SYSVIPC and > POSIX_MQUEUE. Even with your code changes, binder has no dependency on > these configs. Why are you creating one? The actual dependency with > your changes is on "current->nsproxy->ipc_ns" being initialized for > binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it? > > If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this > work? If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will be a static reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) with no namespace-ization. You will get the same one in all processes, everything is the same as without this patch. - choury -
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > > be a static > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > me) with > > no namespace-ization. You will get the same one in all processes, > > everything is > > the same as without this patch. > > except, as far as I can tell, binder_init_ns() would never have been > called on it so the mutex and list heads are not initialized so its > completely broken. Am I missing something? How do those fields get > initialized in this case? > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > goto err_init_binder_device_failed; > } > > - return ret; > + ret = binder_init_ns(_ipc_ns); > + if (ret) > + goto err_init_namespace_failed; > > + return ret; They are initialized here. - choury -
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
> > > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > > be a static > > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by > > me) with > > no namespace-ization. You will get the same one in all processes, > > everything is > > the same as without this patch. > > except, as far as I can tell, binder_init_ns() would never have been > called on it so the mutex and list heads are not initialized so its > completely broken. Am I missing something? How do those fields get > initialized in this case? > @@ -5832,8 +5888,12 @@ static int __init binder_init(void) > goto err_init_binder_device_failed; > } > > - return ret; > + ret = binder_init_ns(_ipc_ns); > + if (ret) > + goto err_init_namespace_failed; > > + return ret; They are initialized here. - choury -
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 7:09 PM chouryzhou(周威) wrote: > > > > > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > > It seems like this mechanism would work even if both are disabled -- > > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > > "#ifndef CONFIG_IPC_NS" > > Let me explain it in detail. If SYSIPC and IPC_NS are both defined, > current->nsproxy->ipc_ns will save the ipc namespace variables. We just use > it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, > current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, > which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set > (IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns. > We make a fack init_ipc_ns here and use it. Yes, I can read the code. I'm wondering specifically about SYSVIPC and POSIX_MQUEUE. Even with your code changes, binder has no dependency on these configs. Why are you creating one? The actual dependency with your changes is on "current->nsproxy->ipc_ns" being initialized for binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it? If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this work? > > > why eliminate name? The string name is very useful for differentiating > > normal "framework" binder transactions vs "hal" or "vendor" > > transactions. If we just have a device number it will be hard to tell > > in the logs even which namespace it belongs to. We need to keep both > > the "name" (for which there might be multiple in each ns) and some > > indication of which namespace this is. Maybe we assign some sort of > > namespace ID during binder_init_ns(). > > I will remain the name of device. The inum of ipc_ns can be treated as > namespace ID in ipc_ns. > > > As mentioned above, we need to retain name and probably also want a ns > > id of some sort. So context now has 3 components if IPC_NS, so maybe a > > helper function to print context like: > > > > static void binder_seq_print_context(struct seq_file *m, struct > > binder_context *context) > > { > > #ifdef CONFIG_IPC_NS > > seq_printf(m, "%d-%d-%s", context->ns_id, context->device, > > context->name); > > #else > > seq_printf(m, "%d", context->name); > > #endif > > } > > > > (same comment below everywhere context is printed) > > > > Should these debugfs nodes should be ns aware and only print debugging > > info for the context of the thread accessing the node? If so, we would > > also want to be namespace-aware when printing pids. > > Nowadays, debugfs is not namespace-ized, and pid namespace is not associated > with ipc namespace. Will it be more complicated to debug this if we just > print > the info for current thread? Because we will have to enter the ipc namespace > firstly. But add ipc inum should be no problem. With the name and ns id, debugging would be fine from the host-side. I don't understand the container use cases enough to know if you need to be able to debug binder transaction failures from within the container -- in which case it seems like you'd want the container-version of the PIDs -- but obviously this depends on how the containers are set up and what the use-cases really are. I'm ok with leaving that for a later patch. > > - choury - > >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 7:09 PM chouryzhou(周威) wrote: > > > > > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > > It seems like this mechanism would work even if both are disabled -- > > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > > "#ifndef CONFIG_IPC_NS" > > Let me explain it in detail. If SYSIPC and IPC_NS are both defined, > current->nsproxy->ipc_ns will save the ipc namespace variables. We just use > it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, > current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, > which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set > (IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns. > We make a fack init_ipc_ns here and use it. Yes, I can read the code. I'm wondering specifically about SYSVIPC and POSIX_MQUEUE. Even with your code changes, binder has no dependency on these configs. Why are you creating one? The actual dependency with your changes is on "current->nsproxy->ipc_ns" being initialized for binder -- which is dependent on CONFIG_IPC_NS being enabled, isn't it? If SYSVIPC or POSIX_MQUEUE are enabled, but IPC_NS is disabled, does this work? > > > why eliminate name? The string name is very useful for differentiating > > normal "framework" binder transactions vs "hal" or "vendor" > > transactions. If we just have a device number it will be hard to tell > > in the logs even which namespace it belongs to. We need to keep both > > the "name" (for which there might be multiple in each ns) and some > > indication of which namespace this is. Maybe we assign some sort of > > namespace ID during binder_init_ns(). > > I will remain the name of device. The inum of ipc_ns can be treated as > namespace ID in ipc_ns. > > > As mentioned above, we need to retain name and probably also want a ns > > id of some sort. So context now has 3 components if IPC_NS, so maybe a > > helper function to print context like: > > > > static void binder_seq_print_context(struct seq_file *m, struct > > binder_context *context) > > { > > #ifdef CONFIG_IPC_NS > > seq_printf(m, "%d-%d-%s", context->ns_id, context->device, > > context->name); > > #else > > seq_printf(m, "%d", context->name); > > #endif > > } > > > > (same comment below everywhere context is printed) > > > > Should these debugfs nodes should be ns aware and only print debugging > > info for the context of the thread accessing the node? If so, we would > > also want to be namespace-aware when printing pids. > > Nowadays, debugfs is not namespace-ized, and pid namespace is not associated > with ipc namespace. Will it be more complicated to debug this if we just > print > the info for current thread? Because we will have to enter the ipc namespace > firstly. But add ipc inum should be no problem. With the name and ns id, debugging would be fine from the host-side. I don't understand the container use cases enough to know if you need to be able to debug binder transaction failures from within the container -- in which case it seems like you'd want the container-version of the PIDs -- but obviously this depends on how the containers are set up and what the use-cases really are. I'm ok with leaving that for a later patch. > > - choury - > >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 8:43 PM chouryzhou(周威) wrote: > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > be a static > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) > with > no namespace-ization. You will get the same one in all processes, everything > is > the same as without this patch. except, as far as I can tell, binder_init_ns() would never have been called on it so the mutex and list heads are not initialized so its completely broken. Am I missing something? How do those fields get initialized in this case? > > - choury - >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder(Internet mail)
On Fri, Nov 9, 2018 at 8:43 PM chouryzhou(周威) wrote: > > If IPC_NS is disabled, "current-nsporxy->ipc_ns" will also exists, it will > be a static > reference of "init_ipc_ns" (in ipc/msgutil.c, not defined in binder.c by me) > with > no namespace-ization. You will get the same one in all processes, everything > is > the same as without this patch. except, as far as I can tell, binder_init_ns() would never have been called on it so the mutex and list heads are not initialized so its completely broken. Am I missing something? How do those fields get initialized in this case? > > - choury - >
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
> > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > It seems like this mechanism would work even if both are disabled -- > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > "#ifndef CONFIG_IPC_NS" Let me explain it in detail. If SYSIPC and IPC_NS are both defined, current->nsproxy->ipc_ns will save the ipc namespace variables. We just use it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set (IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns. We make a fack init_ipc_ns here and use it. > why eliminate name? The string name is very useful for differentiating > normal "framework" binder transactions vs "hal" or "vendor" > transactions. If we just have a device number it will be hard to tell > in the logs even which namespace it belongs to. We need to keep both > the "name" (for which there might be multiple in each ns) and some > indication of which namespace this is. Maybe we assign some sort of > namespace ID during binder_init_ns(). I will remain the name of device. The inum of ipc_ns can be treated as namespace ID in ipc_ns. > As mentioned above, we need to retain name and probably also want a ns > id of some sort. So context now has 3 components if IPC_NS, so maybe a > helper function to print context like: > > static void binder_seq_print_context(struct seq_file *m, struct > binder_context *context) > { > #ifdef CONFIG_IPC_NS > seq_printf(m, "%d-%d-%s", context->ns_id, context->device, > context->name); > #else > seq_printf(m, "%d", context->name); > #endif > } > > (same comment below everywhere context is printed) > > Should these debugfs nodes should be ns aware and only print debugging > info for the context of the thread accessing the node? If so, we would > also want to be namespace-aware when printing pids. Nowadays, debugfs is not namespace-ized, and pid namespace is not associated with ipc namespace. Will it be more complicated to debug this if we just print the info for current thread? Because we will have to enter the ipc namespace firstly. But add ipc inum should be no problem. - choury -
Re: Re: [PATCH V3] binder: ipc namespace support for android binder
> > I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. > It seems like this mechanism would work even if both are disabled -- > as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and > allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to > "#ifndef CONFIG_IPC_NS" Let me explain it in detail. If SYSIPC and IPC_NS are both defined, current->nsproxy->ipc_ns will save the ipc namespace variables. We just use it. If SYSIPC (or POSIX_MQUEUE) is defined while IPC_NS is not set, current->nsproxy->ipc_ns will always refer to init_ipc_ns in ipc/msgutil.c, which is also fine to us. But if neither SYSIPC nor POSIX_MQUEUE is set (IPC_NS can't be set in this situation), there is no current->nsproxy->ipc_ns. We make a fack init_ipc_ns here and use it. > why eliminate name? The string name is very useful for differentiating > normal "framework" binder transactions vs "hal" or "vendor" > transactions. If we just have a device number it will be hard to tell > in the logs even which namespace it belongs to. We need to keep both > the "name" (for which there might be multiple in each ns) and some > indication of which namespace this is. Maybe we assign some sort of > namespace ID during binder_init_ns(). I will remain the name of device. The inum of ipc_ns can be treated as namespace ID in ipc_ns. > As mentioned above, we need to retain name and probably also want a ns > id of some sort. So context now has 3 components if IPC_NS, so maybe a > helper function to print context like: > > static void binder_seq_print_context(struct seq_file *m, struct > binder_context *context) > { > #ifdef CONFIG_IPC_NS > seq_printf(m, "%d-%d-%s", context->ns_id, context->device, > context->name); > #else > seq_printf(m, "%d", context->name); > #endif > } > > (same comment below everywhere context is printed) > > Should these debugfs nodes should be ns aware and only print debugging > info for the context of the thread accessing the node? If so, we would > also want to be namespace-aware when printing pids. Nowadays, debugfs is not namespace-ized, and pid namespace is not associated with ipc namespace. Will it be more complicated to debug this if we just print the info for current thread? Because we will have to enter the ipc namespace firstly. But add ipc inum should be no problem. - choury -
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, 09 Nov 2018, Todd Kjos wrote: print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. Ah, very good then. Thanks, Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, 09 Nov 2018, Todd Kjos wrote: print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. Ah, very good then. Thanks, Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 10:27 AM Davidlohr Bueso wrote: > > On Thu, 08 Nov 2018, chouryzhou(??) wrote: > > >+#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 > > Now, I took a look at how the binder_procs list is used; and no, what > follows isn't really related to this patch, but a general observation. > > I think that a mutex is also an overkill and you might wanna replace it > with a spinlock/rwlock. Can anything block while holding the > binder_procs_lock? > I don't see anything... you mainly need it for consulting the hlist calling > print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so > no blocking there. print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. > Also, if this is perhaps because of long hold times, dunno, > the rb_first_cached primitive might reduce some of it, although I don't know > how big the rbtrees in binder can get and if it matters at all. > > Anyway, that said and along with addressing Todd's comments, the ipc/ bits > look > good. Feel free to add my: > > Reviewed-by: Davidlohr Bueso > > Thanks, > Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Fri, Nov 9, 2018 at 10:27 AM Davidlohr Bueso wrote: > > On Thu, 08 Nov 2018, chouryzhou(??) wrote: > > >+#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 > > Now, I took a look at how the binder_procs list is used; and no, what > follows isn't really related to this patch, but a general observation. > > I think that a mutex is also an overkill and you might wanna replace it > with a spinlock/rwlock. Can anything block while holding the > binder_procs_lock? > I don't see anything... you mainly need it for consulting the hlist calling > print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so > no blocking there. print_binder_proc() drops proc->inner_lock and calls binder_alloc_print_allocated() which acquires proc->alloc->mutex. Likewise, print_binder_stats() calls print_binder_proc_stats() which drops its locks to call binder_alloc_print_pages() which also acquires proc->alloc->mutex. So binder_procs_lock needs to be a mutex since it can block on proc->alloc->mutex. > Also, if this is perhaps because of long hold times, dunno, > the rb_first_cached primitive might reduce some of it, although I don't know > how big the rbtrees in binder can get and if it matters at all. > > Anyway, that said and along with addressing Todd's comments, the ipc/ bits > look > good. Feel free to add my: > > Reviewed-by: Davidlohr Bueso > > Thanks, > Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, 08 Nov 2018, chouryzhou(??) wrote: +#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 Now, I took a look at how the binder_procs list is used; and no, what follows isn't really related to this patch, but a general observation. I think that a mutex is also an overkill and you might wanna replace it with a spinlock/rwlock. Can anything block while holding the binder_procs_lock? I don't see anything... you mainly need it for consulting the hlist calling print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so no blocking there. Also, if this is perhaps because of long hold times, dunno, the rb_first_cached primitive might reduce some of it, although I don't know how big the rbtrees in binder can get and if it matters at all. Anyway, that said and along with addressing Todd's comments, the ipc/ bits look good. Feel free to add my: Reviewed-by: Davidlohr Bueso Thanks, Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, 08 Nov 2018, chouryzhou(??) wrote: +#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 Now, I took a look at how the binder_procs list is used; and no, what follows isn't really related to this patch, but a general observation. I think that a mutex is also an overkill and you might wanna replace it with a spinlock/rwlock. Can anything block while holding the binder_procs_lock? I don't see anything... you mainly need it for consulting the hlist calling print_binder_proc[_stat]() - which will take the proc->inner_lock anyway, so no blocking there. Also, if this is perhaps because of long hold times, dunno, the rb_first_cached primitive might reduce some of it, although I don't know how big the rbtrees in binder can get and if it matters at all. Anyway, that said and along with addressing Todd's comments, the ipc/ bits look good. Feel free to add my: Reviewed-by: Davidlohr Bueso Thanks, Davidlohr
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 8, 2018 at 5:02 AM chouryzhou(周威) wrote: > > We are working for running android in container, but we found that 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 more than one 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. Although statistics in > debugfs > remain global. > > Signed-off-by: chouryzhou > --- > drivers/android/binder.c | 128 > +++--- > include/linux/ipc_namespace.h | 15 + > ipc/namespace.c | 10 +++- > 3 files changed, 117 insertions(+), 36 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index cb30a524d16d..22e45bb937e6 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -68,6 +68,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -80,13 +81,18 @@ > #include "binder_alloc.h" > #include "binder_trace.h" > > + > +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE) I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. It seems like this mechanism would work even if both are disabled -- as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to "#ifndef CONFIG_IPC_NS" > +struct ipc_namespace init_ipc_ns; > +#define ipcns (_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); > > @@ -232,7 +238,7 @@ struct binder_transaction_log_entry { > int return_error_line; > uint32_t return_error; > uint32_t return_error_param; > - const char *context_name; > + int context_device; > }; > struct binder_transaction_log { > atomic_t cur; > @@ -263,19 +269,64 @@ 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; > - const char *name; > + intdevice; > }; > > 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(>binder_procs_lock); > + hlist_for_each_entry_safe(context, tmp, >binder_contexts, hlist) { > + mutex_destroy(>context_mgr_node_lock); > + hlist_del(>hlist); > + kfree(context); > + } > +} > + > +int binder_init_ns(struct ipc_namespace *ns) > +{ > + int ret; > + struct binder_device *device; > + > + mutex_init(>binder_procs_lock); > + INIT_HLIST_HEAD(>binder_procs); > + INIT_HLIST_HEAD(>binder_contexts); > + > + hlist_for_each_entry(device, _devices, hlist) { > + struct binder_context *context; > + > + context = kzalloc(sizeof(*context), GFP_KERNEL); > + if (!context) { > + ret = -ENOMEM; > + goto err; > + } > + > + context->device = device->miscdev.minor; > + context->binder_context_mgr_uid = INVALID_UID; > + mutex_init(>context_mgr_node_lock); > + > + hlist_add_head(>hlist, >binder_contexts); > + } > + > + return 0; > +err: > + binder_exit_ns(ns); > + return ret; > +} > + > + > /** > * struct binder_work - work enqueued on a worklist > * @entry: node enqueued on list > @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc, > e->target_handle = tr->target.handle; > e->data_size = tr->data_size; > e->offsets_size = tr->offsets_size; > - e->context_name = proc->context->name; > + e->context_device = proc->context->device; why eliminate name? The string name is very useful for differentiating normal "framework" binder transactions vs "hal" or "vendor" transactions. If we just have a device number it will be hard to tell in the logs even which namespace it belongs to. We need to keep both the "name" (for which there might be multiple in each ns) and some indication of which namespace this is. Maybe we assign some sort of namespace ID during binder_init_ns(). > > if (reply) { >
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 8, 2018 at 5:02 AM chouryzhou(周威) wrote: > > We are working for running android in container, but we found that 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 more than one 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. Although statistics in > debugfs > remain global. > > Signed-off-by: chouryzhou > --- > drivers/android/binder.c | 128 > +++--- > include/linux/ipc_namespace.h | 15 + > ipc/namespace.c | 10 +++- > 3 files changed, 117 insertions(+), 36 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index cb30a524d16d..22e45bb937e6 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -68,6 +68,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -80,13 +81,18 @@ > #include "binder_alloc.h" > #include "binder_trace.h" > > + > +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE) I still don't understand the dependencies on SYSVIPC or POSIX_MQUEUE. It seems like this mechanism would work even if both are disabled -- as long as IPC_NS is enabled. Seems cleaner to change init/Kconfig and allow IPC_NS if CONFIG_ANDROID_BINDER_IPC and change this line to "#ifndef CONFIG_IPC_NS" > +struct ipc_namespace init_ipc_ns; > +#define ipcns (_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); > > @@ -232,7 +238,7 @@ struct binder_transaction_log_entry { > int return_error_line; > uint32_t return_error; > uint32_t return_error_param; > - const char *context_name; > + int context_device; > }; > struct binder_transaction_log { > atomic_t cur; > @@ -263,19 +269,64 @@ 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; > - const char *name; > + intdevice; > }; > > 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(>binder_procs_lock); > + hlist_for_each_entry_safe(context, tmp, >binder_contexts, hlist) { > + mutex_destroy(>context_mgr_node_lock); > + hlist_del(>hlist); > + kfree(context); > + } > +} > + > +int binder_init_ns(struct ipc_namespace *ns) > +{ > + int ret; > + struct binder_device *device; > + > + mutex_init(>binder_procs_lock); > + INIT_HLIST_HEAD(>binder_procs); > + INIT_HLIST_HEAD(>binder_contexts); > + > + hlist_for_each_entry(device, _devices, hlist) { > + struct binder_context *context; > + > + context = kzalloc(sizeof(*context), GFP_KERNEL); > + if (!context) { > + ret = -ENOMEM; > + goto err; > + } > + > + context->device = device->miscdev.minor; > + context->binder_context_mgr_uid = INVALID_UID; > + mutex_init(>context_mgr_node_lock); > + > + hlist_add_head(>hlist, >binder_contexts); > + } > + > + return 0; > +err: > + binder_exit_ns(ns); > + return ret; > +} > + > + > /** > * struct binder_work - work enqueued on a worklist > * @entry: node enqueued on list > @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc, > e->target_handle = tr->target.handle; > e->data_size = tr->data_size; > e->offsets_size = tr->offsets_size; > - e->context_name = proc->context->name; > + e->context_device = proc->context->device; why eliminate name? The string name is very useful for differentiating normal "framework" binder transactions vs "hal" or "vendor" transactions. If we just have a device number it will be hard to tell in the logs even which namespace it belongs to. We need to keep both the "name" (for which there might be multiple in each ns) and some indication of which namespace this is. Maybe we assign some sort of namespace ID during binder_init_ns(). > > if (reply) { >
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 08, 2018 at 01:02:32PM +, chouryzhou(周威) wrote: > We are working for running android in container, but we found that 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 more than one 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. Although statistics in > debugfs > remain global. > > Signed-off-by: chouryzhou > --- > drivers/android/binder.c | 128 > +++--- > include/linux/ipc_namespace.h | 15 + > ipc/namespace.c | 10 +++- > 3 files changed, 117 insertions(+), 36 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index cb30a524d16d..22e45bb937e6 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -68,6 +68,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -80,13 +81,18 @@ > #include "binder_alloc.h" > #include "binder_trace.h" > > + > +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE) > +struct ipc_namespace init_ipc_ns; > +#define ipcns (_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); > > @@ -232,7 +238,7 @@ struct binder_transaction_log_entry { > int return_error_line; > uint32_t return_error; > uint32_t return_error_param; > - const char *context_name; > + int context_device; > }; > struct binder_transaction_log { > atomic_t cur; > @@ -263,19 +269,64 @@ 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; > - const char *name; > + intdevice; > }; > > 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(>binder_procs_lock); > + hlist_for_each_entry_safe(context, tmp, >binder_contexts, hlist) { > + mutex_destroy(>context_mgr_node_lock); > + hlist_del(>hlist); > + kfree(context); > + } > +} > + > +int binder_init_ns(struct ipc_namespace *ns) > +{ > + int ret; > + struct binder_device *device; > + > + mutex_init(>binder_procs_lock); > + INIT_HLIST_HEAD(>binder_procs); > + INIT_HLIST_HEAD(>binder_contexts); > + > + hlist_for_each_entry(device, _devices, hlist) { > + struct binder_context *context; > + > + context = kzalloc(sizeof(*context), GFP_KERNEL); > + if (!context) { > + ret = -ENOMEM; > + goto err; > + } > + > + context->device = device->miscdev.minor; > + context->binder_context_mgr_uid = INVALID_UID; > + mutex_init(>context_mgr_node_lock); > + > + hlist_add_head(>hlist, >binder_contexts); > + } > + > + return 0; > +err: > + binder_exit_ns(ns); > + return ret; > +} > + > + > /** > * struct binder_work - work enqueued on a worklist > * @entry: node enqueued on list > @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc, > e->target_handle = tr->target.handle; > e->data_size = tr->data_size; > e->offsets_size = tr->offsets_size; > - e->context_name = proc->context->name; > + e->context_device = proc->context->device; > > if (reply) { > binder_inner_proc_lock(proc); > @@ -4922,6 +4973,7 @@ static int binder_open(struct inode *nodp, struct file > *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 +4989,15 @@ static int binder_open(struct inode *nodp, struct file > *filp) So the idea is that on open() of a binder device you attach an ipc namespace to the opening process? So you'd basically be able to create a new device node with the same minor and major number as the one in the host ipc namespace in
Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 08, 2018 at 01:02:32PM +, chouryzhou(周威) wrote: > We are working for running android in container, but we found that 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 more than one 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. Although statistics in > debugfs > remain global. > > Signed-off-by: chouryzhou > --- > drivers/android/binder.c | 128 > +++--- > include/linux/ipc_namespace.h | 15 + > ipc/namespace.c | 10 +++- > 3 files changed, 117 insertions(+), 36 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index cb30a524d16d..22e45bb937e6 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -68,6 +68,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -80,13 +81,18 @@ > #include "binder_alloc.h" > #include "binder_trace.h" > > + > +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE) > +struct ipc_namespace init_ipc_ns; > +#define ipcns (_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); > > @@ -232,7 +238,7 @@ struct binder_transaction_log_entry { > int return_error_line; > uint32_t return_error; > uint32_t return_error_param; > - const char *context_name; > + int context_device; > }; > struct binder_transaction_log { > atomic_t cur; > @@ -263,19 +269,64 @@ 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; > - const char *name; > + intdevice; > }; > > 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(>binder_procs_lock); > + hlist_for_each_entry_safe(context, tmp, >binder_contexts, hlist) { > + mutex_destroy(>context_mgr_node_lock); > + hlist_del(>hlist); > + kfree(context); > + } > +} > + > +int binder_init_ns(struct ipc_namespace *ns) > +{ > + int ret; > + struct binder_device *device; > + > + mutex_init(>binder_procs_lock); > + INIT_HLIST_HEAD(>binder_procs); > + INIT_HLIST_HEAD(>binder_contexts); > + > + hlist_for_each_entry(device, _devices, hlist) { > + struct binder_context *context; > + > + context = kzalloc(sizeof(*context), GFP_KERNEL); > + if (!context) { > + ret = -ENOMEM; > + goto err; > + } > + > + context->device = device->miscdev.minor; > + context->binder_context_mgr_uid = INVALID_UID; > + mutex_init(>context_mgr_node_lock); > + > + hlist_add_head(>hlist, >binder_contexts); > + } > + > + return 0; > +err: > + binder_exit_ns(ns); > + return ret; > +} > + > + > /** > * struct binder_work - work enqueued on a worklist > * @entry: node enqueued on list > @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc, > e->target_handle = tr->target.handle; > e->data_size = tr->data_size; > e->offsets_size = tr->offsets_size; > - e->context_name = proc->context->name; > + e->context_device = proc->context->device; > > if (reply) { > binder_inner_proc_lock(proc); > @@ -4922,6 +4973,7 @@ static int binder_open(struct inode *nodp, struct file > *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 +4989,15 @@ static int binder_open(struct inode *nodp, struct file > *filp) So the idea is that on open() of a binder device you attach an ipc namespace to the opening process? So you'd basically be able to create a new device node with the same minor and major number as the one in the host ipc namespace in