netdev - Re: [PATCH 2/3] bpf: Add d

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: <2c93a2c75e55291473370d9805f8dd0484acd5a3.camel@chromium.org>
Date:   Thu, 02 Apr 2020 16:02:55 +0200
From:   Florent Revest <revest@...omium.org>
To:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        Yonghong Song <yhs@...com>, Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>, bgregg@...flix.com,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 2/3] bpf: Add d_path helper

On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> + *	Description
> + *		Return full path for given 'struct path' object, which
> + *		needs to be the kernel BTF 'path' object. The path is
> + *		returned in buffer provided 'buf' of size 'sz'.
> + *
> + *	Return
> + *		length of returned string on success, or a negative
> + *		error in case of failure
> + *

You might want to add that d_path is ambiguous since it can add
" (deleted)" at the end of your path and you don't know whether this is
actually part of the file path or not. :) 

> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> +{
> +	char *p = d_path(path, buf, sz - 1);

I am curious why you'd use sz - 1 here? In my experience, d_path's
output is 0 limited so you shouldn't need to keep an extra byte for
that (if that was the intention here).

> +	int len;
> +
> +	if (IS_ERR(p)) {
> +		len = PTR_ERR(p);
> +	} else {
> +		len = strlen(p);
> +		if (len && p != buf) {
> +			memmove(buf, p, len);

Have you considered returning the offset within buf instead and let the
BPF program do pointer arithmetics to find the beginning of the string?

> +			buf[len] = 0;

If my previous comment about sz - 1 is true, then this wouldn't be
necessary, you could just use memmove with len + 1.

> +		}
> +	}
> +
> +	return len;
> +}