| 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: <20150520153449.GF29162@danjae.kornet>
Date: Thu, 21 May 2015 00:34:49 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Jiri Olsa <jolsa@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 4/4] perf tools: Add dso__data_get/put_fd()
On Wed, May 20, 2015 at 11:33:09AM +0300, Adrian Hunter wrote:
> On 20/05/15 09:34, Namhyung Kim wrote:
> > Using dso__data_fd() in multi-thread environment is not safe since
> > returned fd can be closed and/or reused anytime. So convert it to the
> > dso__data_get/put_fd() pair to protect the access with lock.
>
> This is good, but ideally dso__data_open_lock should be a rwlock.
Agreed. But as far as I can see, it might be a recursive mutex since
it needs to allow to call dso__data_* functions while keeping fd open
(ie. the dso__data_open_lock held).
>
> >
> > The original dso__data_fd() is deprecated and kept only for testing.
>
> Maybe move it into perf/tests/dso-data.c since that seems to be the only caller.
Okay.
>
> >
> > Cc: Adrian Hunter <adrian.hunter@...el.com>
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> > tools/perf/util/dso.c | 44 +++++++++++++++++++++++++++++---------
> > tools/perf/util/dso.h | 9 ++++++--
> > tools/perf/util/unwind-libunwind.c | 38 +++++++++++++++++++-------------
> > 3 files changed, 64 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 21fae6908717..5227e41925c2 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -471,27 +471,49 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
> > }
> >
> > /**
> > - * dso__data_fd - Get dso's data file descriptor
> > + * dso__data_get_fd - Get dso's data file descriptor
> > * @dso: dso object
> > * @machine: machine object
> > *
> > * External interface to find dso's file, open it and
> > - * returns file descriptor.
> > + * returns file descriptor. Should be paired with
> > + * dso__data_put_fd().
> > */
> > -int dso__data_fd(struct dso *dso, struct machine *machine)
> > +int dso__data_get_fd(struct dso *dso, struct machine *machine)
> > {
> > + pthread_mutex_lock(&dso__data_open_lock);
>
> I would check the return on all lock functions and consider failure to be an
> error. i.e.
>
> if (pthread_mutex_lock(&dso__data_open_lock))
> return -1;
Ah, forgot to check the locking operation itself. So do you suggest
that we should check the return value of the locking in every place?
> > +
> > if (dso->data.status == DSO_DATA_STATUS_ERROR)
> > return -1;
>
> The status check can be done before taking the lock.
Right. But I did it this way since I'd like to make sure to grab the
lock unconditionally when calling the get() function. See below.
>
> >
> > - pthread_mutex_lock(&dso__data_open_lock);
> > -
> > if (dso->data.fd < 0)
> > try_to_open_dso(dso, machine);
> >
> > - pthread_mutex_unlock(&dso__data_open_lock);
> > return dso->data.fd;
> > }
> >
> > +void dso__data_put_fd(struct dso *dso __maybe_unused)
> > +{
> > + pthread_mutex_unlock(&dso__data_open_lock);
> > +}
> > +
> > +/**
> > + * dso__data_get_fd - Get dso's data file descriptor
> > + * @dso: dso object
> > + * @machine: machine object
> > + *
> > + * Obsolete interface to find dso's file, open it and
> > + * returns file descriptor. It's not thread-safe in that
> > + * the returned fd may be reused for other file.
> > + */
> > +int dso__data_fd(struct dso *dso, struct machine *machine)
> > +{
> > + int fd = dso__data_get_fd(dso, machine);
> > +
> > + dso__data_put_fd(dso);
> > + return fd;
> > +}
> > +
> > bool dso__data_status_seen(struct dso *dso, enum dso_data_status_seen by)
> > {
> > u32 flag = 1 << by;
> > @@ -1198,12 +1220,14 @@ size_t dso__fprintf(struct dso *dso, enum map_type type, FILE *fp)
> > enum dso_type dso__type(struct dso *dso, struct machine *machine)
> > {
> > int fd;
> > + enum dso_type type = DSO__TYPE_UNKNOWN;
> >
> > - fd = dso__data_fd(dso, machine);
> > - if (fd < 0)
> > - return DSO__TYPE_UNKNOWN;
> > + fd = dso__data_get_fd(dso, machine);
> > + if (fd >= 0)
> > + type = dso__type_fd(fd);
> > + dso__data_put_fd(dso);
> >
> > - return dso__type_fd(fd);
> > + return type;
> > }
> >
> > int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index b26ec3ab1336..de9d98c44ae2 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -240,7 +240,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> >
> > /*
> > * The dso__data_* external interface provides following functions:
> > - * dso__data_fd
> > + * dso__data_fd (obsolete)
> > + * dso__data_get_fd
> > + * dso__data_put_fd
> > * dso__data_close
> > * dso__data_size
> > * dso__data_read_offset
> > @@ -257,8 +259,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> > * The current usage of the dso__data_* interface is as follows:
> > *
> > * Get DSO's fd:
> > - * int fd = dso__data_fd(dso, machine);
> > + * int fd = dso__data_get_fd(dso, machine);
> > * USE 'fd' SOMEHOW
> > + * dso__data_put_fd(dso)
>
> Usually, with paired functions, the second of the pair is not called if the
> first fails. It probably has to be that way anyway if you check the error
> return from the lock function.
Fair enough.
Thank you for review!
Namhyung
>
> > *
> > * Read DSO's data:
> > * n = dso__data_read_offset(dso_0, &machine, 0, buf, BUFSIZE);
> > @@ -278,6 +281,8 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
> > * TODO
> > */
> > int dso__data_fd(struct dso *dso, struct machine *machine);
> > +int dso__data_get_fd(struct dso *dso, struct machine *machine);
> > +void dso__data_put_fd(struct dso *dso);
> > void dso__data_close(struct dso *dso);
> >
> > off_t dso__data_size(struct dso *dso, struct machine *machine);
> > diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> > index 7b09a443a280..6d4ab3251729 100644
> > --- a/tools/perf/util/unwind-libunwind.c
> > +++ b/tools/perf/util/unwind-libunwind.c
> > @@ -269,13 +269,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
> > u64 offset = dso->data.eh_frame_hdr_offset;
> >
> > if (offset == 0) {
> > - fd = dso__data_fd(dso, machine);
> > - if (fd < 0)
> > - return -EINVAL;
> > -
> > - /* Check the .eh_frame section for unwinding info */
> > - offset = elf_section_offset(fd, ".eh_frame_hdr");
> > - dso->data.eh_frame_hdr_offset = offset;
> > + fd = dso__data_get_fd(dso, machine);
> > + if (fd >= 0) {
> > + /* Check the .eh_frame section for unwinding info */
> > + offset = elf_section_offset(fd, ".eh_frame_hdr");
> > + dso->data.eh_frame_hdr_offset = offset;
> > + }
> > + dso__data_put_fd(dso);
> > }
> >
> > if (offset)
> > @@ -294,13 +294,19 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
> > u64 ofs = dso->data.debug_frame_offset;
> >
> > if (ofs == 0) {
> > - fd = dso__data_fd(dso, machine);
> > - if (fd < 0)
> > - return -EINVAL;
> > -
> > - /* Check the .debug_frame section for unwinding info */
> > - ofs = elf_section_offset(fd, ".debug_frame");
> > - dso->data.debug_frame_offset = ofs;
> > + int ret = 0;
> > +
> > + fd = dso__data_get_fd(dso, machine);
> > + if (fd >= 0) {
> > + /* Check the .debug_frame section for unwinding info */
> > + ofs = elf_section_offset(fd, ".debug_frame");
> > + dso->data.debug_frame_offset = ofs;
> > + } else
> > + ret = -EINVAL;
> > +
> > + dso__data_put_fd(dso);
> > + if (ret)
> > + return ret;
> > }
> >
> > *offset = ofs;
> > @@ -353,10 +359,12 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
> > #ifndef NO_LIBUNWIND_DEBUG_FRAME
> > /* Check the .debug_frame section for unwinding info */
> > if (!read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
> > - int fd = dso__data_fd(map->dso, ui->machine);
> > + int fd = dso__data_get_fd(map->dso, ui->machine);
> > int is_exec = elf_is_exec(fd, map->dso->name);
> > unw_word_t base = is_exec ? 0 : map->start;
> >
> > + dso__data_put_fd(map->dso);
> > +
> > memset(&di, 0, sizeof(di));
> > if (dwarf_find_debug_frame(0, &di, ip, base, map->dso->name,
> > map->start, map->end))
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists