netdev - Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load

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: <5daa362b4c9b6_68182abd481885b455@john-XPS-13-9370.notmuch>
Date:   Fri, 18 Oct 2019 15:01:15 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>
Cc:     andriin@...com, ast@...nel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [bpf-next PATCH] bpf: libbpf, support older style kprobe load

Daniel Borkmann wrote:
> On Fri, Oct 18, 2019 at 07:54:26AM -0700, John Fastabend wrote:
> > Following ./Documentation/trace/kprobetrace.rst add support for loading
> > kprobes programs on older kernels.
> > 
> > Signed-off-by: John Fastabend <john.fastabend@...il.com>
> > ---
> >  tools/lib/bpf/libbpf.c |   81 +++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 73 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index fcea6988f962..12b3105d112c 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> >  	return parse_uint_from_file(file, "config:%d\n");
> >  }
> >  
> > +static int use_kprobe_debugfs(const char *name,
> > +			      uint64_t offset, int pid, bool retprobe)
> 
> offset & pid unused?

Well pid should be dropped and I'll add support for offset. I've not
being using offset so missed it here.

> 
> > +{
> > +	const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> > +	int fd = open(file, O_WRONLY | O_APPEND, 0);
> > +	char buf[PATH_MAX];
> > +	int err;
> > +
> > +	if (fd < 0) {
> > +		pr_warning("failed open kprobe_events: %s\n",
> > +			   strerror(errno));
> > +		return -errno;
> > +	}
> > +
> > +	snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> > +		 retprobe ? 'r' : 'p', name, name);
> > +
> > +	err = write(fd, buf, strlen(buf));
> > +	close(fd);
> > +	if (err < 0)
> > +		return -errno;
> > +	return 0;
> > +}
> > +
> >  static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> >  				 uint64_t offset, int pid)
> >  {
> >  	struct perf_event_attr attr = {};
> >  	char errmsg[STRERR_BUFSIZE];
> > +	uint64_t config1 = 0;
> >  	int type, pfd, err;
> >  
> >  	type = uprobe ? determine_uprobe_perf_type()
> >  		      : determine_kprobe_perf_type();
> >  	if (type < 0) {
> > -		pr_warning("failed to determine %s perf type: %s\n",
> > -			   uprobe ? "uprobe" : "kprobe",
> > -			   libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > -		return type;
> > +		if (uprobe) {
> > +			pr_warning("failed to determine uprobe perf type %s: %s\n",
> > +				   name,
> > +				   libbpf_strerror_r(type,
> > +						     errmsg, sizeof(errmsg)));
> > +		} else {
> > +			/* If we do not have an event_source/../kprobes then we
> > +			 * can try to use kprobe-base event tracing, for details
> > +			 * see ./Documentation/trace/kprobetrace.rst
> > +			 */
> > +			const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> > +			char c[PATH_MAX];
> > +			int fd, n;
> > +
> > +			snprintf(c, sizeof(c), "%s/%s/id", file, name);
> > +
> > +			err = use_kprobe_debugfs(name, offset, pid, retprobe);
> > +			if (err)
> > +				return err;
> 
> Should we throw a pr_warning() here as well when bailing out?

Sure makes sense.

> 
> > +			type = PERF_TYPE_TRACEPOINT;
> > +			fd = open(c, O_RDONLY, 0);
> > +			if (fd < 0) {
> > +				pr_warning("failed to open tracepoint %s: %s\n",
> > +					   c, strerror(errno));
> > +				return -errno;
> > +			}
> > +			n = read(fd, c, sizeof(c));
> > +			close(fd);
> > +			if (n < 0) {
> > +				pr_warning("failed to read %s: %s\n",
> > +					   c, strerror(errno));
> > +				return -errno;
> > +			}
> > +			c[n] = '\0';
> > +			config1 = strtol(c, NULL, 0);
> > +			attr.size = sizeof(attr);
> > +			attr.type = type;
> > +			attr.config = config1;
> > +			attr.sample_period = 1;
> > +			attr.wakeup_events = 1;
> 
> Is there a reason you set latter two whereas below they are not set,
> does it not default to these?

We can drop this.

> 
> > +		}
> > +	} else {
> > +		config1 = ptr_to_u64(name);
> > +		attr.size = sizeof(attr);
> > +		attr.type = type;
> > +		attr.config1 = config1; /* kprobe_func or uprobe_path */
> > +		attr.config2 = offset;  /* kprobe_addr or probe_offset */
> >  	}
> >  	if (retprobe) {
> >  		int bit = uprobe ? determine_uprobe_retprobe_bit()
> > @@ -5033,10 +5102,6 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> >  		}
> >  		attr.config |= 1 << bit;
> >  	}
> 
> What happens in case of retprobe, don't you (unwantedly) bail out here
> again (even through you've set up the retprobe earlier)?

Will fix as well. Looking to see how it passed on my box here but either
way its cryptic so will clean up.

> 
> > -	attr.size = sizeof(attr);
> > -	attr.type = type;
> > -	attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> > -	attr.config2 = offset;		 /* kprobe_addr or probe_offset */
> >  
> >  	/* pid filter is meaningful only for uprobes */
> >  	pfd = syscall(__NR_perf_event_open, &attr,
> >