netdev - Re: [PATCH bpf-next] tools: bpftool: add a command to dump the trace pipe

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: <25478278-9b6b-3a52-8e46-34b692043b08@netronome.com>
Date:   Wed, 5 Dec 2018 18:15:23 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next] tools: bpftool: add a command to dump the trace
 pipe

2018-12-05 08:50 UTC-0800 ~ Alexei Starovoitov 
<alexei.starovoitov@...il.com>
> On Wed, Dec 05, 2018 at 10:28:24AM +0000, Quentin Monnet wrote:
>> BPF programs can use the bpf_trace_printk() helper to print debug
>> information into the trace pipe. Add a subcommand
>> "bpftool prog tracelog" to simply dump this pipe to the console.
>>
>> This is for a good part copied from iproute2, where the feature is
>> available with "tc exec bpf dbg". Changes include dumping pipe content
>> to stdout instead of stderr and adding JSON support (content is dumped
>> as an array of strings, one per line read from the pipe). This version
>> is dual-licensed, with Daniel's permission.
>>
>> Cc: Daniel Borkmann <daniel@...earbox.net>
>> Suggested-by: Daniel Borkmann <daniel@...earbox.net>
>> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>> ---

>> +static bool find_tracefs_pipe(char *mnt)
>> +{
>> +	static const char * const known_mnts[] = {
>> +		"/sys/kernel/debug/tracing",
>> +		"/sys/kernel/tracing",
>> +		"/tracing",
>> +		"/trace",
> 
> I wonder where this list came from?
> I only knew of 1st one.

I only met the first form too. I took the list from iproute2. I can 
change it in the future if we find out trying all these paths is not 
relevant.

>> +	};
>> +	const char *pipe_name = "/trace_pipe";
>> +	const char *fstype = "tracefs";
>> +	char type[100], format[32];
>> +	const char * const *ptr;
>> +	bool found = false;
>> +	FILE *fp;
>> +
>> +	for (ptr = known_mnts; ptr < known_mnts + ARRAY_SIZE(known_mnts); ptr++)
>> +		if (find_tracefs_mnt_single(TRACEFS_MAGIC, mnt, *ptr))
>> +			goto exit_found;
>> +
>> +	fp = fopen("/proc/mounts", "r");
>> +	if (!fp)
>> +		return false;
>> +
>> +	/* Allow room for NULL terminating byte and pipe file name */
>> +	snprintf(format, sizeof(format), "%%*s %%%zds %%99s %%*s %%*d %%*d\\n",
>> +		 PATH_MAX - strlen(pipe_name) - 1);
> 
> before scanning trace_pipe could you add a check that trace_options are compatible?
> Otherwise there will be a lot of garbage printed.
> afaik default is rarely changed, so the patch is ok as-is.
> The followup some time in the future would be perfect.

Sure. What do you mean exactly by compatible options? I can check that 
"trace_printk" is set, is there any other option that would be relevant?

Thanks,
Quentin