Hi,
following is a revised set of the CGroup Namespace patchset which Aditya
Kali has previously sent. The code can also be found in the cgroupns.v4
branch of
https://git.kernel.org/cgit/linux/kernel/git/sergeh/linux-security.git/
To summarize the semantics:
1. CLONE_NEWCGROUP re-uses 0x02000000, which was previously CLONE_STOPPED
2. unsharing a cgroup namespace makes all your current cgroups your new
cgroup root.
3. /proc/pid/cgroup always shows cgroup paths relative to the reader's
cgroup namespce root. A task outside of your cgroup looks like
8:memory:/../../..
4. when a task mounts a cgroupfs, the cgroup which shows up as root depends
on the mounting task's cgroup namespace.
5. setns to a cgroup namespace switches your cgroup namespace but not
your cgroups.
With this, using github.com/hallyn/lxc #2015-11-09/cgns (and
github.com/hallyn/lxcfs #2015-11-10/cgns) we can start a container in a full
proper cgroup namespace, avoiding either cgmanager or lxcfs cgroup bind mounts.
This is completely backward compatible and will be completely invisible
to any existing cgroup users (except for those running inside a cgroup
namespace and looking at /proc/pid/cgroup of tasks outside their
namespace.)
Changes from V3:
1. Rebased onto latest cgroup changes. In particular switch to
css_set_lock and ns_common.
2. Support all hierarchies.
Changes from V2:
1. Added documentation in Documentation/cgroups/namespace.txt
2. Fixed a bug that caused crash
3. Incorporated some other suggestions from last patchset:
- removed use of threadgroup_lock() while creating new cgroupns
- use task_lock() instead of rcu_read_lock() while accessing
task->nsproxy
- optimized setns() to own cgroupns
- simplified code around sane-behavior mount option parsing
4. Restored ACKs from Serge Hallyn from v1 on few patches that have
not changed since then.
Changes from V1:
1. No pinning of processes within cgroupns. Tasks can be freely moved
across cgroups even outside of their cgroupns-root. Usual DAC/MAC policies
apply as before.
2. Path in /proc/<pid>/cgroup is now always shown and is relative to
cgroupns-root. So path can contain '/..' strings depending on cgroupns-root
of the reader and cgroup of <pid>.
3. setns() does not require the process to first move under target
cgroupns-root.
Changes form RFC (V0):
1. setns support for cgroupns
2. 'mount -t cgroup cgroup <mntpt>' from inside a cgroupns now
mounts the cgroup hierarcy with cgroupns-root as the filesystem root.
3. writes to cgroup files outside of cgroupns-root are not allowed
4. visibility of /proc/<pid>/cgroup is further restricted by not showing
anything if the <pid> is in a sibling cgroupns and its cgroup falls outside
your cgroupns-root.
From: Aditya Kali <redacted>
move cgroup_get() and cgroup_put() into cgroup.h so that
they can be called from other places.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge Hallyn <redacted>
---
include/linux/cgroup.h | 21 +++++++++++++++++++++
kernel/cgroup.c | 22 ----------------------
2 files changed, 21 insertions(+), 22 deletions(-)
From: Aditya Kali <redacted>
CLONE_NEWCGROUP will be used to create new cgroup namespace.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge Hallyn <redacted>
---
include/uapi/linux/sched.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -21,8 +21,7 @@#define CLONE_DETACHED 0x00400000 /* Unused, ignored */#define CLONE_UNTRACED 0x00800000 /* set if the tracing process can't force CLONE_PTRACE on this clone */#define CLONE_CHILD_SETTID 0x01000000 /* set the TID in the child */-/* 0x02000000 was previously the unused CLONE_STOPPED (Start in stopped state)-andisnowavailableforre-use.*/+#define CLONE_NEWCGROUP 0x02000000 /* New cgroup namespace */#define CLONE_NEWUTS 0x04000000 /* New utsname namespace */#define CLONE_NEWIPC 0x08000000 /* New ipc namespace */#define CLONE_NEWUSER 0x10000000 /* New user namespace */
From: Aditya Kali <redacted>
This patch enables cgroup mounting inside userns when a process
as appropriate privileges. The cgroup filesystem mounted is
rooted at the cgroupns-root. Thus, in a container-setup, only
the hierarchy under the cgroupns-root is exposed inside the container.
This allows container management tools to run inside the containers
without depending on any global state.
In order to support this, a new kernfs api is added to lookup the
dentry for the cgroupns-root.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
fs/kernfs/mount.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/kernfs.h | 2 ++
kernel/cgroup.c | 32 +++++++++++++++++++++++++++++++-
3 files changed, 81 insertions(+), 1 deletion(-)
@@ -62,6 +62,54 @@ struct kernfs_root *kernfs_root_from_sb(struct super_block *sb)returnNULL;}+/**+*kernfs_obtain_root-getadentryforthegivenkernfs_node+*@sb:thekernfssuper_block+*@kn:kernfs_nodeforwhichadentryisneeded+*+*Thiscanusedusedbycallerswhichwanttomountonlyapartofthekernfs+*asrootofthefilesystem.+*/+structdentry*kernfs_obtain_root(structsuper_block*sb,+structkernfs_node*kn)+{+structdentry*dentry;+structinode*inode;++BUG_ON(sb->s_op!=&kernfs_sops);++/* inode for the given kernfs_node should already exist. */+inode=ilookup(sb,kn->ino);+if(!inode){+pr_debug("kernfs: could not get inode for '");+pr_cont_kernfs_path(kn);+pr_cont("'.\n");+returnERR_PTR(-EINVAL);+}++/* instantiate and link root dentry */+dentry=d_obtain_root(inode);+if(!dentry){+pr_debug("kernfs: could not get dentry for '");+pr_cont_kernfs_path(kn);+pr_cont("'.\n");+returnERR_PTR(-ENOMEM);+}++/* If this is a new dentry, set it up. We need kernfs_mutex because this+*maybecalledbycallersotherthankernfs_fill_super.*/+mutex_lock(&kernfs_mutex);+if(!dentry->d_fsdata){+kernfs_get(kn);+dentry->d_fsdata=kn;+}else{+WARN_ON(dentry->d_fsdata!=kn);+}+mutex_unlock(&kernfs_mutex);++returndentry;+}+staticintkernfs_fill_super(structsuper_block*sb,unsignedlongmagic){structkernfs_super_info*info=kernfs_info(sb);
@@ -1980,6 +1980,14 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,intret;inti;boolnew_sb;+structcgroup_namespace*ns=+get_cgroup_ns(current->nsproxy->cgroup_ns);++/* Check if the caller has permission to mount. */+if(!ns_capable(ns->user_ns,CAP_SYS_ADMIN)){+put_cgroup_ns(ns);+returnERR_PTR(-EPERM);+}/**Thefirsttimeanyonetriestomountacgroup,enablethelist
@@ -2112,11 +2120,31 @@ out_free:kfree(opts.release_agent);kfree(opts.name);-if(ret)+if(ret){+put_cgroup_ns(ns);returnERR_PTR(ret);+}dentry=kernfs_mount(fs_type,flags,root->kf_root,CGROUP_SUPER_MAGIC,&new_sb);++if(!IS_ERR(dentry)){+/* In non-init cgroup namespace, instead of root cgroup's+*dentry,wereturnthedentrycorrespondingtothe+*cgroupns->root_cgrp.+*/+if(ns!=&init_cgroup_ns){+structdentry*nsdentry;+structcgroup*cgrp;++cgrp=cset_cgroup_from_root(ns->root_cgrps,root);+nsdentry=kernfs_obtain_root(dentry->d_sb,+cgrp->kn);+dput(dentry);+dentry=nsdentry;+}+}+if(IS_ERR(dentry)||!new_sb)cgroup_put(&root->cgrp);
@@ -0,0 +1,142 @@+ CGroup Namespaces++CGroup Namespace provides a mechanism to virtualize the view of the+/proc/<pid>/cgroup file. The CLONE_NEWCGROUP clone-flag can be used with+clone() and unshare() syscalls to create a new cgroup namespace.+The process running inside the cgroup namespace will have its /proc/<pid>/cgroup+output restricted to cgroupns-root. cgroupns-root is the cgroup of the process+at the time of creation of the cgroup namespace.++Prior to CGroup Namespace, the /proc/<pid>/cgroup file used to show complete+path of the cgroup of a process. In a container setup (where a set of cgroups+and namespaces are intended to isolate processes), the /proc/<pid>/cgroup file+may leak potential system level information to the isolated processes.++For Example:+ $ cat /proc/self/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/container_id1++The path '/batchjobs/container_id1' can generally be considered as system-data+and its desirable to not expose it to the isolated process.++CGroup Namespaces can be used to restrict visibility of this path.+For Example:+ # Before creating cgroup namespace+ $ ls -l /proc/self/ns/cgroup+ lrwxrwxrwx 1 root root 0 2014-07-15 10:37 /proc/self/ns/cgroup -> cgroup:[4026531835]+ $ cat /proc/self/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/container_id1++ # unshare(CLONE_NEWCGROUP) and exec /bin/bash+ $ ~/unshare -c+ [ns]$ ls -l /proc/self/ns/cgroup+ lrwxrwxrwx 1 root root 0 2014-07-15 10:35 /proc/self/ns/cgroup -> cgroup:[4026532183]+ # From within new cgroupns, process sees that its in the root cgroup+ [ns]$ cat /proc/self/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/++ # From global cgroupns:+ $ cat /proc/<pid>/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/container_id1++ # Unshare cgroupns along with userns and mountns+ # Following calls unshare(CLONE_NEWCGROUP|CLONE_NEWUSER|CLONE_NEWNS), then+ # sets up uid/gid map and execs /bin/bash+ $ ~/unshare -c -u -m+ # Originally, we were in /batchjobs/container_id1 cgroup. Mount our own cgroup+ # hierarchy.+ [ns]$ mount -t cgroup cgroup /tmp/cgroup+ [ns]$ ls -l /tmp/cgroup+ total 0+ -r--r--r-- 1 root root 0 2014-10-13 09:32 cgroup.controllers+ -r--r--r-- 1 root root 0 2014-10-13 09:32 cgroup.populated+ -rw-r--r-- 1 root root 0 2014-10-13 09:25 cgroup.procs+ -rw-r--r-- 1 root root 0 2014-10-13 09:32 cgroup.subtree_control++The cgroupns-root (/batchjobs/container_id1 in above example) becomes the+filesystem root for the namespace specific cgroupfs mount.++The virtualization of /proc/self/cgroup file combined with restricting+the view of cgroup hierarchy by namespace-private cgroupfs mount+should provide a completely isolated cgroup view inside the container.++In its current form, the cgroup namespaces patcheset provides following+behavior:++(1) The 'cgroupns-root' for a cgroup namespace is the cgroup in which+ the process calling unshare is running.+ For ex. if a process in /batchjobs/container_id1 cgroup calls unshare,+ cgroup /batchjobs/container_id1 becomes the cgroupns-root.+ For the init_cgroup_ns, this is the real root ('/') cgroup+ (identified in code as cgrp_dfl_root.cgrp).++(2) The cgroupns-root cgroup does not change even if the namespace+ creator process later moves to a different cgroup.+ $ ~/unshare -c # unshare cgroupns in some cgroup+ [ns]$ cat /proc/self/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/+ [ns]$ mkdir sub_cgrp_1+ [ns]$ echo 0 > sub_cgrp_1/cgroup.procs+ [ns]$ cat /proc/self/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgrp_1++(3) Each process gets its CGROUPNS specific view of /proc/<pid>/cgroup+(a) Processes running inside the cgroup namespace will be able to see+ cgroup paths (in /proc/self/cgroup) only inside their root cgroup+ [ns]$ sleep 100000 & # From within unshared cgroupns+ [1] 7353+ [ns]$ echo 7353 > sub_cgrp_1/cgroup.procs+ [ns]$ cat /proc/7353/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgrp_1++(b) From global cgroupns, the real cgroup path will be visible:+ $ cat /proc/7353/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/container_id1/sub_cgrp_1++(c) From a sibling cgroupns (cgroupns root-ed at a different cgroup), cgroup+ path relative to its own cgroupns-root will be shown:+ # ns2's cgroupns-root is at '/batchjobs/container_id2'+ [ns2]$ cat /proc/7353/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/../container_id2/sub_cgrp_1++ Note that the relative path always starts with '/' to indicate that its+ relative to the cgroupns-root of the caller.++(4) Processes inside a cgroupns can move in-and-out of the cgroupns-root+ (if they have proper access to external cgroups).+ # From inside cgroupns (with cgroupns-root at /batchjobs/container_id1), and+ # assuming that the global hierarchy is still accessible inside cgroupns:+ $ cat /proc/7353/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgrp_1+ $ echo 7353 > batchjobs/container_id2/cgroup.procs+ $ cat /proc/7353/cgroup+ 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/../container_id2++ Note that this kind of setup is not encouraged. A task inside cgroupns+ should only be exposed to its own cgroupns hierarchy. Otherwise it makes+ the virtualization of /proc/<pid>/cgroup less useful.++(5) Setns to another cgroup namespace is allowed when:+ (a) the process has CAP_SYS_ADMIN in its current userns+ (b) the process has CAP_SYS_ADMIN in the target cgroupns' userns+ No implicit cgroup changes happen with attaching to another cgroupns. It+ is expected that the somone moves the attaching process under the target+ cgroupns-root.++(6) When some thread from a multi-threaded process unshares its+ cgroup-namespace, the new cgroupns gets applied to the entire process (all+ the threads). For the unified-hierarchy this is expected as it only allows+ process-level containerization. For the legacy hierarchies this may be+ unexpected. So all the threads in the process will have the same cgroup.++(7) The cgroup namespace is alive as long as there is atleast 1+ process inside it. When the last process exits, the cgroup+ namespace is destroyed. The cgroupns-root and the actual cgroups+ remain though.++(8) Namespace specific cgroup hierarchy can be mounted by a process running+ inside cgroupns:+ $ mount -t cgroup -o __DEVEL__sane_behavior cgroup $MOUNT_POINT++ This will mount the unified cgroup hierarchy with cgroupns-root as the+ filesystem root. The process needs CAP_SYS_ADMIN in its userns and mntns.
From: Aditya Kali <redacted>
Introduce the ability to create new cgroup namespace. The newly created
cgroup namespace remembers the cgroup of the process at the point
of creation of the cgroup namespace (referred as cgroupns-root).
The main purpose of cgroup namespace is to virtualize the contents
of /proc/self/cgroup file. Processes inside a cgroup namespace
are only able to see paths relative to their namespace root
(unless they are moved outside of their cgroupns-root, at which point
they will see a relative path from their cgroupns-root).
For a correctly setup container this enables container-tools
(like libcontainer, lxc, lmctfy, etc.) to create completely virtualized
containers without leaking system level cgroup hierarchy to the task.
This patch only implements the 'unshare' part of the cgroupns.
Signed-off-by: Aditya Kali <redacted>
Signed-off-by: Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
fs/proc/namespaces.c | 3 +
include/linux/cgroup.h | 21 ++++---
include/linux/cgroup_namespace.h | 46 ++++++++++++++
include/linux/nsproxy.h | 2 +
include/linux/proc_ns.h | 4 ++
kernel/Makefile | 2 +-
kernel/cgroup.c | 39 +++++++++++-
kernel/cgroup_namespace.c | 127 ++++++++++++++++++++++++++++++++++++++
kernel/fork.c | 2 +-
kernel/nsproxy.c | 21 ++++++-
10 files changed, 252 insertions(+), 15 deletions(-)
create mode 100644 include/linux/cgroup_namespace.h
create mode 100644 kernel/cgroup_namespace.c
@@ -5237,6 +5270,8 @@ int __init cgroup_init(void)BUG_ON(cgroup_init_cftypes(NULL,cgroup_dfl_base_files));BUG_ON(cgroup_init_cftypes(NULL,cgroup_legacy_base_files));+get_user_ns(init_cgroup_ns.user_ns);+mutex_lock(&cgroup_mutex);/* Add init_css_set to the hash table */
@@ -0,0 +1,127 @@+/*+*Copyright(C)2014GoogleInc.+*+*Author:AdityaKali(adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org)+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsoftheGNUGeneralPublicLicenseaspublishedbytheFree+*SoftwareFoundation,version2oftheLicense.+*/++#include<linux/cgroup.h>+#include<linux/cgroup_namespace.h>+#include<linux/sched.h>+#include<linux/slab.h>+#include<linux/nsproxy.h>+#include<linux/proc_ns.h>++conststructproc_ns_operationscgroupns_operations;++staticstructcgroup_namespace*alloc_cgroup_ns(void)+{+structcgroup_namespace*new_ns;+intret;++new_ns=kzalloc(sizeof(structcgroup_namespace),GFP_KERNEL);+if(!new_ns)+returnERR_PTR(-ENOMEM);+ret=ns_alloc_inum(&new_ns->ns);+if(ret){+kfree(new_ns);+returnERR_PTR(ret);+}+atomic_set(&new_ns->count,1);+new_ns->ns.ops=&cgroupns_operations;+returnnew_ns;+}++externvoidput_css_set(structcss_set*cset);+externvoidget_css_set(structcss_set*cset);++voidfree_cgroup_ns(structcgroup_namespace*ns)+{+put_css_set(ns->root_cgrps);+put_user_ns(ns->user_ns);+ns_free_inum(&ns->ns);+kfree(ns);+}+EXPORT_SYMBOL(free_cgroup_ns);++structcgroup_namespace*copy_cgroup_ns(unsignedlongflags,+structuser_namespace*user_ns,+structcgroup_namespace*old_ns)+{+structcgroup_namespace*new_ns=NULL;+structcss_set*cgrps=NULL;+interr;++BUG_ON(!old_ns);++if(!(flags&CLONE_NEWCGROUP))+returnget_cgroup_ns(old_ns);++/* Allow only sysadmin to create cgroup namespace. */+err=-EPERM;+if(!ns_capable(user_ns,CAP_SYS_ADMIN))+gotoerr_out;++cgrps=task_css_set(current);+get_css_set(cgrps);++err=-ENOMEM;+new_ns=alloc_cgroup_ns();+if(!new_ns)+gotoerr_out;++new_ns->user_ns=get_user_ns(user_ns);+new_ns->root_cgrps=cgrps;++returnnew_ns;++err_out:+if(cgrps)+put_css_set(cgrps);+kfree(new_ns);+returnERR_PTR(err);+}++staticintcgroupns_install(structnsproxy*nsproxy,void*ns)+{+pr_info("setns not supported for cgroup namespace");+return-EINVAL;+}++staticstructns_common*cgroupns_get(structtask_struct*task)+{+structcgroup_namespace*ns=NULL;+structnsproxy*nsproxy;++task_lock(task);+nsproxy=task->nsproxy;+if(nsproxy){+ns=nsproxy->cgroup_ns;+get_cgroup_ns(ns);+}+task_unlock(task);++returnns?&ns->ns:NULL;+}++staticvoidcgroupns_put(structns_common*ns)+{+put_cgroup_ns(to_cg_ns(ns));+}++conststructproc_ns_operationscgroupns_operations={+.name="cgroup",+.type=CLONE_NEWCGROUP,+.get=cgroupns_get,+.put=cgroupns_put,+.install=cgroupns_install,+};++static__initintcgroup_namespaces_init(void)+{+return0;+}+subsys_initcall(cgroup_namespaces_init);
@@ -1884,7 +1884,7 @@ static int check_unshare_flags(unsigned long unshare_flags)if(unshare_flags&~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNET|-CLONE_NEWUSER|CLONE_NEWPID))+CLONE_NEWUSER|CLONE_NEWPID|CLONE_NEWCGROUP))return-EINVAL;/**Notimplemented,butpretenditworksifthereisnothing
@@ -180,7 +197,7 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,interr=0;if(!(unshare_flags&(CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|-CLONE_NEWNET|CLONE_NEWPID)))+CLONE_NEWNET|CLONE_NEWPID|CLONE_NEWCGROUP)))return0;user_ns=new_cred?new_cred->user_ns:current_user_ns();
From: Aditya Kali <redacted>
setns on a cgroup namespace is allowed only if
task has CAP_SYS_ADMIN in its current user-namespace and
over the user-namespace associated with target cgroupns.
No implicit cgroup changes happen with attaching to another
cgroupns. It is expected that the somone moves the attaching
process under the target cgroupns-root.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
kernel/cgroup_namespace.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
@@ -85,10 +85,27 @@ err_out:returnERR_PTR(err);}-staticintcgroupns_install(structnsproxy*nsproxy,void*ns)+staticinlinestructcgroup_namespace*to_cg_ns(structns_common*ns){+returncontainer_of(ns,structcgroup_namespace,ns);+}++staticintcgroupns_install(structnsproxy*nsproxy,structns_common*ns){-pr_info("setns not supported for cgroup namespace");-return-EINVAL;+structcgroup_namespace*cgroup_ns=to_cg_ns(ns);++if(!ns_capable(current_user_ns(),CAP_SYS_ADMIN)||+!ns_capable(cgroup_ns->user_ns,CAP_SYS_ADMIN))+return-EPERM;++/* Don't need to do anything if we are attaching to our own cgroupns. */+if(cgroup_ns==nsproxy->cgroup_ns)+return0;++get_cgroup_ns(cgroup_ns);+put_cgroup_ns(nsproxy->cgroup_ns);+nsproxy->cgroup_ns=cgroup_ns;++return0;}staticstructns_common*cgroupns_get(structtask_struct*task)
From: Aditya Kali <redacted>
The new function kernfs_path_from_node() generates and returns
kernfs path of a given kernfs_node relative to a given parent
kernfs_node.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
fs/kernfs/dir.c | 195 ++++++++++++++++++++++++++++++++++++++++++------
include/linux/kernfs.h | 3 +
2 files changed, 177 insertions(+), 21 deletions(-)
@@ -44,28 +44,159 @@ static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)returnstrlcpy(buf,kn->parent?kn->name:"/",buflen);}-staticchar*__must_checkkernfs_path_locked(structkernfs_node*kn,char*buf,-size_tbuflen)+/**+*kernfs_node_depth-computedepthofthekernfsnodefromroot.+*Therootnodeitselfisconsideredtobeatdepth0.+*/+staticsize_tkernfs_node_depth(structkernfs_node*kn){-char*p=buf+buflen;+size_tdepth=0;++BUG_ON(!kn);+while(kn->parent){+depth++;+kn=kn->parent;+}+returndepth;+}++/**+*kernfs_path_from_node_locked-findarelativepathfrom@kn_fromto@kn_to+*@kn_from:referencenodeofthepath+*@kn_to:kernfsnodetowhichpathisneeded+*@buf:buffertocopythepathinto+*@buflen:sizeof@buf+*+*Weneedtohandlecoupleofscenarioshere:+*[1]when@kn_fromisanancestorof@kn_toatsomelevel+*kn_from:/n1/n2/n3+*kn_to:/n1/n2/n3/n4/n5+*result:/n4/n5+*+*[2]when@kn_fromisonadifferenthierarchyandweneedtofindcommon+*ancestorbetween@kn_fromand@kn_to.+*kn_from:/n1/n2/n3/n4+*kn_to:/n1/n2/n5+*result:/../../n5+*OR+*kn_from:/n1/n2/n3/n4/n5[depth=5]+*kn_to:/n1/n2/n3[depth=3]+*result:/../..+*/+staticchar*__must_checkkernfs_path_from_node_locked(+structkernfs_node*kn_from,+structkernfs_node*kn_to,+char*buf,+size_tbuflen)+{+char*p=buf;+structkernfs_node*kn;+size_tdepth_from=0,depth_to,d;intlen;-*--p='\0';+/* We atleast need 2 bytes to write "/\0". */+BUG_ON(buflen<2);-do{-len=strlen(kn->name);-if(p-buf<len+1){-buf[0]='\0';-p=NULL;-break;+/* Short-circuit the easy case - kn_to is the root node. */+if((kn_from==kn_to)||(!kn_from&&!kn_to->parent)){+*p='/';+*(p+1)='\0';+returnp;+}++/* We can find the relative path only if both the nodes belong to the+*samekernfsroot.+*/+if(kn_from){+BUG_ON(kernfs_root(kn_from)!=kernfs_root(kn_to));+depth_from=kernfs_node_depth(kn_from);+}++depth_to=kernfs_node_depth(kn_to);++/* We compose path from left to right. So first write out all possible+*"/.."stringsneededtoreachfrom'kn_from'tothecommonancestor.+*/+if(kn_from){+while(depth_from>depth_to){+len=strlen("/..");+if((buflen-(p-buf))<len+1){+/* buffer not big enough. */+buf[0]='\0';+returnNULL;+}+memcpy(p,"/..",len);+p+=len;+*p='\0';+--depth_from;+kn_from=kn_from->parent;}++d=depth_to;+kn=kn_to;+while(depth_from<d){+kn=kn->parent;+d--;+}++/* Now we have 'depth_from == depth_to' at this point. Add more+*"/.."suntilwereachcommonancestor.Intheworstcase,+*rootnodewillbethecommonancestor.+*/+while(depth_from>0){+/* If we reached common ancestor, stop. */+if(kn_from==kn)+break;+len=strlen("/..");+if((buflen-(p-buf))<len+1){+/* buffer not big enough. */+buf[0]='\0';+returnNULL;+}+memcpy(p,"/..",len);+p+=len;+*p='\0';+--depth_from;+kn_from=kn_from->parent;+kn=kn->parent;+}+}++/* Figure out how many bytes we need to write the path.+*/+d=depth_to;+kn=kn_to;+len=0;+while(depth_from<d){+/* Account for "/<name>". */+len+=strlen(kn->name)+1;+kn=kn->parent;+--d;+}++if((buflen-(p-buf))<len+1){+/* buffer not big enough. */+buf[0]='\0';+returnNULL;+}++/* We have enough space. Move 'p' ahead by computed length and start+*writingnodenamesintobuffer.+*/+p+=len;+*p='\0';+d=depth_to;+kn=kn_to;+while(d>depth_from){+len=strlen(kn->name);p-=len;memcpy(p,kn->name,len);*--p='/';kn=kn->parent;-}while(kn&&kn->parent);+--d;+}-returnp;+returnbuf;}/**
@@ -2210,6 +2210,31 @@ char *task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)}EXPORT_SYMBOL_GPL(task_cgroup_path);+/*+*get_task_cgroup-returnsthecgroupofthetaskinthedefaultcgroup+*hierarchy.+*+*@task:targettask+*Thisfunctionreturnsthe@task'scgrouponthedefaultcgrouphierarchy.The+*returnedcgrouphasitsreferenceincremented(bycallingcgroup_get()).So+*thecallermustcgroup_put()theobtainedreferenceonceitisdonewithit.+*/+structcgroup*get_task_cgroup(structtask_struct*task)+{+structcgroup*cgrp;++mutex_lock(&cgroup_mutex);+spin_lock_bh(&css_set_lock);++cgrp=task_cgroup_from_root(task,&cgrp_dfl_root);+cgroup_get(cgrp);++spin_unlock_bh(&css_set_lock);+mutex_unlock(&cgroup_mutex);+returncgrp;+}+EXPORT_SYMBOL_GPL(get_task_cgroup);+/* used to track tasks and other necessary states during migration */structcgroup_taskset{/* the src and dst cset list running through cset->mg_node */
From: Richard Weinberger <hidden> Date: 2015-11-16 20:41:19
Serge,
On Mon, Nov 16, 2015 at 8:51 PM, [off-list ref] wrote:
To summarize the semantics:
1. CLONE_NEWCGROUP re-uses 0x02000000, which was previously CLONE_STOPPED
2. unsharing a cgroup namespace makes all your current cgroups your new
cgroup root.
3. /proc/pid/cgroup always shows cgroup paths relative to the reader's
cgroup namespce root. A task outside of your cgroup looks like
8:memory:/../../..
4. when a task mounts a cgroupfs, the cgroup which shows up as root depends
on the mounting task's cgroup namespace.
5. setns to a cgroup namespace switches your cgroup namespace but not
your cgroups.
With this, using github.com/hallyn/lxc #2015-11-09/cgns (and
github.com/hallyn/lxcfs #2015-11-10/cgns) we can start a container in a full
proper cgroup namespace, avoiding either cgmanager or lxcfs cgroup bind mounts.
This is completely backward compatible and will be completely invisible
to any existing cgroup users (except for those running inside a cgroup
namespace and looking at /proc/pid/cgroup of tasks outside their
namespace.)
cgroupns-root.
IIRC one downside of this series was that only the new "sane" cgroup
layout was supported
and hence it was useless for everything which expected the default layout.
Hence, still no systemd for us. :)
Is this now different?
--
Thanks,
//richard
From: "Serge E. Hallyn" <serge@hallyn.com> Date: 2015-11-16 20:46:13
On Mon, Nov 16, 2015 at 09:41:15PM +0100, Richard Weinberger wrote:
Serge,
On Mon, Nov 16, 2015 at 8:51 PM, [off-list ref] wrote:
quoted
To summarize the semantics:
1. CLONE_NEWCGROUP re-uses 0x02000000, which was previously CLONE_STOPPED
2. unsharing a cgroup namespace makes all your current cgroups your new
cgroup root.
3. /proc/pid/cgroup always shows cgroup paths relative to the reader's
cgroup namespce root. A task outside of your cgroup looks like
8:memory:/../../..
4. when a task mounts a cgroupfs, the cgroup which shows up as root depends
on the mounting task's cgroup namespace.
5. setns to a cgroup namespace switches your cgroup namespace but not
your cgroups.
With this, using github.com/hallyn/lxc #2015-11-09/cgns (and
github.com/hallyn/lxcfs #2015-11-10/cgns) we can start a container in a full
proper cgroup namespace, avoiding either cgmanager or lxcfs cgroup bind mounts.
This is completely backward compatible and will be completely invisible
to any existing cgroup users (except for those running inside a cgroup
namespace and looking at /proc/pid/cgroup of tasks outside their
namespace.)
cgroupns-root.
IIRC one downside of this series was that only the new "sane" cgroup
layout was supported
and hence it was useless for everything which expected the default layout.
Hence, still no systemd for us. :)
Is this now different?
From: Richard Weinberger <richard@nod.at> Date: 2015-11-16 20:51:07
Am 16.11.2015 um 21:46 schrieb Serge E. Hallyn:
On Mon, Nov 16, 2015 at 09:41:15PM +0100, Richard Weinberger wrote:
quoted
Serge,
On Mon, Nov 16, 2015 at 8:51 PM, [off-list ref] wrote:
quoted
To summarize the semantics:
1. CLONE_NEWCGROUP re-uses 0x02000000, which was previously CLONE_STOPPED
2. unsharing a cgroup namespace makes all your current cgroups your new
cgroup root.
3. /proc/pid/cgroup always shows cgroup paths relative to the reader's
cgroup namespce root. A task outside of your cgroup looks like
8:memory:/../../..
4. when a task mounts a cgroupfs, the cgroup which shows up as root depends
on the mounting task's cgroup namespace.
5. setns to a cgroup namespace switches your cgroup namespace but not
your cgroups.
With this, using github.com/hallyn/lxc #2015-11-09/cgns (and
github.com/hallyn/lxcfs #2015-11-10/cgns) we can start a container in a full
proper cgroup namespace, avoiding either cgmanager or lxcfs cgroup bind mounts.
This is completely backward compatible and will be completely invisible
to any existing cgroup users (except for those running inside a cgroup
namespace and looking at /proc/pid/cgroup of tasks outside their
namespace.)
cgroupns-root.
IIRC one downside of this series was that only the new "sane" cgroup
layout was supported
and hence it was useless for everything which expected the default layout.
Hence, still no systemd for us. :)
Is this now different?
Yes, all hierachies are no supported.
Should read "now"? :-)
If so, *awesome*!
Thanks,
//richard
From: "Serge E. Hallyn" <serge@hallyn.com> Date: 2015-11-16 20:54:56
On Mon, Nov 16, 2015 at 09:50:55PM +0100, Richard Weinberger wrote:
Am 16.11.2015 um 21:46 schrieb Serge E. Hallyn:
quoted
On Mon, Nov 16, 2015 at 09:41:15PM +0100, Richard Weinberger wrote:
quoted
Serge,
On Mon, Nov 16, 2015 at 8:51 PM, [off-list ref] wrote:
quoted
To summarize the semantics:
1. CLONE_NEWCGROUP re-uses 0x02000000, which was previously CLONE_STOPPED
2. unsharing a cgroup namespace makes all your current cgroups your new
cgroup root.
3. /proc/pid/cgroup always shows cgroup paths relative to the reader's
cgroup namespce root. A task outside of your cgroup looks like
8:memory:/../../..
4. when a task mounts a cgroupfs, the cgroup which shows up as root depends
on the mounting task's cgroup namespace.
5. setns to a cgroup namespace switches your cgroup namespace but not
your cgroups.
With this, using github.com/hallyn/lxc #2015-11-09/cgns (and
github.com/hallyn/lxcfs #2015-11-10/cgns) we can start a container in a full
proper cgroup namespace, avoiding either cgmanager or lxcfs cgroup bind mounts.
This is completely backward compatible and will be completely invisible
to any existing cgroup users (except for those running inside a cgroup
namespace and looking at /proc/pid/cgroup of tasks outside their
namespace.)
cgroupns-root.
IIRC one downside of this series was that only the new "sane" cgroup
layout was supported
and hence it was useless for everything which expected the default layout.
Hence, still no systemd for us. :)
Is this now different?
I don't think this is BUG worthy. Just return NULL? Also, the only
reason the original function returned char * was because the starting
point may not be the start of the buffer which helps keeping the
implementation simple. If this function is gonna be complex anyway, a
better approach would be returning ssize_t and implement a simliar
behavior to strlcpy().
quoted hunk
+ /* Short-circuit the easy case - kn_to is the root node. */+ if ((kn_from == kn_to) || (!kn_from && !kn_to->parent)) {+ *p = '/';+ *(p + 1) = '\0';
Hmm... so if kn_from == kn_to, the output is "/"?
quoted hunk
+ return p;+ }++ /* We can find the relative path only if both the nodes belong to the+ * same kernfs root.+ */+ if (kn_from) {+ BUG_ON(kernfs_root(kn_from) != kernfs_root(kn_to));
Ditto, just return NULL and maybe trigger WARN_ON_ONCE().
quoted hunk
+ depth_from = kernfs_node_depth(kn_from);+ }++ depth_to = kernfs_node_depth(kn_to);++ /* We compose path from left to right. So first write out all possible
^
, so
+ * "/.." strings needed to reach from 'kn_from' to the common ancestor.
+ */
Please fully-wing multiline comments.
quoted hunk
+ if (kn_from) {+ while (depth_from > depth_to) {+ len = strlen("/..");
Maybe do something like the following instead?
const char parent_str[] = "/..";
size_t len = sizeof(parent_str) - 1;
quoted hunk
+ if ((buflen - (p - buf)) < len + 1) {+ /* buffer not big enough. */+ buf[0] = '\0';+ return NULL;+ }+ memcpy(p, "/..", len);+ p += len;+ *p = '\0';+ --depth_from;+ kn_from = kn_from->parent; }++ d = depth_to;+ kn = kn_to;+ while (depth_from < d) {+ kn = kn->parent;+ d--;+ }++ /* Now we have 'depth_from == depth_to' at this point. Add more
Ditto with winging.
quoted hunk
+ * "/.."s until we reach common ancestor. In the worst case,+ * root node will be the common ancestor.+ */+ while (depth_from > 0) {+ /* If we reached common ancestor, stop. */+ if (kn_from == kn)+ break;+ len = strlen("/..");+ if ((buflen - (p - buf)) < len + 1) {+ /* buffer not big enough. */+ buf[0] = '\0';+ return NULL;+ }+ memcpy(p, "/..", len);+ p += len;+ *p = '\0';+ --depth_from;+ kn_from = kn_from->parent;+ kn = kn->parent;+ }
Hmmm... I wonder whether this and the above block can be merged.
Wouldn't it be simpler to calculate common ancestor and generate
/.. till it reached that point?
Thanks.
--
tejun
+/*+ * get_task_cgroup - returns the cgroup of the task in the default cgroup+ * hierarchy.+ *+ * @task: target task+ * This function returns the @task's cgroup on the default cgroup hierarchy. The+ * returned cgroup has its reference incremented (by calling cgroup_get()). So+ * the caller must cgroup_put() the obtained reference once it is done with it.+ */+struct cgroup *get_task_cgroup(struct task_struct *task)+{+ struct cgroup *cgrp;++ mutex_lock(&cgroup_mutex);+ spin_lock_bh(&css_set_lock);++ cgrp = task_cgroup_from_root(task, &cgrp_dfl_root);+ cgroup_get(cgrp);++ spin_unlock_bh(&css_set_lock);+ mutex_unlock(&cgroup_mutex);+ return cgrp;+}+EXPORT_SYMBOL_GPL(get_task_cgroup);
So, exposing cgroup_mutex this way can lead to ugly lock dependency
issues as cgroup_mutex is expected to be outside of pretty much
everything. task_cgroup_path() does it but it has no users (should
prolly removed) and cgroup_attach_task_all() is pretty specific.
Hmmm... cc'ing Li (btw, please cc him and Johannes from the next
posting). Li, I don't think cset_cgroup_from_root() really needs
cgroup_mutex. css_set_lock seems to be enough. What do you think?
Thanks.
--
tejun
Hello,
On Mon, Nov 16, 2015 at 01:51:41PM -0600, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
quoted hunk
From: Aditya Kali <redacted>
move cgroup_get() and cgroup_put() into cgroup.h so that
they can be called from other places.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge Hallyn <redacted>
---
include/linux/cgroup.h | 21 +++++++++++++++++++++
kernel/cgroup.c | 22 ----------------------
2 files changed, 21 insertions(+), 22 deletions(-)
So these are being exposed for cgroup NS support. Hmmm... idk, does
cgroup NS support needs to be in a spearate file? The added amount
isn't that big. If we split cgroup.c, I'd much prefer to have
cgroup-internal.h for internally shared stuff than pushing them out to
cgroup.h.
Thanks.
--
tejun
I personally prefer putting just the return type on a separate line
when things get too long.
static inline struct cgroup_namespace *
get_cgroup_ns(struct cgroup_namespace *ns)
quoted hunk
+{+ if (ns)+ atomic_inc(&ns->count);+ return ns;+}
Ugh... if the function doesn't do anything about the return type,
please make it a void function. We tried the above style with kobj
and driver model and it ended up pretty horrible.
Hello,
On Mon, Nov 16, 2015 at 01:51:44PM -0600, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
quoted hunk
+struct dentry *kernfs_obtain_root(struct super_block *sb,+ struct kernfs_node *kn)+{+ struct dentry *dentry;+ struct inode *inode;++ BUG_ON(sb->s_op != &kernfs_sops);++ /* inode for the given kernfs_node should already exist. */+ inode = ilookup(sb, kn->ino);+ if (!inode) {+ pr_debug("kernfs: could not get inode for '");+ pr_cont_kernfs_path(kn);+ pr_cont("'.\n");+ return ERR_PTR(-EINVAL);+ }
Hmmm... but inode might not have been instantiated yet. Why not use
kernfs_get_inode()?
quoted hunk
+ /* instantiate and link root dentry */+ dentry = d_obtain_root(inode);+ if (!dentry) {+ pr_debug("kernfs: could not get dentry for '");+ pr_cont_kernfs_path(kn);+ pr_cont("'.\n");+ return ERR_PTR(-ENOMEM);+ }++ /* If this is a new dentry, set it up. We need kernfs_mutex because this+ * may be called by callers other than kernfs_fill_super. */
So, this would effectively allow namespace mounts to claim controllers
which aren't configured otherwise which doesn't seem like a good idea.
I think the right thing to do for namespace mounts is to always
require an existing superblock.
Thanks.
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-11-24 22:35:15
On Tue, Nov 24, 2015 at 11:30:56AM -0500, Tejun Heo wrote:
Hello,
On Mon, Nov 16, 2015 at 01:51:41PM -0600, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
quoted
From: Aditya Kali <redacted>
move cgroup_get() and cgroup_put() into cgroup.h so that
they can be called from other places.
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge Hallyn <redacted>
---
include/linux/cgroup.h | 21 +++++++++++++++++++++
kernel/cgroup.c | 22 ----------------------
2 files changed, 21 insertions(+), 22 deletions(-)
So these are being exposed for cgroup NS support. Hmmm... idk, does
cgroup NS support needs to be in a spearate file? The added amount
isn't that big. If we split cgroup.c, I'd much prefer to have
cgroup-internal.h for internally shared stuff than pushing them out to
cgroup.h.
Yeah, I think it makes more sense to merge them.
thanks,
-serge
So, this would effectively allow namespace mounts to claim controllers
which aren't configured otherwise which doesn't seem like a good idea.
I think the right thing to do for namespace mounts is to always
require an existing superblock.
Hmmm... but that wouldn't work with non-root and user ns. I think
what's necessary is ensuring that namespace scoped mount never creates
a new hierarchy but always reuses an existing one.
Thanks.
--
tejun
Hmmm... but that wouldn't work with non-root and user ns. I think
Are you sure? IIUC that code block is only hit when we didn't find
an already-mounted subsystem.
what's necessary is ensuring that namespace scoped mount never creates
a new hierarchy but always reuses an existing one.
Thanks.
--
tejun
_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
From: Serge E. Hallyn <hidden> Date: 2015-11-27 05:17:52
On Tue, Nov 24, 2015 at 12:16:10PM -0500, Tejun Heo wrote:
Hello,
On Mon, Nov 16, 2015 at 01:51:44PM -0600, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
quoted
+struct dentry *kernfs_obtain_root(struct super_block *sb,+ struct kernfs_node *kn)+{+ struct dentry *dentry;+ struct inode *inode;++ BUG_ON(sb->s_op != &kernfs_sops);++ /* inode for the given kernfs_node should already exist. */+ inode = ilookup(sb, kn->ino);+ if (!inode) {+ pr_debug("kernfs: could not get inode for '");+ pr_cont_kernfs_path(kn);+ pr_cont("'.\n");+ return ERR_PTR(-EINVAL);+ }
Hmmm... but inode might not have been instantiated yet. Why not use
kernfs_get_inode()?
quoted
+ /* instantiate and link root dentry */+ dentry = d_obtain_root(inode);+ if (!dentry) {+ pr_debug("kernfs: could not get dentry for '");+ pr_cont_kernfs_path(kn);+ pr_cont("'.\n");+ return ERR_PTR(-ENOMEM);+ }++ /* If this is a new dentry, set it up. We need kernfs_mutex because this+ * may be called by callers other than kernfs_fill_super. */
Wouldn't it be simpler to walk dentry from kernfs root than
duplicating dentry instantiation?
Sorry I don't think I'm following. Are you suggesting walking the
kn->parent chain backward and doing d_lookup() at each point starting
with sb->s_root?
(Note I've rewritten this to find a common ancestor and walk back to
and from that, as you suggested later in this email)
quoted
+ /* Short-circuit the easy case - kn_to is the root node. */+ if ((kn_from == kn_to) || (!kn_from && !kn_to->parent)) {+ *p = '/';+ *(p + 1) = '\0';
Hmm... so if kn_from == kn_to, the output is "/"?
Yes, that's what seems to make the most sense for cgroup namespaces. I
could see a case for '.' being used instead in general, but for cgroup
namespaces I think we'd have to convert those back to '/'. Otherwise
we'll fail in being able to run legacy software, which would get
confused.
-serge
Hello, Serge.
On Thu, Nov 26, 2015 at 11:17:45PM -0600, Serge E. Hallyn wrote:
quoted
Wouldn't it be simpler to walk dentry from kernfs root than
duplicating dentry instantiation?
Sorry I don't think I'm following. Are you suggesting walking the
kn->parent chain backward and doing d_lookup() at each point starting
with sb->s_root?
Yeah, something like that. I wonder whether there are already code
paths doing that. What we need is a straight path walk. I could be
wrong but it shouldn't be that complex and if it works out we can
avoid introducing another instantiation / lookup path.
Thanks.
--
tejun
Hello,
On Thu, Nov 26, 2015 at 11:25:11PM -0600, Serge E. Hallyn wrote:
quoted
quoted
+ /* Short-circuit the easy case - kn_to is the root node. */+ if ((kn_from == kn_to) || (!kn_from && !kn_to->parent)) {+ *p = '/';+ *(p + 1) = '\0';
Hmm... so if kn_from == kn_to, the output is "/"?
Yes, that's what seems to make the most sense for cgroup namespaces. I
could see a case for '.' being used instead in general, but for cgroup
namespaces I think we'd have to convert those back to '/'. Otherwise
we'll fail in being able to run legacy software, which would get
confused.
Yeah, I agree but the name is kinda misleading tho. The output isn't
really a relative path but rather absolute path against the specified
root. Maybe updating the function and parameter names would be
helpful?
Thanks.
--
tejun
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
From: Serge E. Hallyn <hidden> Date: 2015-11-30 18:38:03
On Mon, Nov 30, 2015 at 10:11:47AM -0500, Tejun Heo wrote:
Hello,
On Thu, Nov 26, 2015 at 11:25:11PM -0600, Serge E. Hallyn wrote:
quoted
quoted
quoted
+ /* Short-circuit the easy case - kn_to is the root node. */+ if ((kn_from == kn_to) || (!kn_from && !kn_to->parent)) {+ *p = '/';+ *(p + 1) = '\0';
Hmm... so if kn_from == kn_to, the output is "/"?
Yes, that's what seems to make the most sense for cgroup namespaces. I
could see a case for '.' being used instead in general, but for cgroup
namespaces I think we'd have to convert those back to '/'. Otherwise
we'll fail in being able to run legacy software, which would get
confused.
Yeah, I agree but the name is kinda misleading tho. The output isn't
really a relative path but rather absolute path against the specified
root. Maybe updating the function and parameter names would be
helpful?
Thanks.
Ok - updating the comment is simple enough. Though the name/params
kernfs_path_from_node_locked(from, to) still seem to make sense. Would
you prefer something like kernfs_absolute_path_from node_locked()? I
hesitate to call 'from' 'root' since kernfs_root is a thing and this
is not that.
Hello, Serge.
On Mon, Nov 30, 2015 at 12:37:58PM -0600, Serge E. Hallyn wrote:
quoted
Yeah, I agree but the name is kinda misleading tho. The output isn't
really a relative path but rather absolute path against the specified
root. Maybe updating the function and parameter names would be
helpful?
Ok - updating the comment is simple enough. Though the name/params
kernfs_path_from_node_locked(from, to) still seem to make sense. Would
you prefer something like kernfs_absolute_path_from node_locked()? I
hesitate to call 'from' 'root' since kernfs_root is a thing and this
is not that.
Hmmm... I see. Let's just make sure that the comment is clear about
the fact that it calculates (pseudo) absolute path rather than
relative path.
Thanks.
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-12-01 02:08:13
On Mon, Nov 30, 2015 at 05:53:18PM -0500, Tejun Heo wrote:
Hello, Serge.
On Mon, Nov 30, 2015 at 12:37:58PM -0600, Serge E. Hallyn wrote:
quoted
quoted
Yeah, I agree but the name is kinda misleading tho. The output isn't
really a relative path but rather absolute path against the specified
root. Maybe updating the function and parameter names would be
helpful?
Ok - updating the comment is simple enough. Though the name/params
kernfs_path_from_node_locked(from, to) still seem to make sense. Would
you prefer something like kernfs_absolute_path_from node_locked()? I
hesitate to call 'from' 'root' since kernfs_root is a thing and this
is not that.
Hmmm... I see. Let's just make sure that the comment is clear about
the fact that it calculates (pseudo) absolute path rather than
relative path.
Thanks.
Ok, new patch follows (and is pushed at
https://git.kernel.org/cgit/linux/kernel/git/sergeh/linux-security.git/log/?h=2015-11-30/cgroupns)
[PATCH 1/7] kernfs: Add API to generate relative kernfs path
The new function kernfs_path_from_node() generates and returns kernfs
path of a given kernfs_node relative to a given parent kernfs_node.
Changelog 20151125:
- Fully-wing multilinecomments
- Rework kernfs_path_from_node_locked() logic
- Replace BUG_ONs with returning NULL
- Use a const char* for /.. and precalculate its size
Changelog 20151130:
- Update kernfs_path_from_node_locked comment
Signed-off-by: Aditya Kali <redacted>
Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
fs/kernfs/dir.c | 182 +++++++++++++++++++++++++++++++++++++++++--------
include/linux/kernfs.h | 3 +
2 files changed, 158 insertions(+), 27 deletions(-)
@@ -44,28 +44,134 @@ static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)returnstrlcpy(buf,kn->parent?kn->name:"/",buflen);}-staticchar*__must_checkkernfs_path_locked(structkernfs_node*kn,char*buf,-size_tbuflen)+/* kernfs_node_depth - compute depth from @from to @to */+staticsize_tkernfs_node_distance(structkernfs_node*from,structkernfs_node*to){-char*p=buf+buflen;-intlen;+size_tdepth=0;-*--p='\0';+BUG_ON(!to);+BUG_ON(!from);-do{-len=strlen(kn->name);-if(p-buf<len+1){-buf[0]='\0';-p=NULL;-break;-}-p-=len;-memcpy(p,kn->name,len);-*--p='/';-kn=kn->parent;-}while(kn&&kn->parent);+while(to->parent&&to!=from){+depth++;+to=to->parent;+}+returndepth;+}-returnp;+staticstructkernfs_node*kernfs_common_ancestor(structkernfs_node*a,+structkernfs_node*b)+{+size_tda=kernfs_node_distance(kernfs_root(a)->kn,a);+size_tdb=kernfs_node_distance(kernfs_root(b)->kn,b);++if(da==0)+returna;+if(db==0)+returnb;++while(da>db){+a=a->parent;+da--;+}+while(db>da){+b=b->parent;+db--;+}++/* worst case b and a will be the same at root */+while(b!=a){+b=b->parent;+a=a->parent;+}++returna;+}++/**+*kernfs_path_from_node_locked-findapseudo-absolutepathto@kn_to,+*wherekn_fromistreatedasrootofthepath.+*@kn_from:kernfsnodewhichshouldbetreatedasrootforthepath+*@kn_to:kernfsnodetowhichpathisneeded+*@buf:buffertocopythepathinto+*@buflen:sizeof@buf+*+*Weneedtohandlecoupleofscenarioshere:+*[1]when@kn_fromisanancestorof@kn_toatsomelevel+*kn_from:/n1/n2/n3+*kn_to:/n1/n2/n3/n4/n5+*result:/n4/n5+*+*[2]when@kn_fromisonadifferenthierarchyandweneedtofindcommon+*ancestorbetween@kn_fromand@kn_to.+*kn_from:/n1/n2/n3/n4+*kn_to:/n1/n2/n5+*result:/../../n5+*OR+*kn_from:/n1/n2/n3/n4/n5[depth=5]+*kn_to:/n1/n2/n3[depth=3]+*result:/../..+*/+staticchar*+__must_checkkernfs_path_from_node_locked(structkernfs_node*kn_from,+structkernfs_node*kn_to,char*buf,+size_tbuflen)+{+char*p=buf;+structkernfs_node*kn,*common;+constcharparent_str[]="/..";+inti;+size_tdepth_from,depth_to,len=0,nlen=0,+plen=sizeof(parent_str)-1;++/* We atleast need 2 bytes to write "/\0". */+if(buflen<2)+returnNULL;++if(!kn_from)+kn_from=kernfs_root(kn_to)->kn;++if(kn_from==kn_to){+*p='/';+*(++p)='\0';+returnbuf;+}++common=kernfs_common_ancestor(kn_from,kn_to);+if(!common){+WARN_ONCE("%s: kn_from and kn_to on different roots\n",+__func__);+returnNULL;+}++depth_to=kernfs_node_distance(common,kn_to);+depth_from=kernfs_node_distance(common,kn_from);++for(i=0;i<depth_from;i++){+if(len+plen+1>buflen)+returnNULL;+strcpy(p,parent_str);+p+=plen;+len+=plen;+}++/* Calculate how many bytes we need for the rest */+for(kn=kn_to;kn!=common;kn=kn->parent)+nlen+=strlen(kn->name)+1;++if(len+nlen+1>buflen)+returnNULL;++p+=nlen;+*p='\0';+for(kn=kn_to;kn!=common;kn=kn->parent){+nlen=strlen(kn->name);+p-=nlen;+memcpy(p,kn->name,nlen);+*(--p)='/';+}++returnbuf;}/**
From: Serge E. Hallyn <hidden> Date: 2015-12-01 04:07:10
On Mon, Nov 30, 2015 at 10:09:38AM -0500, Tejun Heo wrote:
Hello, Serge.
On Thu, Nov 26, 2015 at 11:17:45PM -0600, Serge E. Hallyn wrote:
quoted
quoted
Wouldn't it be simpler to walk dentry from kernfs root than
duplicating dentry instantiation?
Sorry I don't think I'm following. Are you suggesting walking the
kn->parent chain backward and doing d_lookup() at each point starting
with sb->s_root?
Yeah, something like that. I wonder whether there are already code
paths doing that. What we need is a straight path walk. I could be
wrong but it shouldn't be that complex and if it works out we can
avoid introducing another instantiation / lookup path.
Thanks.
So actually the way the code is now, the first mount cannot
be done from a non-init user namespace; and kernfs_obtain_root()
is only called from non-init user namespace. So can we assume
that the root dentry will be instantiated? (or can it get
evicted?)
If we can assume that then most of that fn can go away.
Hey, Serge.
On Mon, Nov 30, 2015 at 10:07:04PM -0600, Serge E. Hallyn wrote:
So actually the way the code is now, the first mount cannot
be done from a non-init user namespace; and kernfs_obtain_root()
is only called from non-init user namespace. So can we assume
that the root dentry will be instantiated? (or can it get
evicted?)
If we can assume that then most of that fn can go away.
The v2 hierarchy is always mounted and non-init ns shouldn't be able
to create new v1 hierarchies, so the root dentry should always be
there.
Thanks.
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-12-01 21:58:57
On Tue, Dec 01, 2015 at 11:46:49AM -0500, Tejun Heo wrote:
Hey, Serge.
On Mon, Nov 30, 2015 at 10:07:04PM -0600, Serge E. Hallyn wrote:
quoted
So actually the way the code is now, the first mount cannot
be done from a non-init user namespace; and kernfs_obtain_root()
is only called from non-init user namespace. So can we assume
that the root dentry will be instantiated? (or can it get
evicted?)
If we can assume that then most of that fn can go away.
The v2 hierarchy is always mounted and non-init ns shouldn't be able
to create new v1 hierarchies, so the root dentry should always be
there.
I mispoke before though - it's not the hierarchy's root dentry,
but rather a dentry for a descendent cgroup which will become the
root dentry for the new superblock. We do know that there must be
a css_set with a cgroup. I'm still trying to track down whether
that cgrou's inode's dentry can ever be flushed. I would think
not but am not sure.
Hello, Serge.
On Tue, Dec 01, 2015 at 03:58:53PM -0600, Serge E. Hallyn wrote:
I mispoke before though - it's not the hierarchy's root dentry,
but rather a dentry for a descendent cgroup which will become the
root dentry for the new superblock. We do know that there must be
a css_set with a cgroup. I'm still trying to track down whether
that cgrou's inode's dentry can ever be flushed. I would think
not but am not sure.
Hmmm... I'm not really following. The inode can be flushed and that's
why it needs to be walked down from root. What am I missing here?
Thanks.
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-12-02 16:56:42
On Wed, Dec 02, 2015 at 11:53:12AM -0500, Tejun Heo wrote:
Hello, Serge.
On Tue, Dec 01, 2015 at 03:58:53PM -0600, Serge E. Hallyn wrote:
quoted
I mispoke before though - it's not the hierarchy's root dentry,
but rather a dentry for a descendent cgroup which will become the
root dentry for the new superblock. We do know that there must be
a css_set with a cgroup. I'm still trying to track down whether
that cgrou's inode's dentry can ever be flushed. I would think
not but am not sure.
Hmmm... I'm not really following. The inode can be flushed and that's
why it needs to be walked down from root. What am I missing here?
Can it be flushed when we know that the cgroup is being pinned by
a css_set? (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)
-serge
On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
Can it be flushed when we know that the cgroup is being pinned by
a css_set? (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)
Yeap, it can be flushed. There's no ref coming out of cgroup to the
vfs objects.
Thanks.
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-12-02 17:02:45
On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
quoted
Can it be flushed when we know that the cgroup is being pinned by
a css_set? (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)
Yeap, it can be flushed. There's no ref coming out of cgroup to the
vfs objects.
Ok, thanks. Still seems to me to be more work to actually walk the
path ourselves, but I'll go that route and see what it looks like :)
thanks
-serge
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
On Wed, Dec 02, 2015 at 11:02:39AM -0600, Serge E. Hallyn wrote:
On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
quoted
On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
quoted
Can it be flushed when we know that the cgroup is being pinned by
a css_set? (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)
Yeap, it can be flushed. There's no ref coming out of cgroup to the
vfs objects.
Ok, thanks. Still seems to me to be more work to actually walk the
path ourselves, but I'll go that route and see what it looks like :)
I just dislike having two separate paths instantiating the same
objects and would prefer doing it the same way userland would do if
that isn't too complex but yeah it might turn out to be a lot more
work.
Thanks a lot!
--
tejun
From: Serge E. Hallyn <hidden> Date: 2015-12-03 22:47:11
On Wed, Dec 02, 2015 at 12:05:51PM -0500, Tejun Heo wrote:
On Wed, Dec 02, 2015 at 11:02:39AM -0600, Serge E. Hallyn wrote:
quoted
On Wed, Dec 02, 2015 at 11:58:39AM -0500, Tejun Heo wrote:
quoted
On Wed, Dec 02, 2015 at 10:56:37AM -0600, Serge E. Hallyn wrote:
quoted
Can it be flushed when we know that the cgroup is being pinned by
a css_set? (There's either a task or a cgroup_namespace pinning it
or we wouldn't get here)
Yeap, it can be flushed. There's no ref coming out of cgroup to the
vfs objects.
Ok, thanks. Still seems to me to be more work to actually walk the
path ourselves, but I'll go that route and see what it looks like :)
I just dislike having two separate paths instantiating the same
objects and would prefer doing it the same way userland would do if
that isn't too complex but yeah it might turn out to be a lot more
work.
Thanks a lot!
Here's a patch to make that change. Seems to be working for me. If it
looks ok I can fold it into the prevoius patches and resend the new set.
PATCH 1/1] kernfs_obtain_root: switch to walking the path [fold up]
Signed-off-by: Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
fs/kernfs/mount.c | 80 ++++++++++++++++++++++++++++++++-----------------------
1 file changed, 47 insertions(+), 33 deletions(-)
@@ -74,42 +96,34 @@ struct dentry *kernfs_obtain_root(struct super_block *sb,structkernfs_node*kn){structdentry*dentry;-structinode*inode;+structkernfs_node*knparent=NULL;BUG_ON(sb->s_op!=&kernfs_sops);-/* inode for the given kernfs_node should already exist. */-inode=kernfs_get_inode(sb,kn);-if(!inode){-pr_debug("kernfs: could not get inode for '");-pr_cont_kernfs_path(kn);-pr_cont("'.\n");-returnERR_PTR(-EINVAL);-}--/* instantiate and link root dentry */-dentry=d_obtain_root(inode);-if(!dentry){-pr_debug("kernfs: could not get dentry for '");-pr_cont_kernfs_path(kn);-pr_cont("'.\n");-returnERR_PTR(-ENOMEM);-}--/*-*Ifthisisanewdentry,setitup.Weneedkernfs_mutexbecause-*thismaybecalledbycallersotherthankernfs_fill_super.-*/-mutex_lock(&kernfs_mutex);-if(!dentry->d_fsdata){-kernfs_get(kn);-dentry->d_fsdata=kn;-}else{-WARN_ON(dentry->d_fsdata!=kn);-}-mutex_unlock(&kernfs_mutex);--returndentry;+dentry=dget(sb->s_root);+if(!kn->parent)// this is the root+returndentry;++knparent=find_kn_ancestor_below(kn,NULL);+BUG_ON(!knparent);++do{+structdentry*dtmp;+structkernfs_node*kntmp;++if(kn==knparent)+returndentry;+kntmp=find_kn_ancestor_below(kn,knparent);+BUG_ON(!kntmp);+dtmp=lookup_one_len(kntmp->name,dentry,strlen(kntmp->name));+dput(dentry);+if(IS_ERR(dtmp))+returndtmp;+knparent=kntmp;+dentry=dtmp;+}while(1);++// notreached}staticintkernfs_fill_super(structsuper_block*sb,unsignedlongmagic)
Hello, Serge.
On Thu, Dec 03, 2015 at 04:47:06PM -0600, Serge E. Hallyn wrote:
...
quoted hunk
+ dentry = dget(sb->s_root);+ if (!kn->parent) // this is the root+ return dentry;++ knparent = find_kn_ancestor_below(kn, NULL);+ BUG_ON(!knparent);
Doing WARN_ON() and returning failure is better, I think. Failing ns
mount is an okay failure mode and a lot better than crashing the
system. Also, how about find_next_ancestor() for the name of the
function?
quoted hunk
+ do {+ struct dentry *dtmp;+ struct kernfs_node *kntmp;++ if (kn == knparent)+ return dentry;+ kntmp = find_kn_ancestor_below(kn, knparent);+ BUG_ON(!kntmp);+ dtmp = lookup_one_len(kntmp->name, dentry, strlen(kntmp->name));+ dput(dentry);+ if (IS_ERR(dtmp))+ return dtmp;+ knparent = kntmp;+ dentry = dtmp;+ } while (1);
Other than the nitpicks, looks good to me.
Thanks.
--
tejun
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel