linux-kernel - Re: [RFC PATCH v2 1/3] tracing: Show real address for trace event arguments

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: <20201015090955.5a48b0ba@gandalf.local.home>
Date:   Thu, 15 Oct 2020 09:09:55 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...nel.org>
Subject: Re: [RFC PATCH v2 1/3] tracing: Show real address for trace event
 arguments

On Thu, 15 Oct 2020 18:00:08 +0900
Masami Hiramatsu <mhiramat@...nel.org> wrote:

> +#define STATIC_FMT_BUF_SIZE	128
> +static char static_fmt_buf[STATIC_FMT_BUF_SIZE];
> +
> +const char *trace_event_format(struct trace_iterator *iter, const char *fmt)
> +{
> +	const char *p, *new_fmt;
> +	char *q;
> +
> +	if (WARN_ON_ONCE(!fmt))
> +		return fmt;
> +retry:
> +	p = fmt;
> +	new_fmt = q = iter->fmt;
> +	while (*p) {
> +		if (unlikely(q - new_fmt + 3 > iter->fmt_size)) {
> +			/* expand format buffer if needed */
> +			if (iter->fmt == static_fmt_buf)
> +				return fmt;
> +
> +			iter->fmt_size = iter->fmt_size ? iter->fmt_size * 2
> +							: STATIC_FMT_BUF_SIZE;

Doubling may be too much, as fmt strings are not going to grow
exponentially.

> +			kfree(iter->fmt);
> +			iter->fmt = kmalloc(iter->fmt_size, GFP_KERNEL);

Need to test the return value of kmalloc. But I would use realloc instead,
and continue:

			char *tmp_fmt;
			unsigned int tmp_size;

			tmp_size = iter->fmt_size + STATIC_FMT_BUF_SIZE;

			tmp_fmt = krealloc(iter->fmt, tmp_size, GFP_KERNEL);
			if (!tmp_fmt)
				return fmt;

			q += tmp_fmt - new_fmt;
			new_fmt = iter->fmt = tmp_fmt;
			iter->fmt_size = tmp_size;

> +			goto retry;

Then you don't even need the retry, and just continue processing.

Also, if it fails to allocate, you still have the iter->fmt and fmt_size
intact and correct.

-- Steve

> +		}
> +		*q++ = *p++;
> +		/* Replace %p with %px */
> +		if (p[-1] == '%') {
> +			if (p[0] == '%') {
> +				*q++ = *p++;
> +			} else if (p[0] == 'p' && !isalnum(p[1])) {
> +				*q++ = *p++;
> +				*q++ = 'x';
> +			}
> +		}
> +	}
> +	*q = '\0';
> +
> +	return new_fmt;
> +}
> +