From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754147AbbCSPva (ORCPT <rfc822;w@1wt.eu>);
	Thu, 19 Mar 2015 11:51:30 -0400
Received: from mail-ig0-f180.google.com ([209.85.213.180]:38457 "EHLO
	mail-ig0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751339AbbCSPv2 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 19 Mar 2015 11:51:28 -0400
Message-ID: <550AF081.1050104@plumgrid.com>
Date: Thu, 19 Mar 2015 08:51:29 -0700
From: Alexei Starovoitov <ast@plumgrid.com>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
        Arnaldo Carvalho de Melo <acme@infradead.org>,
        Jiri Olsa <jolsa@redhat.com>,
        Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        "David S. Miller" <davem@davemloft.net>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, linux-api@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 tip 4/8] tracing: allow BPF programs to call bpf_trace_printk()
References: <1426542584-9406-1-git-send-email-ast@plumgrid.com>	<1426542584-9406-5-git-send-email-ast@plumgrid.com> <20150319112931.14f3569b@gandalf.local.home>
In-Reply-To: <20150319112931.14f3569b@gandalf.local.home>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/19/15 8:29 AM, Steven Rostedt wrote:
>> +	/* check format string for allowed specifiers */
>> +	for (i = 0; i < fmt_size; i++)
>
> Even though there's only a single "if" statement after the "for", it is
> usually considered proper to add the brackets if the next line is
> complex (more than one line). Which it is in this case.

ok.

>> +			} else if (fmt[i] == 'p') {
>> +				mod_l[fmt_cnt] = true;
>> +				fmt_cnt++;
>
> So you also allow pointer conversions like "%pS" and "%pF"?

good catch. it's a bug. We shouldn't allow things like pV, pD, etc
Something like pK and pS may be ok, but pF is not because of arch
dependencies. So instead of analyzing all possibilities. I'll allow
%p only. bpf_trace_printk is debug only anyway.

>> +	return __trace_printk(1/* fake ip will not be printed */, fmt,
>> +			      mod_l[0] ? r3 : (u32) r3,
>> +			      mod_l[1] ? r4 : (u32) r4,
>> +			      mod_l[2] ? r5 : (u32) r5);
>
> Does the above work on 32 bit machines? as "%ld" would be (u32), but
> here you are passing in u64.

another great catch. it wouldn't crash, but would print junk. will fix.