linux-kernel - Re: [PATCH] tracing: Have saved_cmdlines arrays all in one allocation

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]

Date: Mon, 12 Feb 2024 19:13:36 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Tim Chen <tim.c.chen@...ux.intel.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Mark Rutland <mark.rutland@....com>, Vincent Donnefort
 <vdonnefort@...gle.com>, Sven Schnelle <svens@...ux.ibm.com>, Mete Durlu
 <meted@...ux.ibm.com>
Subject: Re: [PATCH] tracing: Have saved_cmdlines arrays all in one
 allocation

On Mon, 12 Feb 2024 15:39:03 -0800
Tim Chen <tim.c.chen@...ux.intel.com> wrote:

> > diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
> > index e4fbcc3bede5..210c74dcd016 100644
> > --- a/kernel/trace/trace_sched_switch.c
> > +++ b/kernel/trace/trace_sched_switch.c
> > @@ -201,7 +201,7 @@ static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
> >  	int order;
> >  
> >  	/* Figure out how much is needed to hold the given number of cmdlines */
> > -	orig_size = sizeof(*s) + val * TASK_COMM_LEN;
> > +	orig_size = sizeof(*s) + val * (TASK_COMM_LEN + sizeof(int));  
> 
> Strictly speaking, *map_cmdline_to_pid is unsigned int so it is more consistent
> to use sizeof(unsigned) in line above.  But I'm nitpicking and I'm fine to
> leave it as is.

I was thinking about making that into a macro as it is used in two places.

/* Holds the size of a cmdline and pid element */
#define SAVED_CMDLINE_MAP_ELEMENT_SIZE(s)		\
	(TASK_COMM_LEN + sizeof((s)->map_cmdline_to_pid[0]))

	orig_size = sizeof(*s) + val * SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);

> 
> >  	order = get_order(orig_size);
> >  	size = 1 << (order + PAGE_SHIFT);
> >  	page = alloc_pages(GFP_KERNEL, order);
> > @@ -212,16 +212,11 @@ static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
> >  	memset(s, 0, sizeof(*s));
> >  
> >  	/* Round up to actual allocation */
> > -	val = (size - sizeof(*s)) / TASK_COMM_LEN;
> > +	val = (size - sizeof(*s)) / (TASK_COMM_LEN + sizeof(int));

	val = (size - sizeof(*s)) / SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);

-- Steve

> >  	s->cmdline_num = val;
> >  
> > -	s->map_cmdline_to_pid = kmalloc_array(val,
> > -					      sizeof(*s->map_cmdline_to_pid),
> > -					      GFP_KERNEL);
> > -	if (!s->map_cmdline_to_pid) {
> > -		free_saved_cmdlines_buffer(s);
> > -		return NULL;
> > -	}
> > +	/* Place map_cmdline_to_pid array right after saved_cmdlines */
> > +	s->map_cmdline_to_pid = (unsigned *)&s->saved_cmdlines[val * TASK_COMM_LEN];
> >  
> >  	s->cmdline_idx = 0;
> >  	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,