diff mbox series

[v3] lib: Do not fail a test if oom score cannot be adjusted.

Message ID 20211223024841.801160-1-liwang@redhat.com
State Accepted
Headers show
Series [v3] lib: Do not fail a test if oom score cannot be adjusted. | expand

Commit Message

Li Wang Dec. 23, 2021, 2:48 a.m. UTC
From: Petr Vorel <pvorel@suse.cz>

Setting value < 0 in /proc/<pid>/oom_score_adj requires CAP_SYS_RESOURCE.
However setting the library process score is a best effort operation,
so let's skip it silently when the user is not privileged to do so.

Fixes: 8a0827766d ("lib: add functions to adjust oom score")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Li Wang <liwang@redhat.com>
---

Notes:
    v2 --> v3
    	* drop the useless CAP_SYS_ADMIN
    	* add some new commments
    CI job: https://github.com/wangli5665/ltp/actions/runs/1613852940

 include/tst_memutils.h | 10 +++++++-
 lib/tst_memutils.c     | 54 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 9 deletions(-)

Comments

Yang Xu \(Fujitsu\) Dec. 23, 2021, 3:03 a.m. UTC | #1
Hi Li
> From: Petr Vorel<pvorel@suse.cz>
>
> Setting value<  0 in/proc/<pid>/oom_score_adj requires CAP_SYS_RESOURCE.
> However setting the library process score is a best effort operation,
> so let's skip it silently when the user is not privileged to do so.
>
> Fixes: 8a0827766d ("lib: add functions to adjust oom score")
> Signed-off-by: Petr Vorel<pvorel@suse.cz>
> Signed-off-by: Cyril Hrubis<chrubis@suse.cz>
> Signed-off-by: Li Wang<liwang@redhat.com>
Now, LGTM,
Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com>

ps: ubuntun:groxy fails because of other thing..., it is another problem.

Best Regards
Yang Xu
> ---
>
> Notes:
>      v2 -->  v3
>      	* drop the useless CAP_SYS_ADMIN
>      	* add some new commments
>      CI job:https://github.com/wangli5665/ltp/actions/runs/1613852940
>
Petr Vorel Dec. 23, 2021, 7:57 a.m. UTC | #2
Hi all,

> Notes:
>     v2 --> v3
>     	* drop the useless CAP_SYS_ADMIN
>     	* add some new commments
Yes, CAP_SYS_ADMIN was redundant, thus I merged it.
Thanks a lot all.

>     CI job: https://github.com/wangli5665/ltp/actions/runs/1613852940

Yes, ubuntu cannot reach repos:

    Ign:1 http://security.ubuntu.com/ubuntu groovy-security InRelease
    Ign:2 http://archive.ubuntu.com/ubuntu groovy InRelease
    Err:3 http://security.ubuntu.com/ubuntu groovy-security Release
      404  Not Found [IP: 91.189.91.39 80]
    Ign:4 http://archive.ubuntu.com/ubuntu groovy-updates InRelease
    Ign:5 http://archive.ubuntu.com/ubuntu groovy-backports InRelease
    Err:6 http://archive.ubuntu.com/ubuntu groovy Release
      404  Not Found [IP: 91.189.88.142 80]
    Err:7 http://archive.ubuntu.com/ubuntu groovy-updates Release
      404  Not Found [IP: 91.189.88.142 80]
    Err:8 http://archive.ubuntu.com/ubuntu groovy-backports Release
      404  Not Found [IP: 91.189.88.142 80]
    Reading package lists...
    E: The repository 'http://security.ubuntu.com/ubuntu groovy-security Release' does not have a Release file.
    E: The repository 'http://archive.ubuntu.com/ubuntu groovy Release' does not have a Release file.
    E: The repository 'http://archive.ubuntu.com/ubuntu groovy-updates Release' does not have a Release file.
    E: The repository 'http://archive.ubuntu.com/ubuntu groovy-backports Release' does not have a Release file.

I'm testing upgrade to impish:

https://github.com/pevik/ltp/actions/runs/1614667613

Kind regards,
Petr
Petr Vorel Dec. 23, 2021, 8:29 a.m. UTC | #3
Hi all,

> I'm testing upgrade to impish:

> https://github.com/pevik/ltp/actions/runs/1614667613

Hm, impish is also broken. Hope it's just temporary issue.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
index 68a6e3771..45dec55bc 100644
--- a/include/tst_memutils.h
+++ b/include/tst_memutils.h
@@ -28,13 +28,21 @@  long long tst_available_mem(void);
 /*
  * Enable OOM protection to prevent process($PID) being killed by OOM Killer.
  *   echo -1000 >/proc/$PID/oom_score_adj
+ *
  * If the pid is 0 which means it will set on current(self) process.
  *
+ * Unless the process has CAP_SYS_RESOURCE this call will be no-op because
+ * setting adj value < 0 requires it.
+ *
+ * CAP_SYS_RESOURCE:
+ *   set /proc/[pid]/oom_score_adj to a value lower than the value last set
+ *   by a process with CAP_SYS_RESOURCE.
+ *
  * Note:
  *  This exported tst_enable_oom_protection function can be used at anywhere
  *  you want to protect, but please remember that if you do enable protection
  *  on a process($PID) that all the children will inherit its score and be
- *  ignored by OOM Killer as well. So that's why tst_disable_oom_protection
+ *  ignored by OOM Killer as well. So that's why tst_disable_oom_protection()
  *  to be used in combination.
  */
 void tst_enable_oom_protection(pid_t pid);
diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
index 4dea30330..4a4974761 100644
--- a/lib/tst_memutils.c
+++ b/lib/tst_memutils.c
@@ -11,6 +11,8 @@ 
 
 #define TST_NO_DEFAULT_MAIN
 #include "tst_test.h"
+#include "tst_capability.h"
+#include "lapi/syscalls.h"
 
 #define BLOCKSIZE (16 * 1024 * 1024)
 
@@ -93,6 +95,41 @@  long long tst_available_mem(void)
 	return mem_available;
 }
 
+static int has_caps(void)
+{
+	struct tst_cap_user_header hdr = {
+		.version = 0x20080522,
+		.pid = tst_syscall(__NR_gettid),
+	};
+
+	struct tst_cap_user_data caps[2];
+
+	if (tst_capget(&hdr, caps))
+		tst_brk(TBROK | TERRNO, "tst_capget()");
+
+	if (caps[0].effective & (1U << CAP_SYS_RESOURCE))
+		return 1;
+
+	return 0;
+}
+
+static int write_score(const char *path, int score)
+{
+	FILE *f;
+
+	f = fopen(path, "w");
+	if (!f)
+		return 1;
+
+	if (fprintf(f, "%d", score) <= 0)
+		return 1;
+
+	if (fclose(f))
+		return 1;
+
+	return 0;
+}
+
 static void set_oom_score_adj(pid_t pid, int value)
 {
 	int val;
@@ -111,17 +148,18 @@  static void set_oom_score_adj(pid_t pid, int value)
 			tst_brk(TBROK, "%s does not exist, please check if PID is valid", score_path);
 	}
 
-	FILE_PRINTF(score_path, "%d", value);
+	if (write_score(score_path, value)) {
+		if (!has_caps())
+			return;
+
+		tst_res(TWARN, "Can't adjust score, even with capabilities!?");
+		return;
+	}
+
 	FILE_SCANF(score_path, "%d", &val);
 
-	if (val != value) {
-		if (value < 0) {
-			tst_res(TWARN, "'%s' cannot be set to %i, are you root?",
-				score_path, value);
-			return;
-		}
+	if (val != value)
 		tst_brk(TBROK, "oom_score_adj = %d, but expect %d.", val, value);
-	}
 }
 
 void tst_enable_oom_protection(pid_t pid)