bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
@ 2019-07-08 15:11 Matt Hart
  2019-07-08 17:58 ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Matt Hart @ 2019-07-08 15:11 UTC (permalink / raw)
  To: andriin; +Cc: andrii.nakryiko, ast, bpf, daniel, kernel-team, netdev, sdf

Hi all,

I bisected a perf build error on ARMv7 to this patch:
libbpf.c: In function ‘perf_event_open_probe’:
libbpf.c:4112:17: error: cast from pointer to integer of different
size [-Werror=pointer-to-int-cast]
  attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
                 ^

Is this a known issue?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-07-08 15:11 [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API Matt Hart
@ 2019-07-08 17:58 ` Andrii Nakryiko
  2019-07-08 19:27   ` Matt Hart
  0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2019-07-08 17:58 UTC (permalink / raw)
  To: Matt Hart
  Cc: Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann,
	Kernel Team, Networking, Stanislav Fomichev

On Mon, Jul 8, 2019 at 8:11 AM Matt Hart <matthew.hart@linaro.org> wrote:
>
> Hi all,
>
> I bisected a perf build error on ARMv7 to this patch:
> libbpf.c: In function ‘perf_event_open_probe’:
> libbpf.c:4112:17: error: cast from pointer to integer of different
> size [-Werror=pointer-to-int-cast]
>   attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
>                  ^
>
> Is this a known issue?

No, thanks for reporting!

It should be

attr.config1 = (uint64_t)(uintptr_t)(void *)name;

to avoid warning on 32-bit architectures.

I'll post a fix later today, but if you could verify this fixes
warning for you, I'd really appreciate that! Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-07-08 17:58 ` Andrii Nakryiko
@ 2019-07-08 19:27   ` Matt Hart
  2019-07-08 19:55     ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Matt Hart @ 2019-07-08 19:27 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann,
	Kernel Team, Networking, Stanislav Fomichev

On Mon, 8 Jul 2019 at 18:58, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 8:11 AM Matt Hart <matthew.hart@linaro.org> wrote:
> >
> > Hi all,
> >
> > I bisected a perf build error on ARMv7 to this patch:
> > libbpf.c: In function ‘perf_event_open_probe’:
> > libbpf.c:4112:17: error: cast from pointer to integer of different
> > size [-Werror=pointer-to-int-cast]
> >   attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> >                  ^
> >
> > Is this a known issue?
>
> No, thanks for reporting!
>
> It should be
>
> attr.config1 = (uint64_t)(uintptr_t)(void *)name;
>
> to avoid warning on 32-bit architectures.

Tested with manual change and can confirm perf now builds without errors.

>
> I'll post a fix later today, but if you could verify this fixes
> warning for you, I'd really appreciate that! Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-07-08 19:27   ` Matt Hart
@ 2019-07-08 19:55     ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-07-08 19:55 UTC (permalink / raw)
  To: Matt Hart
  Cc: Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann,
	Kernel Team, Networking, Stanislav Fomichev

On Mon, Jul 8, 2019 at 12:27 PM Matt Hart <matthew.hart@linaro.org> wrote:
>
> On Mon, 8 Jul 2019 at 18:58, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 8:11 AM Matt Hart <matthew.hart@linaro.org> wrote:
> > >
> > > Hi all,
> > >
> > > I bisected a perf build error on ARMv7 to this patch:
> > > libbpf.c: In function ‘perf_event_open_probe’:
> > > libbpf.c:4112:17: error: cast from pointer to integer of different
> > > size [-Werror=pointer-to-int-cast]
> > >   attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> > >                  ^
> > >
> > > Is this a known issue?
> >
> > No, thanks for reporting!
> >
> > It should be
> >
> > attr.config1 = (uint64_t)(uintptr_t)(void *)name;
> >
> > to avoid warning on 32-bit architectures.
>
> Tested with manual change and can confirm perf now builds without errors.

Thanks for testing!

I'll add Tested-by: Matt Hart <matthew.hart@linaro.org> when posting a fix.

>
> >
> > I'll post a fix later today, but if you could verify this fixes
> > warning for you, I'd really appreciate that! Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-06-28 20:09       ` Song Liu
@ 2019-06-28 20:34         ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-06-28 20:34 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
	bpf, Stanislav Fomichev, Kernel Team

On Fri, Jun 28, 2019 at 1:09 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Fri, Jun 28, 2019 at 12:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@gmail.com> wrote:
> > >
> > > On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.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@fb.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.

Ok, will switch to fscanf.

>
> >
> > >
> > > > +
> > > > +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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-06-28 19:59     ` Andrii Nakryiko
@ 2019-06-28 20:09       ` Song Liu
  2019-06-28 20:34         ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2019-06-28 20:09 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
	bpf, Stanislav Fomichev, Kernel Team

On Fri, Jun 28, 2019 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >
> > On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.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@fb.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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-06-28 19:46   ` Song Liu
@ 2019-06-28 19:59     ` Andrii Nakryiko
  2019-06-28 20:09       ` Song Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2019-06-28 19:59 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
	bpf, Stanislav Fomichev, Kernel Team

On Fri, Jun 28, 2019 at 12:46 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.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@fb.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.

>
> > +
> > +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 :)

>
> > +               return err;
> > +       }
> > +       return pfd;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> > +                                           bool retprobe,
> > +                                           const char *func_name)
> > +{
> > +       char errmsg[STRERR_BUFSIZE];
> > +       struct bpf_link *link;
> > +       int pfd, err;
> > +
> > +       pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> > +                                   0 /* offset */, -1 /* pid */);
> > +       if (pfd < 0) {
> > +               pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> > +                          bpf_program__title(prog, false),
> > +                          retprobe ? "kretprobe" : "kprobe", func_name,
> > +                          libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > +               return ERR_PTR(pfd);
> > +       }
> > +       link = bpf_program__attach_perf_event(prog, pfd);
> > +       if (IS_ERR(link)) {
> > +               close(pfd);
> > +               err = PTR_ERR(link);
> > +               pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> > +                          bpf_program__title(prog, false),
> > +                          retprobe ? "kretprobe" : "kprobe", func_name,
> > +                          libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +               return link;
> > +       }
> > +       return link;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> > +                                           bool retprobe, pid_t pid,
> > +                                           const char *binary_path,
> > +                                           size_t func_offset)
> > +{
> > +       char errmsg[STRERR_BUFSIZE];
> > +       struct bpf_link *link;
> > +       int pfd, err;
> > +
> > +       pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> > +                                   binary_path, func_offset, pid);
> > +       if (pfd < 0) {
> > +               pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> > +                          bpf_program__title(prog, false),
> > +                          retprobe ? "uretprobe" : "uprobe",
> > +                          binary_path, func_offset,
> > +                          libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > +               return ERR_PTR(pfd);
> > +       }
> > +       link = bpf_program__attach_perf_event(prog, pfd);
> > +       if (IS_ERR(link)) {
> > +               close(pfd);
> > +               err = PTR_ERR(link);
> > +               pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> > +                          bpf_program__title(prog, false),
> > +                          retprobe ? "uretprobe" : "uprobe",
> > +                          binary_path, func_offset,
> > +                          libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +               return link;
> > +       }
> > +       return link;
> > +}
> > +
> >  enum bpf_perf_event_ret
> >  bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> >                            void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 1bf66c4a9330..bd767cc11967 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
> >
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> > +                          const char *func_name);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> > +                          pid_t pid, const char *binary_path,
> > +                          size_t func_offset);
> >
> >  struct bpf_insn;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 756f5aa802e9..57a40fb60718 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
> >         global:
> >                 bpf_link__destroy;
> >                 bpf_object__load_xattr;
> > +               bpf_program__attach_kprobe;
> >                 bpf_program__attach_perf_event;
> > +               bpf_program__attach_uprobe;
> >                 btf_dump__dump_type;
> >                 btf_dump__free;
> >                 btf_dump__new;
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-06-28  5:52 ` [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API Andrii Nakryiko
@ 2019-06-28 19:46   ` Song Liu
  2019-06-28 19:59     ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2019-06-28 19:46 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Networking,
	bpf, Stanislav Fomichev, Kernel Team

On Thu, Jun 27, 2019 at 10:53 PM Andrii Nakryiko <andriin@fb.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@fb.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?

> +
> +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.

> +               return err;
> +       }
> +       return pfd;
> +}
> +
> +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> +                                           bool retprobe,
> +                                           const char *func_name)
> +{
> +       char errmsg[STRERR_BUFSIZE];
> +       struct bpf_link *link;
> +       int pfd, err;
> +
> +       pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> +                                   0 /* offset */, -1 /* pid */);
> +       if (pfd < 0) {
> +               pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> +                          bpf_program__title(prog, false),
> +                          retprobe ? "kretprobe" : "kprobe", func_name,
> +                          libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> +               return ERR_PTR(pfd);
> +       }
> +       link = bpf_program__attach_perf_event(prog, pfd);
> +       if (IS_ERR(link)) {
> +               close(pfd);
> +               err = PTR_ERR(link);
> +               pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> +                          bpf_program__title(prog, false),
> +                          retprobe ? "kretprobe" : "kprobe", func_name,
> +                          libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> +               return link;
> +       }
> +       return link;
> +}
> +
> +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> +                                           bool retprobe, pid_t pid,
> +                                           const char *binary_path,
> +                                           size_t func_offset)
> +{
> +       char errmsg[STRERR_BUFSIZE];
> +       struct bpf_link *link;
> +       int pfd, err;
> +
> +       pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> +                                   binary_path, func_offset, pid);
> +       if (pfd < 0) {
> +               pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> +                          bpf_program__title(prog, false),
> +                          retprobe ? "uretprobe" : "uprobe",
> +                          binary_path, func_offset,
> +                          libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> +               return ERR_PTR(pfd);
> +       }
> +       link = bpf_program__attach_perf_event(prog, pfd);
> +       if (IS_ERR(link)) {
> +               close(pfd);
> +               err = PTR_ERR(link);
> +               pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> +                          bpf_program__title(prog, false),
> +                          retprobe ? "uretprobe" : "uprobe",
> +                          binary_path, func_offset,
> +                          libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> +               return link;
> +       }
> +       return link;
> +}
> +
>  enum bpf_perf_event_ret
>  bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
>                            void **copy_mem, size_t *copy_size,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1bf66c4a9330..bd767cc11967 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
>
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> +                          const char *func_name);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> +                          pid_t pid, const char *binary_path,
> +                          size_t func_offset);
>
>  struct bpf_insn;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 756f5aa802e9..57a40fb60718 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
>         global:
>                 bpf_link__destroy;
>                 bpf_object__load_xattr;
> +               bpf_program__attach_kprobe;
>                 bpf_program__attach_perf_event;
> +               bpf_program__attach_uprobe;
>                 btf_dump__dump_type;
>                 btf_dump__free;
>                 btf_dump__new;
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
  2019-06-28  5:52 [PATCH v3 bpf-next 0/9] libbpf: add bpf_link and tracing attach APIs Andrii Nakryiko
@ 2019-06-28  5:52 ` Andrii Nakryiko
  2019-06-28 19:46   ` Song Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2019-06-28  5:52 UTC (permalink / raw)
  To: andrii.nakryiko, ast, daniel, netdev, bpf, sdf, kernel-team
  Cc: Andrii Nakryiko

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@fb.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);
+}
+
+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)));
+		return err;
+	}
+	return pfd;
+}
+
+struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
+					    bool retprobe,
+					    const char *func_name)
+{
+	char errmsg[STRERR_BUFSIZE];
+	struct bpf_link *link;
+	int pfd, err;
+
+	pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
+				    0 /* offset */, -1 /* pid */);
+	if (pfd < 0) {
+		pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
+			   bpf_program__title(prog, false),
+			   retprobe ? "kretprobe" : "kprobe", func_name,
+			   libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
+		return ERR_PTR(pfd);
+	}
+	link = bpf_program__attach_perf_event(prog, pfd);
+	if (IS_ERR(link)) {
+		close(pfd);
+		err = PTR_ERR(link);
+		pr_warning("program '%s': failed to attach to %s '%s': %s\n",
+			   bpf_program__title(prog, false),
+			   retprobe ? "kretprobe" : "kprobe", func_name,
+			   libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
+		return link;
+	}
+	return link;
+}
+
+struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
+					    bool retprobe, pid_t pid,
+					    const char *binary_path,
+					    size_t func_offset)
+{
+	char errmsg[STRERR_BUFSIZE];
+	struct bpf_link *link;
+	int pfd, err;
+
+	pfd = perf_event_open_probe(true /* uprobe */, retprobe,
+				    binary_path, func_offset, pid);
+	if (pfd < 0) {
+		pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
+			   bpf_program__title(prog, false),
+			   retprobe ? "uretprobe" : "uprobe",
+			   binary_path, func_offset,
+			   libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
+		return ERR_PTR(pfd);
+	}
+	link = bpf_program__attach_perf_event(prog, pfd);
+	if (IS_ERR(link)) {
+		close(pfd);
+		err = PTR_ERR(link);
+		pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
+			   bpf_program__title(prog, false),
+			   retprobe ? "uretprobe" : "uprobe",
+			   binary_path, func_offset,
+			   libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
+		return link;
+	}
+	return link;
+}
+
 enum bpf_perf_event_ret
 bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
 			   void **copy_mem, size_t *copy_size,
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 1bf66c4a9330..bd767cc11967 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
+			   const char *func_name);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
+			   pid_t pid, const char *binary_path,
+			   size_t func_offset);
 
 struct bpf_insn;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 756f5aa802e9..57a40fb60718 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
 	global:
 		bpf_link__destroy;
 		bpf_object__load_xattr;
+		bpf_program__attach_kprobe;
 		bpf_program__attach_perf_event;
+		bpf_program__attach_uprobe;
 		btf_dump__dump_type;
 		btf_dump__free;
 		btf_dump__new;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-07-08 19:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-08 15:11 [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API Matt Hart
2019-07-08 17:58 ` Andrii Nakryiko
2019-07-08 19:27   ` Matt Hart
2019-07-08 19:55     ` Andrii Nakryiko
  -- strict thread matches above, loose matches on Subject: below --
2019-06-28  5:52 [PATCH v3 bpf-next 0/9] libbpf: add bpf_link and tracing attach APIs Andrii Nakryiko
2019-06-28  5:52 ` [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API Andrii Nakryiko
2019-06-28 19:46   ` Song Liu
2019-06-28 19:59     ` Andrii Nakryiko
2019-06-28 20:09       ` Song Liu
2019-06-28 20:34         ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).