linux-kernel - Re: [PATCH v2] mm: Update mark

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240208120057.01547849@rorschach.local.home>
Date: Thu, 8 Feb 2024 12:00:57 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Carlos Galo <carlosgalo@...gle.com>
Cc: akpm@...ux-foundation.org, surenb@...gle.com, android-mm@...gle.com,
 kernel-team@...roid.com, Masami Hiramatsu <mhiramat@...nel.org>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-mm@...ck.org, Roman Gushchin
 <roman.gushchin@...ux.dev>, Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH v2] mm: Update mark_victim tracepoints fields

On Thu, 11 Jan 2024 21:05:30 +0000
Carlos Galo <carlosgalo@...gle.com> wrote:

> The current implementation of the mark_victim tracepoint provides only
> the process ID (pid) of the victim process. This limitation poses
> challenges for userspace tools that need additional information
> about the OOM victim. The association between pid and the additional
> data may be lost after the kill, making it difficult for userspace to
> correlate the OOM event with the specific process.
> 
> In order to mitigate this limitation, add the following fields:
> 
> - UID
>    In Android each installed application has a unique UID. Including
>    the `uid` assists in correlating OOM events with specific apps.
> 
> - Process Name (comm)
>    Enables identification of the affected process.
> 
> - OOM Score
>    Allows userspace to get additional insights of the relative kill
>    priority of the OOM victim.
> 

From a tracing POV this looks fine to me, but it's up to the mm
maintainers to decide to take it.

> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Signed-off-by: Carlos Galo <carlosgalo@...gle.com>
> ---
> v2: Fixed build error. Added missing comma when printing `__entry->uid`.
> 
>  include/trace/events/oom.h | 19 +++++++++++++++----
>  mm/oom_kill.c              |  6 +++++-
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/include/trace/events/oom.h b/include/trace/events/oom.h
> index 26a11e4a2c36..fb8a5d1b8a0a 100644
> --- a/include/trace/events/oom.h
> +++ b/include/trace/events/oom.h
> @@ -72,19 +72,30 @@ TRACE_EVENT(reclaim_retry_zone,
>  );
>  
>  TRACE_EVENT(mark_victim,
> -	TP_PROTO(int pid),
> +	TP_PROTO(struct task_struct *task, uid_t uid),
>  
> -	TP_ARGS(pid),
> +	TP_ARGS(task, uid),
>  
>  	TP_STRUCT__entry(
>  		__field(int, pid)
> +		__field(uid_t, uid)
> +		__string(comm, task->comm)
> +		__field(short, oom_score_adj)
>  	),
>  
>  	TP_fast_assign(
> -		__entry->pid = pid;
> +		__entry->pid = task->pid;
> +		__entry->uid = uid;
> +		__assign_str(comm, task->comm);
> +		__entry->oom_score_adj = task->signal->oom_score_adj;
>  	),
>  
> -	TP_printk("pid=%d", __entry->pid)
> +	TP_printk("pid=%d uid=%u comm=%s oom_score_adj=%hd",
> +		__entry->pid,
> +		__entry->uid,
> +		__get_str(comm),
> +		__entry->oom_score_adj
> +	)
>  );
>  
>  TRACE_EVENT(wake_reaper,
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 9e6071fde34a..0698c00c5da6 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -44,6 +44,7 @@
>  #include <linux/kthread.h>
>  #include <linux/init.h>
>  #include <linux/mmu_notifier.h>
> +#include <linux/cred.h>
>  
>  #include <asm/tlb.h>
>  #include "internal.h"
> @@ -753,6 +754,7 @@ static inline void queue_oom_reaper(struct task_struct *tsk)
>   */
>  static void mark_oom_victim(struct task_struct *tsk)
>  {
> +	const struct cred *cred;
>  	struct mm_struct *mm = tsk->mm;
>  
>  	WARN_ON(oom_killer_disabled);
> @@ -772,7 +774,9 @@ static void mark_oom_victim(struct task_struct *tsk)
>  	 */
>  	__thaw_task(tsk);
>  	atomic_inc(&oom_victims);
> -	trace_mark_victim(tsk->pid);
> +	cred = get_task_cred(tsk);

get_task_cred() isn't a trivial function and is used only for tracing.
But this isn't a fast path so I guess that is fine.

For tracing:

Reviewed-by: Steven Rostedt (Google) <rostedt@...dmis.org>

-- Steve


> +	trace_mark_victim(tsk, cred->uid.val);
> +	put_cred(cred);
>  }
>  
>  /**
> 
> base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a