[PATCH 0/4] samples/cgroup: fix issues in the event listener samples

[PATCH 0/4] samples/cgroup: fix issues in the event listener samples

[Song Hu]

The two event-listener samples in samples/cgroup mishandle the
lifetime of the monitored file and poll() errors in several ways.

Patch 1 fixes the poll error guard in memcg_event_listener, which
tests "errno != EAGAIN", an error poll() never reports, so an
interrupted poll (EINTR) from a signal exits the listener instead of
resuming.

Patch 2 makes memcg_event_listener exit cleanly when the monitored
cgroup is removed.  Since eea5d2bb34ba ("kernfs: Send IN_DELETE_SELF
and IN_IGNORED") kernfs delivers both events on rmdir; the sample
warns and keeps polling, so a supervisor still counts it as alive
while the monitored cgroup is gone.  Verified against a real
container lifecycle: the unpatched listener stays alive across
docker rm -f, the patched one exits.

Patch 3 arms the inotify watch before reading the counter baseline in
memcg_event_listener, closing the window in which events were lost.

Patch 4 stops cgroup_event_listener from letting dirname(3) modify
argv[1] in place, which truncated the path reported in every event
line and made the two-listener setup taught by
Documentation/admin-guide/cgroup-v1/memory.rst produce
indistinguishable output.

Patches 1 and 2 were verified against a real container lifecycle
(docker rm -f) in a QEMU/KVM virtual machine; patch 3 runs on every
listener start; patch 4 is compile-tested only.

Song Hu (4):
  samples/cgroup: fix poll error handling in memcg_event_listener
  samples/cgroup: exit cleanly when the monitored memcg is removed
  samples/cgroup: arm the inotify watch before reading the baseline
  samples/cgroup: don't let dirname() clobber the monitored path

 samples/cgroup/cgroup_event_listener.c | 18 +++++++---
 samples/cgroup/memcg_event_listener.c  | 50 +++++++++++++++++++-------
 2 files changed, 51 insertions(+), 17 deletions(-)

-- 
2.53.0

[PATCH 1/4] samples/cgroup: fix poll error handling in memcg_event_listener

[Song Hu]

The poll error guard tests "errno != EAGAIN", an error poll() never
reports.  The one benign failure the loop does hit is an interrupted
poll (EINTR) when a signal arrives; with this guard the listener
exits via err() instead of resuming.

Continue on EINTR and only bail out on real errors.

Signed-off-by: Song Hu <husong@kylinos.cn>
---
 samples/cgroup/memcg_event_listener.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/samples/cgroup/memcg_event_listener.c b/samples/cgroup/memcg_event_listener.c
index 41425edbd88a..ec7c03fa7887 100644
--- a/samples/cgroup/memcg_event_listener.c
+++ b/samples/cgroup/memcg_event_listener.c
@@ -234,8 +234,11 @@ static void monitor_events(struct memcg_events *events)
 
 	for (;;) {
 		ret = poll(fds, ARRAY_SIZE(fds), -1);
-		if (ret < 0 && errno != EAGAIN)
-			err(EXIT_FAILURE, "Can't poll memcg events (%d)", ret);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			err(EXIT_FAILURE, "Can't poll memcg events");
+		}
 
 		if (fds[0].revents & POLLERR)
 			err(EXIT_FAILURE, "Got POLLERR during monitor events");
-- 
2.53.0

[PATCH 2/4] samples/cgroup: exit cleanly when the monitored memcg is removed

[Song Hu]

The handler processes IN_MODIFY only.  Since commit eea5d2bb34ba
("kernfs: Send IN_DELETE_SELF and IN_IGNORED") kernfs delivers both
events when the cgroup is removed, but the sample warns "No IN_MODIFY
event, skip it" and returns to poll() forever: after an rmdir the
listener never notices and keeps polling, while a supervisor still
counts it as alive, and the cleanup_memcg_events() call in main() is
unreachable.  The
accompanying selftest, commit 2de27980e1d4 ("selftests: memcg: Add
tests for IN_DELETE_SELF and IN_IGNORED"), exercises exactly this
two-event sequence.

Handle the removal pair by printing a clear message and exiting, and
subscribe IN_DELETE_SELF explicitly.  monitor_events() returns through
the normal path, which also makes the cleanup call reachable again.

Signed-off-by: Song Hu <husong@kylinos.cn>
---
 samples/cgroup/memcg_event_listener.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/samples/cgroup/memcg_event_listener.c b/samples/cgroup/memcg_event_listener.c
index ec7c03fa7887..996bf6fb117c 100644
--- a/samples/cgroup/memcg_event_listener.c
+++ b/samples/cgroup/memcg_event_listener.c
@@ -199,7 +199,7 @@ static int read_memcg_events(struct memcg_events *events, bool show_diff)
 	return ret;
 }
 
-static void process_memcg_events(struct memcg_events *events,
+static bool process_memcg_events(struct memcg_events *events,
 				 struct inotify_event *event)
 {
 	int ret;
@@ -207,19 +207,27 @@ static void process_memcg_events(struct memcg_events *events,
 	if (events->inotify_wd != event->wd) {
 		warnx("Unknown inotify event %d, should be %d", event->wd,
 		      events->inotify_wd);
-		return;
+		return true;
+	}
+
+	if (event->mask & (IN_DELETE_SELF | IN_IGNORED)) {
+		printf("The monitored file %s was removed, exiting\n",
+		       events->path);
+		return false;
 	}
 
 	printf("Received event in %s:\n", events->path);
 
 	if (!(event->mask & IN_MODIFY)) {
 		warnx("No IN_MODIFY event, skip it");
-		return;
+		return true;
 	}
 
 	ret = read_memcg_events(events, /* show_diff = */true);
 	if (ret)
 		warnx("Can't read memcg events");
+
+	return true;
 }
 
 static void monitor_events(struct memcg_events *events)
@@ -247,6 +255,7 @@ static void monitor_events(struct memcg_events *events)
 			struct inotify_event *event;
 			char buffer[INOTIFY_BUFFER_SIZE];
 			ssize_t length;
+			bool keep_going = true;
 
 			length = read(fds[0].fd, buffer, INOTIFY_BUFFER_SIZE);
 			if (length <= 0)
@@ -254,9 +263,15 @@ static void monitor_events(struct memcg_events *events)
 
 			event = (struct inotify_event *)buffer;
 			while (INOTIFY_EVENT_OK(event, length)) {
-				process_memcg_events(events, event);
+				keep_going = process_memcg_events(events,
+								  event);
+				if (!keep_going)
+					break;
 				event = INOTIFY_EVENT_NEXT(event, length);
 			}
+
+			if (!keep_going)
+				return;
 		}
 	}
 }
@@ -291,7 +306,8 @@ static int initialize_memcg_events(struct memcg_events *events,
 	}
 
 	events->inotify_wd = inotify_add_watch(events->inotify_fd,
-					       events->path, IN_MODIFY);
+					       events->path,
+					       IN_MODIFY | IN_DELETE_SELF);
 	if (events->inotify_wd < 0) {
 		warn("Couldn't add monitor in dir %s", events->path);
 		return -EIO;
-- 
2.53.0

[PATCH 3/4] samples/cgroup: arm the inotify watch before reading the baseline

[Song Hu]

The listener reads the initial counter state first and arms the
inotify watch afterwards, so any event firing in between is lost:
it is already included in the baseline (no diff will ever be reported)
and no watch existed to queue a notification.

Arm the watch first, then read the baseline.

Signed-off-by: Song Hu <husong@kylinos.cn>
---
 samples/cgroup/memcg_event_listener.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/samples/cgroup/memcg_event_listener.c b/samples/cgroup/memcg_event_listener.c
index 996bf6fb117c..c23af43bf6cc 100644
--- a/samples/cgroup/memcg_event_listener.c
+++ b/samples/cgroup/memcg_event_listener.c
@@ -293,12 +293,6 @@ static int initialize_memcg_events(struct memcg_events *events,
 		return ret;
 	}
 
-	ret = read_memcg_events(events, /* show_diff = */false);
-	if (ret) {
-		warnx("Failed to read initial memcg events state (%d)", ret);
-		return ret;
-	}
-
 	events->inotify_fd = inotify_init();
 	if (events->inotify_fd < 0) {
 		warn("Failed to setup new inotify device");
@@ -313,6 +307,17 @@ static int initialize_memcg_events(struct memcg_events *events,
 		return -EIO;
 	}
 
+	/*
+	 * Read the initial state only after the watch is armed, so no
+	 * event can slip through the window between the baseline read and
+	 * the watch creation.
+	 */
+	ret = read_memcg_events(events, /* show_diff = */false);
+	if (ret) {
+		warnx("Failed to read initial memcg events state (%d)", ret);
+		return ret;
+	}
+
 	printf("Initialized MEMCG events with counters:\n");
 	print_memcg_counters(&events->counters);
 
-- 
2.53.0

[PATCH 4/4] samples/cgroup: don't let dirname() clobber the monitored path

[Song Hu]

dirname(3) is allowed to modify its argument in place, and the libc
implementations do: after building event_control_path, argv[1] has
been truncated to the parent directory.  Every subsequent "crossed"
event line therefore reports the cgroup directory instead of the
monitored file, and the two-listener setup taught by
Documentation/admin-guide/cgroup-v1/memory.rst (usage_in_bytes plus
memsw.usage_in_bytes) produces indistinguishable output lines.

Work on a strdup() copy so event reports keep naming the monitored
file.

Fixes: 60433a9d038d ("samples: introduce new samples subdir for cgroup")
Signed-off-by: Song Hu <husong@kylinos.cn>
---
 samples/cgroup/cgroup_event_listener.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/samples/cgroup/cgroup_event_listener.c b/samples/cgroup/cgroup_event_listener.c
index 3d70dc831a76..308cacec80a8 100644
--- a/samples/cgroup/cgroup_event_listener.c
+++ b/samples/cgroup/cgroup_event_listener.c
@@ -26,17 +26,27 @@ int main(int argc, char **argv)
 	int event_control = -1;
 	char event_control_path[PATH_MAX];
 	char line[LINE_MAX];
+	char *path;
 	int ret;
 
 	if (argc != 3)
 		errx(1, "%s", USAGE_STR);
 
-	cfd = open(argv[1], O_RDONLY);
+	/*
+	 * dirname(3) may modify its argument in place; work on a copy so
+	 * argv[1] still refers to the monitored file when events are
+	 * reported.
+	 */
+	path = strdup(argv[1]);
+	if (!path)
+		err(1, "Cannot duplicate %s", argv[1]);
+
+	cfd = open(path, O_RDONLY);
 	if (cfd == -1)
-		err(1, "Cannot open %s", argv[1]);
+		err(1, "Cannot open %s", path);
 
 	ret = snprintf(event_control_path, PATH_MAX, "%s/cgroup.event_control",
-			dirname(argv[1]));
+			dirname(path));
 	if (ret >= PATH_MAX)
 		errx(1, "Path to cgroup.event_control is too long");
 
@@ -76,7 +86,7 @@ int main(int argc, char **argv)
 		if (ret == -1)
 			err(1, "cgroup.event_control is not accessible any more");
 
-		printf("%s %s: crossed\n", argv[1], argv[2]);
+		printf("%s %s: crossed\n", path, argv[2]);
 	}
 
 	return 0;
-- 
2.53.0

Re: [PATCH 3/4] samples/cgroup: arm the inotify watch before reading the baseline

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] samples/cgroup: events before read_memcg_events() are absorbed into baseline
--

commit e11bb86059068b47a263deb2a44878c61af9a6da
Author: Song Hu <husong@kylinos.cn>

samples/cgroup: arm the inotify watch before reading the baseline

This commit moves the initial read_memcg_events() call in
initialize_memcg_events() to occur after inotify_add_watch() so that
events firing during initialization are not missed before the watch is
created.

> diff --git a/samples/cgroup/memcg_event_listener.c b/samples/cgroup/memcg_event_listener.c
> index 996bf6fb117c4..c23af43bf6ccd 100644
> --- a/samples/cgroup/memcg_event_listener.c
> +++ b/samples/cgroup/memcg_event_listener.c
> @@ -293,12 +293,6 @@ static int initialize_memcg_events(struct memcg_events *events,
>  		return ret;
>  	}
>  
> -	ret = read_memcg_events(events, /* show_diff = */false);
> -	if (ret) {
> -		warnx("Failed to read initial memcg events state (%d)", ret);
> -		return ret;
> -	}
> -
>  	events->inotify_fd = inotify_init();
>  	if (events->inotify_fd < 0) {
>  		warn("Failed to setup new inotify device");
> @@ -313,6 +307,17 @@ static int initialize_memcg_events(struct memcg_events *events,
>  		return -EIO;
>  	}
>  
> +	/*
> +	 * Read the initial state only after the watch is armed, so no
> +	 * event can slip through the window between the baseline read and
> +	 * the watch creation.
> +	 */
> +	ret = read_memcg_events(events, /* show_diff = */false);
> +	if (ret) {
> +		warnx("Failed to read initial memcg events state (%d)", ret);
> +		return ret;
> +	}

[Severity: Medium]
If a memcg event fires after inotify_add_watch() arms the watch in
initialize_memcg_events(), but before read_memcg_events(events, false)
reads the initial counters, won't that event be absorbed into the baseline
without its diff ever being reported?

When the event fires in that window, the kernel increments the cumulative
counter in memory.events and queues an IN_MODIFY notification on
events->inotify_fd. Then initialize_memcg_events() calls
read_memcg_events(events, false):

samples/cgroup/memcg_event_listener.c:read_memcg_events() {
    ...
	for (i = 0; i < ARRAY_SIZE(map); ++i) {
		long diff;

		if (*map[i].new > *map[i].old) {
			diff = *map[i].new - *map[i].old;

			if (show_diff)
				printf("*** %ld MEMCG %s event%s, "
				       "change counter %ld => %ld\n",
				       diff, map[i].name,
				       (diff == 1) ? "" : "s",
				       *map[i].old, *map[i].new);

			*map[i].old += diff;
			any_new_events = true;
		}
	}

	if (show_diff && !any_new_events)
		printf("*** No new untracked memcg events available\n");
    ...
}

Because show_diff is false, *map[i].old is updated to the new counter
value without printing the diff. When monitor_events() subsequently enters
poll() -> process_memcg_events() -> read_memcg_events(events, true) to
handle the queued IN_MODIFY notification, *map[i].new > *map[i].old is now
false, so no diff is reported and it instead prints:

  *** No new untracked memcg events available

Also, the commit message states that in the original order (reading the
baseline first and arming the watch afterwards), an event firing in
between "is already included in the baseline (no diff will ever be
reported) and no watch existed to queue a notification". Isn't that
description inverted? In the original order, an event firing after the
baseline read was not included in the baseline (it only lacked an inotify
notification until a subsequent event arrived), whereas with the new order
an event in the window is included in the baseline while queueing a
notification whose diff is already consumed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102122528.1498193-1-husong@kylinos.cn?part=3

Re: [PATCH 4/4] samples/cgroup: don't let dirname() clobber the monitored path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] samples/cgroup: main() prints dirname()-clobbered path instead of argv[1]
--

commit e427c2a6d742e00fa3e83aac4c0262cf23a479ec
Author: Song Hu <husong@kylinos.cn>

samples/cgroup: don't let dirname() clobber the monitored path

This commit duplicates argv[1] with strdup() before passing it to dirname()
in cgroup_event_listener so that dirname() does not modify argv[1] in place.

> diff --git a/samples/cgroup/cgroup_event_listener.c b/samples/cgroup/cgroup_event_listener.c
> index 3d70dc831a766..308cacec80a8b 100644
> --- a/samples/cgroup/cgroup_event_listener.c
> +++ b/samples/cgroup/cgroup_event_listener.c
> @@ -26,17 +26,27 @@ int main(int argc, char **argv)
>  	int event_control = -1;
>  	char event_control_path[PATH_MAX];
>  	char line[LINE_MAX];
> +	char *path;
>  	int ret;
>  
>  	if (argc != 3)
>  		errx(1, "%s", USAGE_STR);
>  
> -	cfd = open(argv[1], O_RDONLY);
> +	/*
> +	 * dirname(3) may modify its argument in place; work on a copy so
> +	 * argv[1] still refers to the monitored file when events are
> +	 * reported.
> +	 */
> +	path = strdup(argv[1]);
> +	if (!path)
> +		err(1, "Cannot duplicate %s", argv[1]);
> +
> +	cfd = open(path, O_RDONLY);
>  	if (cfd == -1)
> -		err(1, "Cannot open %s", argv[1]);
> +		err(1, "Cannot open %s", path);
>  
>  	ret = snprintf(event_control_path, PATH_MAX, "%s/cgroup.event_control",
> -			dirname(argv[1]));
> +			dirname(path));
>  	if (ret >= PATH_MAX)
>  		errx(1, "Path to cgroup.event_control is too long");
>  
> @@ -76,7 +86,7 @@ int main(int argc, char **argv)
>  		if (ret == -1)
>  			err(1, "cgroup.event_control is not accessible any more");
>  
> -		printf("%s %s: crossed\n", argv[1], argv[2]);
> +		printf("%s %s: crossed\n", path, argv[2]);

[Severity: Medium]
Should printf() in main() keep printing argv[1] here instead of path?

dirname(path) modifies the buffer pointed to by path in place while leaving
argv[1] intact, so switching the printf() call from argv[1] to path causes
main() to print the truncated parent directory rather than the full monitored
file path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102122528.1498193-1-husong@kylinos.cn?part=4