| 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
| ||
|
Message-ID: <CAPhsuW4xbehq0SQdj_GwJcH++AWAqkYPg6GY3h6rSWMHUwBVFw@mail.gmail.com>
Date: Fri, 28 Jun 2019 13:09:05 -0700
From: Song Liu <liu.song.a23@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andriin@...com>, Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Stanislav Fomichev <sdf@...ichev.me>,
Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
On Fri, Jun 28, 2019 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@...il.com> wrote:
> >
> > On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@...com> wrote:
> > >
> > > Add ability to attach to kernel and user probes and retprobes.
> > > Implementation depends on perf event support for kprobes/uprobes.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > > ---
> > > tools/lib/bpf/libbpf.c | 213 +++++++++++++++++++++++++++++++++++++++
> > > tools/lib/bpf/libbpf.h | 7 ++
> > > tools/lib/bpf/libbpf.map | 2 +
> > > 3 files changed, 222 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 606705f878ba..65d2fef41003 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -4016,6 +4016,219 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> > > return (struct bpf_link *)link;
> > > }
> > >
> > > +static int parse_uint(const char *buf)
> > > +{
> > > + int ret;
> > > +
> > > + errno = 0;
> > > + ret = (int)strtol(buf, NULL, 10);
> > > + if (errno) {
> > > + ret = -errno;
> > > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > > + return ret;
> > > + }
> > > + if (ret < 0) {
> > > + pr_debug("failed to parse '%s' as unsigned int\n", buf);
> > > + return -EINVAL;
> > > + }
> > > + return ret;
> > > +}
> > > +
> > > +static int parse_uint_from_file(const char* file)
> > > +{
> > > + char buf[STRERR_BUFSIZE];
> > > + int fd, ret;
> > > +
> > > + fd = open(file, O_RDONLY);
> > > + if (fd < 0) {
> > > + ret = -errno;
> > > + pr_debug("failed to open '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + ret = read(fd, buf, sizeof(buf));
> > > + ret = ret < 0 ? -errno : ret;
> > > + close(fd);
> > > + if (ret < 0) {
> > > + pr_debug("failed to read '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + if (ret == 0 || ret >= sizeof(buf)) {
> > > + buf[sizeof(buf) - 1] = 0;
> > > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > > + return -EINVAL;
> > > + }
> > > + return parse_uint(buf);
> > > +}
> > > +
> > > +static int determine_kprobe_perf_type(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> > > + return parse_uint_from_file(file);
> > > +}
> > > +
> > > +static int determine_uprobe_perf_type(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > > + return parse_uint_from_file(file);
> > > +}
> > > +
> > > +static int parse_config_from_file(const char *file)
> > > +{
> > > + char buf[STRERR_BUFSIZE];
> > > + int fd, ret;
> > > +
> > > + fd = open(file, O_RDONLY);
> > > + if (fd < 0) {
> > > + ret = -errno;
> > > + pr_debug("failed to open '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + ret = read(fd, buf, sizeof(buf));
> > > + ret = ret < 0 ? -errno : ret;
> > > + close(fd);
> > > + if (ret < 0) {
> > > + pr_debug("failed to read '%s': %s\n", file,
> > > + libbpf_strerror_r(ret, buf, sizeof(buf)));
> > > + return ret;
> > > + }
> > > + if (ret == 0 || ret >= sizeof(buf)) {
> > > + buf[sizeof(buf) - 1] = 0;
> > > + pr_debug("unexpected input from '%s': '%s'\n", file, buf);
> > > + return -EINVAL;
> > > + }
> > > + if (strncmp(buf, "config:", 7)) {
> > > + pr_debug("expected 'config:' prefix, found '%s'\n", buf);
> > > + return -EINVAL;
> > > + }
> > > + return parse_uint(buf + 7);
> > > +}
> > > +
> > > +static int determine_kprobe_retprobe_bit(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> > > + return parse_config_from_file(file);
> > > +}
> > > +
> > > +static int determine_uprobe_retprobe_bit(void)
> > > +{
> > > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > > + return parse_config_from_file(file);
> > > +}
> >
> > Can we do the above with fscanf? Would that be easier?
>
> It would be less code, but also less strict semantics. E.g., fscanf
> would happily leave out any garbage after number (e.g., 123blablabla,
> would still parse). Also, from brief googling, fscanf doesn't handle
> overflows well.
>
> So I guess I'd vote for this more verbose, but also more strict
> checking, unless you insist on fscanf.
I don't think we need to worry about kernel giving garbage in sysfs.
Most common error gonna be the file doesn't exist. Error messages
like "Failed to parse <filename>" would be sufficient.
Let's keep it simpler.
>
> >
> > > +
> > > +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];
> > > + 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 (retprobe) {
> > > + int bit = uprobe ? determine_uprobe_retprobe_bit()
> > > + : determine_kprobe_retprobe_bit();
> > > +
> > > + if (bit < 0) {
> > > + pr_warning("failed to determine %s retprobe bit: %s\n",
> > > + uprobe ? "uprobe" : "kprobe",
> > > + libbpf_strerror_r(bit, errmsg,
> > > + sizeof(errmsg)));
> > > + return bit;
> > > + }
> > > + attr.config |= 1 << bit;
> > > + }
> > > + attr.size = sizeof(attr);
> > > + attr.type = type;
> > > + attr.config1 = (uint64_t)(void *)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,
> > > + pid < 0 ? -1 : pid /* pid */,
> > > + pid == -1 ? 0 : -1 /* cpu */,
> > > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > > + if (pfd < 0) {
> > > + err = -errno;
> > > + pr_warning("%s perf_event_open() failed: %s\n",
> > > + uprobe ? "uprobe" : "kprobe",
> > > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> >
> > We have another warning in bpf_program__attach_[k|u]probe(). I guess
> > we can remove this one here.
>
> This points specifically to perf_event_open() failing versus other
> possible failures. Messages in attach_{k,u}probe won't have that, they
> will repeat more generic "failed to attach" message. Believe me, if
> something goes wrong in libbpf, I'd rather have too much logging than
> too little :)
>
Fair enough. Let's be verbose here. :)
Song
Powered by blists - more mailing lists