[PATCH 0/2] help configure some cgroup limits (v2)

[PATCH 0/2] help configure some cgroup limits (v2)

[Topi Miettinen]

Hello,

There are many basic ways to control processes, including capabilities,
cgroups and resource limits. However, there are far fewer ways to find out
useful values for the limits, except blind trial and error.

This patch series attempts to fix that by giving at least a nice starting
point for configuration of PID and device cgroups.

Thanks to the commenters for the previous version.

-Topi

Topi Miettinen (2):
  cgroup_pids: track highwater mark of pids
  device_cgroup: track and present accessed devices

 kernel/cgroup_pids.c     | 51 ++++++++++++++++++++++++++--
 security/device_cgroup.c | 86 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 117 insertions(+), 20 deletions(-)

-- 
2.8.1

[PATCH 0/2] help configure some cgroup limits (v2)

[Topi Miettinen]

Hello,

There are many basic ways to control processes, including capabilities,
cgroups and resource limits. However, there are far fewer ways to find out
useful values for the limits, except blind trial and error.

This patch series attempts to fix that by giving at least a nice starting
point for configuration of PID and device cgroups.

Thanks to the commenters for the previous version.

-Topi

Topi Miettinen (2):
  cgroup_pids: track highwater mark of pids
  device_cgroup: track and present accessed devices

 kernel/cgroup_pids.c     | 51 ++++++++++++++++++++++++++--
 security/device_cgroup.c | 86 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 117 insertions(+), 20 deletions(-)

-- 
2.8.1

[PATCH 1/2] cgroup_pids: highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

[PATCH 1/2] cgroup_pids: highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

[PATCH 1/2] cgroup_pids: track highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

[PATCH 1/2] cgroup_pids: track highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

[PATCH 2/2] device_cgroup: track and present accessed devices

[Topi Miettinen]

Track what devices are accessed, to be able to configure device cgroup
access lists. The information is available in cgroup FS as file
devices.accessed.

Example case demonstrating how to use the info for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/devices/system.slice/smartd.service/devices.accessed
b 8:0 r
c 1:3 rw
root@debian:~# ls -l /dev/char/1\:3 /dev/block/8\:0
lrwxrwxrwx 1 root root 6 Jul 17 19:24 /dev/block/8:0 -> ../sda
lrwxrwxrwx 1 root root 7 Jul 17 19:24 /dev/char/1:3 -> ../null
root@debian:~# cat /etc/systemd/system/smartd.service.d/local.conf
[Service]
DevicePolicy=closed # implies /dev/null rwm
DeviceAllow=/dev/sda r

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 security/device_cgroup.c | 86 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 03c1652..d40a2b9 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -48,6 +48,7 @@ struct dev_exception_item {
 struct dev_cgroup {
 	struct cgroup_subsys_state css;
 	struct list_head exceptions;
+	struct list_head accessed;
 	enum devcg_behavior behavior;
 };
 
@@ -90,18 +91,15 @@ free_and_exit:
 /*
  * called under devcgroup_mutex
  */
-static int dev_exception_add(struct dev_cgroup *dev_cgroup,
-			     struct dev_exception_item *ex)
+static int dev_list_add(struct list_head *exceptions,
+			struct dev_exception_item *ex)
 {
 	struct dev_exception_item *excopy, *walk;
+	bool found = false;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
-	if (!excopy)
-		return -ENOMEM;
-
-	list_for_each_entry(walk, &dev_cgroup->exceptions, list) {
+	list_for_each_entry(walk, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -110,12 +108,15 @@ static int dev_exception_add(struct dev_cgroup *dev_cgroup,
 			continue;
 
 		walk->access |= ex->access;
-		kfree(excopy);
-		excopy = NULL;
+		found = true;
 	}
 
-	if (excopy != NULL)
-		list_add_tail_rcu(&excopy->list, &dev_cgroup->exceptions);
+	if (!found) {
+		excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
+		if (!excopy)
+			return -ENOMEM;
+		list_add_tail_rcu(&excopy->list, exceptions);
+	}
 	return 0;
 }
 
@@ -155,6 +156,16 @@ static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
 	}
 }
 
+static void dev_accessed_clean(struct dev_cgroup *dev_cgroup)
+{
+	struct dev_exception_item *ex, *tmp;
+
+	list_for_each_entry_safe(ex, tmp, &dev_cgroup->accessed, list) {
+		list_del_rcu(&ex->list);
+		kfree_rcu(ex, rcu);
+	}
+}
+
 /**
  * dev_exception_clean - frees all entries of the exception list
  * @dev_cgroup: dev_cgroup with the exception list to be cleaned
@@ -221,6 +232,7 @@ devcgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 	if (!dev_cgroup)
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->accessed);
 	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 
 	return &dev_cgroup->css;
@@ -231,6 +243,7 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
 	struct dev_cgroup *dev_cgroup = css_to_devcgroup(css);
 
 	__dev_exception_clean(dev_cgroup);
+	dev_accessed_clean(dev_cgroup);
 	kfree(dev_cgroup);
 }
 
@@ -272,9 +285,9 @@ static void set_majmin(char *str, unsigned m)
 		sprintf(str, "%u", m);
 }
 
-static int devcgroup_seq_show(struct seq_file *m, void *v)
+static int devcgroup_seq_show_list(struct seq_file *m, struct dev_cgroup *devcgroup,
+				   struct list_head *exceptions, bool allow)
 {
-	struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
 	struct dev_exception_item *ex;
 	char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
 
@@ -285,14 +298,14 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
 	 * - List the exceptions in case the default policy is to deny
 	 * This way, the file remains as a "whitelist of devices"
 	 */
-	if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
+	if (allow) {
 		set_access(acc, ACC_MASK);
 		set_majmin(maj, ~0);
 		set_majmin(min, ~0);
 		seq_printf(m, "%c %s:%s %s\n", type_to_char(DEV_ALL),
 			   maj, min, acc);
 	} else {
-		list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
+		list_for_each_entry_rcu(ex, exceptions, list) {
 			set_access(acc, ex->access);
 			set_majmin(maj, ex->major);
 			set_majmin(min, ex->minor);
@@ -305,6 +318,36 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
 	return 0;
 }
 
+static int devcgroup_seq_show(struct seq_file *m, void *v)
+{
+	struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
+
+	return devcgroup_seq_show_list(m, devcgroup, &devcgroup->exceptions,
+				       devcgroup->behavior == DEVCG_DEFAULT_ALLOW);
+}
+
+static int devcgroup_seq_show_accessed(struct seq_file *m, void *v)
+{
+	struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
+
+	return devcgroup_seq_show_list(m, devcgroup, &devcgroup->accessed, false);
+}
+
+static void devcgroup_add_accessed(struct dev_cgroup *dev_cgroup, short type,
+				   u32 major, u32 minor, short access)
+{
+	struct dev_exception_item ex;
+
+	ex.type = type;
+	ex.major = major;
+	ex.minor = minor;
+	ex.access = access;
+
+	mutex_lock(&devcgroup_mutex);
+	dev_list_add(&dev_cgroup->accessed, &ex);
+	mutex_unlock(&devcgroup_mutex);
+}
+
 /**
  * match_exception	- iterates the exception list trying to find a complete match
  * @exceptions: list of exceptions
@@ -566,7 +609,7 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 		 */
 		if (devcg_root->behavior == DEVCG_DEFAULT_ALLOW &&
 		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
-			rc = dev_exception_add(devcg, ex);
+			rc = dev_list_add(&devcg->exceptions, ex);
 			if (rc)
 				break;
 		} else {
@@ -736,7 +779,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 
 		if (!parent_has_perm(devcgroup, &ex))
 			return -EPERM;
-		rc = dev_exception_add(devcgroup, &ex);
+		rc = dev_list_add(&devcgroup->exceptions, &ex);
 		break;
 	case DEVCG_DENY:
 		/*
@@ -747,7 +790,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY)
 			dev_exception_rm(devcgroup, &ex);
 		else
-			rc = dev_exception_add(devcgroup, &ex);
+			rc = dev_list_add(&devcgroup->exceptions, &ex);
 
 		if (rc)
 			break;
@@ -788,6 +831,11 @@ static struct cftype dev_cgroup_files[] = {
 		.seq_show = devcgroup_seq_show,
 		.private = DEVCG_LIST,
 	},
+	{
+		.name = "accessed",
+		.seq_show = devcgroup_seq_show_accessed,
+		.private = DEVCG_LIST,
+	},
 	{ }	/* terminate */
 };
 
@@ -830,6 +878,8 @@ static int __devcgroup_check_permission(short type, u32 major, u32 minor,
 	if (!rc)
 		return -EPERM;
 
+	devcgroup_add_accessed(dev_cgroup, type, major, minor, access);
+
 	return 0;
 }
 
-- 
2.8.1

Re: [PATCH 1/2] cgroup_pids: track highwater mark of pids

[Tejun Heo]

Hello,

On Sun, Jul 17, 2016 at 11:03:38PM +0300, Topi Miettinen wrote:
> +static void pids_update_highwater_mark(struct pids_cgroup *p)
> +{
> +	while (1) {
> +		int64_t old_mark, new_mark, cur_mark;
> +
> +		old_mark = atomic64_read(&p->highwater_mark);
> +		new_mark = atomic64_read(&p->counter);
> +		if (old_mark >= new_mark)
> +			return;
> +		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
> +					    new_mark);
> +
> +		/* It's OK if the counter was decreased meanwhile */
> +		if (cur_mark == old_mark &&
> +		    atomic64_read(&p->counter) <= new_mark)
> +			return;
> +	}
> +}

I think it'd be better to make this part of pids_charge() - maybe use
atomic64_add_return() to get the current value and track the maximum?

> @@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
>  		.read_s64 = pids_current_read,
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  	},
> +	{
> +		.name = "highwater_mark",
> +		.read_s64 = pids_highwater_mark_read,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},

This should be an entry in the pids.stats file.  Please also update
the documentation accordingly.

Thanks.

-- 
tejun

Re: [PATCH 1/2] cgroup_pids: track highwater mark of pids

[Tejun Heo]

Hello,

On Sun, Jul 17, 2016 at 11:03:38PM +0300, Topi Miettinen wrote:
> +static void pids_update_highwater_mark(struct pids_cgroup *p)
> +{
> +	while (1) {
> +		int64_t old_mark, new_mark, cur_mark;
> +
> +		old_mark = atomic64_read(&p->highwater_mark);
> +		new_mark = atomic64_read(&p->counter);
> +		if (old_mark >= new_mark)
> +			return;
> +		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
> +					    new_mark);
> +
> +		/* It's OK if the counter was decreased meanwhile */
> +		if (cur_mark == old_mark &&
> +		    atomic64_read(&p->counter) <= new_mark)
> +			return;
> +	}
> +}

I think it'd be better to make this part of pids_charge() - maybe use
atomic64_add_return() to get the current value and track the maximum?

> @@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
>  		.read_s64 = pids_current_read,
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  	},
> +	{
> +		.name = "highwater_mark",
> +		.read_s64 = pids_highwater_mark_read,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},

This should be an entry in the pids.stats file.  Please also update
the documentation accordingly.

Thanks.

-- 
tejun