Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1585426 > unrolled thread
| Started by | Elena Reshetova <elena.reshetova@intel.com> |
|---|---|
| First post | 2017-02-21 16:40 +0100 |
| Last post | 2017-02-21 17:10 +0100 |
| Articles | 7 on this page of 27 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH 0/9] tools subsystem refcounter conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
[PATCH 9/9] tools: convert thread_map.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
[PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
Re: [PATCH 2/9] tools: convert cpu_map.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
[PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-21 16:50 +0100
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 15:30 +0100
Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 16:50 +0100
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 17:20 +0100
Re: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
RE: [PATCH 1/9] tools: convert cgroup_sel.refcnt from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 14:20 +0100
[PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:40 +0100
Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 21:40 +0100
Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 23:30 +0100
Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-22 23:40 +0100
RE: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 10:20 +0100
Re: [PATCH 3/9] tools: convert comm_str.refcnt from atomic_t to refcount_t Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 14:20 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-21 16:50 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 00:30 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 00:30 +0100
RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 12:40 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 14:00 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 17:30 +0100
RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-24 08:40 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-24 14:40 +0100
Re: [PATCH 0/9] tools subsystem refcounter conversions Peter Zijlstra <peterz@infradead.org> - 2017-02-21 16:50 +0100
RE: [PATCH 0/9] tools subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-21 17:10 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | "Reshetova, Elena" <elena.reshetova@intel.com> |
|---|---|
| Date | 2017-02-23 12:40 +0100 |
| Message-ID | <tdZUB-Ex-5@gated-at.bofh.it> |
| In reply to | #1586549 |
> Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo > escreveu: > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo > escreveu: > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu: > > > > Now when new refcount_t type and API are finally merged > > > > (see include/linux/refcount.h), the following > > > > patches convert various refcounters in the tools susystem from atomic_t > > > > to refcount_t. By doing this we prevent intentional or accidental > > > > underflows or overflows that can led to use-after-free vulnerabilities. > > > > > > Thanks for working on this! I was almost going to jump on doing this > > > myself! > > > > > > I'll try and get this merged ASAP. > > > > So, please take a look at my tmp.perf/refcount branch at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git I took a look on it and it looks good. Just one thing I want to double check with regards to this commit: https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0 And more specifically to this chunk: @@ -937,7 +937,7 @@ munmap(map->base, perf_mmap__mmap_len(map)); map->base = NULL; map->fd = -1; - atomic_set(&map->refcnt, 0); + refcount_set(&map->refcnt, 0); } auxtrace_mmap__munmap(&map->auxtrace_mmap); } So, when the refcount set to zero in this place, what exactly happens to the perf_map object after? I just want to double check that we don't have another hiding reusage case here when refcounter later on is simply incremented vs. set to "2." > > > > There are multiple fixes in it to get it to build and test it, so far, > > with: > > > > perf top -F 15000 -d 0 > > > > while doing kernel builds and tight usleep 1 loops to create lots of > > short lived threads with its map_groups, maps, dsos, etc. > > > > Now running some build tests in some 36 containers with assorted distros > > and cross compilers. > > Tomorrow I'll inject some refcount errors to test this all. Thank you! Best Regards, Elena.
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-23 14:00 +0100 |
| Message-ID | <te1a1-1n9-9@gated-at.bofh.it> |
| In reply to | #1586830 |
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu: > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo > > escreveu: > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo > > escreveu: > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu: > > > > > Now when new refcount_t type and API are finally merged > > > > > (see include/linux/refcount.h), the following > > > > > patches convert various refcounters in the tools susystem from atomic_t > > > > > to refcount_t. By doing this we prevent intentional or accidental > > > > > underflows or overflows that can led to use-after-free vulnerabilities. > > > > > > > > Thanks for working on this! I was almost going to jump on doing this > > > > myself! > > > > > > > > I'll try and get this merged ASAP. > > > > > > So, please take a look at my tmp.perf/refcount branch at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git > > I took a look on it and it looks good. Just one thing I want to double check with regards to this commit: > https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0 > > And more specifically to this chunk: > > @@ -937,7 +937,7 @@ > munmap(map->base, perf_mmap__mmap_len(map)); > map->base = NULL; > map->fd = -1; > - atomic_set(&map->refcnt, 0); > + refcount_set(&map->refcnt, 0); > } > auxtrace_mmap__munmap(&map->auxtrace_mmap); > } > So, when the refcount set to zero in this place, what exactly happens > to the perf_map object after? I just want to double check that we > don't have another hiding reusage case here when refcounter later on > is simply incremented vs. set to "2." So, this looks fishy and I don't recall why that was done that way, this conversion to refcnt_t and the debug associated with it provides a good opportunity for us to try to get that to a more familiar reference counting workflow, I'll take a look at it. - Arnaldo > > > There are multiple fixes in it to get it to build and test it, so far, > > > with: > > > > > > perf top -F 15000 -d 0 > > > > > > while doing kernel builds and tight usleep 1 loops to create lots of > > > short lived threads with its map_groups, maps, dsos, etc. > > > > > > Now running some build tests in some 36 containers with assorted distros > > > and cross compilers. > > > > Tomorrow I'll inject some refcount errors to test this all. > > > Thank you! > > Best Regards, > Elena.
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-23 17:30 +0100 |
| Message-ID | <te4rh-3IU-27@gated-at.bofh.it> |
| In reply to | #1586830 |
Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > Now when new refcount_t type and API are finally merged
> > > > > (see include/linux/refcount.h), the following
> > > > > patches convert various refcounters in the tools susystem from atomic_t
> > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > >
> > > > Thanks for working on this! I was almost going to jump on doing this
> > > > myself!
> > > >
> > > > I'll try and get this merged ASAP.
> > >
> > > So, please take a look at my tmp.perf/refcount branch at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
>
> I took a look on it and it looks good. Just one thing I want to double check with regards to this commit:
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
>
> And more specifically to this chunk:
>
> @@ -937,7 +937,7 @@
> munmap(map->base, perf_mmap__mmap_len(map));
> map->base = NULL;
> map->fd = -1;
> - atomic_set(&map->refcnt, 0);
> + refcount_set(&map->refcnt, 0);
> }
> auxtrace_mmap__munmap(&map->auxtrace_mmap);
> }
>
> So, when the refcount set to zero in this place, what exactly happens to the perf_map object after?
> I just want to double check that we don't have another hiding reusage case here when refcounter later on is simply incremented vs. set to "2."
So, this is an odd use of a reference count, the patch below should help
understand it?
Those perf_mmap objects are created in a batch fashion, it being zero
just means it isn't yet mmaped at all, and we check for that before
using it.
So, it remains a bug to do a dec for a zeroed refcount, and the
refcount_t infrastructure will catch it, which helps tools/.
- Arnaldo
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 564b924fb48a..5a70f08d2518 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -974,8 +974,19 @@ static struct perf_mmap *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
if (!map)
return NULL;
- for (i = 0; i < evlist->nr_mmaps; i++)
+ for (i = 0; i < evlist->nr_mmaps; i++) {
map[i].fd = -1;
+ /*
+ * When the perf_mmap() call is made we grab one refcount, plus
+ * one extra to let perf_evlist__mmap_consume() get the last
+ * events after all real references (perf_mmap__get()) are
+ * dropped.
+ *
+ * Each PERF_EVENT_IOC_SET_OUTPUT points to this mmap and
+ * thus does perf_mmap__get() on it.
+ */
+ refcount_set(&map[i].refcnt, 0);
+ }
return map;
}
@@ -988,6 +999,7 @@ struct mmap_params {
static int perf_mmap__mmap(struct perf_mmap *map,
struct mmap_params *mp, int fd)
{
+ perf_mmap__get(map);
/*
* The last one will be done at perf_evlist__mmap_consume(), so that we
* make sure we don't prevent tools from consuming every last event in
@@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap *map,
* evlist layer can't just drop it when filtering events in
* perf_evlist__filter_pollfd().
*/
- refcount_set(&map->refcnt, 2);
+ perf_mmap__get(map); /* This is not a dup, see the comment above! */
map->prev = 0;
map->mask = mp->mask;
map->base = mmap(NULL, perf_mmap__mmap_len(map), mp->prot,
> > > There are multiple fixes in it to get it to build and test it, so far,
> > > with:
> > >
> > > perf top -F 15000 -d 0
> > >
> > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > short lived threads with its map_groups, maps, dsos, etc.
> > >
> > > Now running some build tests in some 36 containers with assorted distros
> > > and cross compilers.
> >
> > Tomorrow I'll inject some refcount errors to test this all.
>
>
> Thank you!
>
> Best Regards,
> Elena.
[toc] | [prev] | [next] | [standalone]
| From | "Reshetova, Elena" <elena.reshetova@intel.com> |
|---|---|
| Date | 2017-02-24 08:40 +0100 |
| Message-ID | <teiDT-5lj-7@gated-at.bofh.it> |
| In reply to | #1586998 |
> Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > Now when new refcount_t type and API are finally merged
> > > > > > (see include/linux/refcount.h), the following
> > > > > > patches convert various refcounters in the tools susystem from
> atomic_t
> > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > >
> > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > myself!
> > > > >
> > > > > I'll try and get this merged ASAP.
> > > >
> > > > So, please take a look at my tmp.perf/refcount branch at:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> >
> > I took a look on it and it looks good. Just one thing I want to double check
> with regards to this commit:
> >
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> >
> > And more specifically to this chunk:
> >
> > @@ -937,7 +937,7 @@
> > munmap(map->base,
> perf_mmap__mmap_len(map));
> > map->base = NULL;
> > map->fd = -1;
> > - atomic_set(&map->refcnt, 0);
> > + refcount_set(&map->refcnt, 0);
> > }
> > auxtrace_mmap__munmap(&map->auxtrace_mmap);
> > }
> >
> > So, when the refcount set to zero in this place, what exactly happens to the
> perf_map object after?
> > I just want to double check that we don't have another hiding reusage case
> here when refcounter later on is simply incremented vs. set to "2."
>
> So, this is an odd use of a reference count, the patch below should help
> understand it?
Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
>
> Those perf_mmap objects are created in a batch fashion, it being zero
> just means it isn't yet mmaped at all, and we check for that before
> using it.
>
> So, it remains a bug to do a dec for a zeroed refcount, and the
> refcount_t infrastructure will catch it, which helps tools/.
>
> - Arnaldo
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 564b924fb48a..5a70f08d2518 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -974,8 +974,19 @@ static struct perf_mmap
> *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> if (!map)
> return NULL;
>
> - for (i = 0; i < evlist->nr_mmaps; i++)
> + for (i = 0; i < evlist->nr_mmaps; i++) {
> map[i].fd = -1;
> + /*
> + * When the perf_mmap() call is made we grab
> one refcount, plus
> + * one extra to let perf_evlist__mmap_consume()
> get the last
> + * events after all real references
> (perf_mmap__get()) are
> + * dropped.
> + *
> + * Each PERF_EVENT_IOC_SET_OUTPUT points to
> this mmap and
> + * thus does perf_mmap__get() on it.
> + */
> + refcount_set(&map[i].refcnt, 0);
> + }
> return map;
> }
>
> @@ -988,6 +999,7 @@ struct mmap_params {
> static int perf_mmap__mmap(struct perf_mmap *map,
> struct mmap_params *mp, int fd)
> {
> + perf_mmap__get(map);
> /*
> * The last one will be done at perf_evlist__mmap_consume(),
> so that we
> * make sure we don't prevent tools from consuming every last
> event in
> @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap
> *map,
> * evlist layer can't just drop it when filtering events in
> * perf_evlist__filter_pollfd().
> */
> - refcount_set(&map->refcnt, 2);
> + perf_mmap__get(map); /* This is not a dup, see the comment
> above! */
This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero.
So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
Best Regards,
Elena.
> map->prev = 0;
> map->mask = mp->mask;
> map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> >prot,
>
> > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > with:
> > > >
> > > > perf top -F 15000 -d 0
> > > >
> > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > short lived threads with its map_groups, maps, dsos, etc.
> > > >
> > > > Now running some build tests in some 36 containers with assorted distros
> > > > and cross compilers.
> > >
> > > Tomorrow I'll inject some refcount errors to test this all.
> >
> >
> > Thank you!
> >
> > Best Regards,
> > Elena.
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-24 14:40 +0100 |
| Message-ID | <teogi-MF-17@gated-at.bofh.it> |
| In reply to | #1587323 |
Em Fri, Feb 24, 2017 at 07:27:32AM +0000, Reshetova, Elena escreveu:
> > Em Thu, Feb 23, 2017 at 11:39:10AM +0000, Reshetova, Elena escreveu:
> > > > Em Wed, Feb 22, 2017 at 08:23:29PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > Em Tue, Feb 21, 2017 at 12:39:35PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > > > > Em Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova escreveu:
> > > > > > > Now when new refcount_t type and API are finally merged
> > > > > > > (see include/linux/refcount.h), the following
> > > > > > > patches convert various refcounters in the tools susystem from
> > atomic_t
> > > > > > > to refcount_t. By doing this we prevent intentional or accidental
> > > > > > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > > > >
> > > > > > Thanks for working on this! I was almost going to jump on doing this
> > > > > > myself!
> > > > > >
> > > > > > I'll try and get this merged ASAP.
> > > > >
> > > > > So, please take a look at my tmp.perf/refcount branch at:
> > > > >
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
> > >
> > > I took a look on it and it looks good. Just one thing I want to double check
> > with regards to this commit:
> > >
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/acme/linux/+/58d
> > 561002587bf2572f9e6f4d222659e4068fadf%5E%21/#F0
> > >
> > > And more specifically to this chunk:
> > >
> > > @@ -937,7 +937,7 @@
> > > munmap(map->base,
> > perf_mmap__mmap_len(map));
> > > map->base = NULL;
> > > map->fd = -1;
> > > - atomic_set(&map->refcnt, 0);
> > > + refcount_set(&map->refcnt, 0);
> > > }
> > > auxtrace_mmap__munmap(&map->auxtrace_mmap);
> > > }
> > >
> > > So, when the refcount set to zero in this place, what exactly happens to the
> > perf_map object after?
> > > I just want to double check that we don't have another hiding reusage case
> > here when refcounter later on is simply incremented vs. set to "2."
> >
> > So, this is an odd use of a reference count, the patch below should help
> > understand it?
>
> Yes, it helps, indeed, but I think we have an issue here. See below inline in patch.
>
> >
> > Those perf_mmap objects are created in a batch fashion, it being zero
> > just means it isn't yet mmaped at all, and we check for that before
> > using it.
> >
> > So, it remains a bug to do a dec for a zeroed refcount, and the
> > refcount_t infrastructure will catch it, which helps tools/.
> >
> > - Arnaldo
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 564b924fb48a..5a70f08d2518 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -974,8 +974,19 @@ static struct perf_mmap
> > *perf_evlist__alloc_mmap(struct perf_evlist *evlist)
> > if (!map)
> > return NULL;
> >
> > - for (i = 0; i < evlist->nr_mmaps; i++)
> > + for (i = 0; i < evlist->nr_mmaps; i++) {
> > map[i].fd = -1;
> > + /*
> > + * When the perf_mmap() call is made we grab
> > one refcount, plus
> > + * one extra to let perf_evlist__mmap_consume()
> > get the last
> > + * events after all real references
> > (perf_mmap__get()) are
> > + * dropped.
> > + *
> > + * Each PERF_EVENT_IOC_SET_OUTPUT points to
> > this mmap and
> > + * thus does perf_mmap__get() on it.
> > + */
> > + refcount_set(&map[i].refcnt, 0);
> > + }
> > return map;
> > }
> >
> > @@ -988,6 +999,7 @@ struct mmap_params {
> > static int perf_mmap__mmap(struct perf_mmap *map,
> > struct mmap_params *mp, int fd)
> > {
> > + perf_mmap__get(map);
> > /*
> > * The last one will be done at perf_evlist__mmap_consume(),
> > so that we
> > * make sure we don't prevent tools from consuming every last
> > event in
> > @@ -1001,7 +1013,7 @@ static int perf_mmap__mmap(struct perf_mmap
> > *map,
> > * evlist layer can't just drop it when filtering events in
> > * perf_evlist__filter_pollfd().
> > */
> > - refcount_set(&map->refcnt, 2);
> > + perf_mmap__get(map); /* This is not a dup, see the comment
> > above! */
> This change now means that instead of doing refcount_set to 2 when refcount value is "0", you are doing two
> refcount_inc() via perf_mmap__get(), which is not going to do any increments when refcount value is zero.
> So, in that sense having just one refcount_set to 2 with a good explanation why it is needed is better :)
Duh, thanks for pointint it out, will leave the comments, remove the
pair of perf_mmap__get()
> When I asked about it initially, I just wanted to make sure there are no other refcount_inc()s happening on the object when its refcount value is zero. Which looks to be the case apart from the one above.
>
> Best Regards,
> Elena.
>
> > map->prev = 0;
> > map->mask = mp->mask;
> > map->base = mmap(NULL, perf_mmap__mmap_len(map), mp-
> > >prot,
> >
> > > > > There are multiple fixes in it to get it to build and test it, so far,
> > > > > with:
> > > > >
> > > > > perf top -F 15000 -d 0
> > > > >
> > > > > while doing kernel builds and tight usleep 1 loops to create lots of
> > > > > short lived threads with its map_groups, maps, dsos, etc.
> > > > >
> > > > > Now running some build tests in some 36 containers with assorted distros
> > > > > and cross compilers.
> > > >
> > > > Tomorrow I'll inject some refcount errors to test this all.
> > >
> > >
> > > Thank you!
> > >
> > > Best Regards,
> > > Elena.
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-02-21 16:50 +0100 |
| Message-ID | <tdkRs-4uK-33@gated-at.bofh.it> |
| In reply to | #1585426 |
On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote: > Now when new refcount_t type and API are finally merged > (see include/linux/refcount.h), the following > patches convert various refcounters in the tools susystem from atomic_t > to refcount_t. By doing this we prevent intentional or accidental > underflows or overflows that can led to use-after-free vulnerabilities. > > The below patches are fully independent and can be cherry-picked separately. > Since we convert all kernel subsystems in the same fashion, resulting > in about 300 patches, we have to group them for sending at least in some > fashion to be manageable. Please excuse the long cc list. > > Elena Reshetova (9): > tools: convert cgroup_sel.refcnt from atomic_t to refcount_t > tools: convert cpu_map.refcnt from atomic_t to refcount_t > tools: convert comm_str.refcnt from atomic_t to refcount_t > tools: convert dso.refcnt from atomic_t to refcount_t > tools: convert map.refcnt from atomic_t to refcount_t > tools: convert map_groups.refcnt from atomic_t to refcount_t > tools: convert perf_map.refcnt from atomic_t to refcount_t > tools: convert thread.refcnt from atomic_t to refcount_t > tools: convert thread_map.refcnt from atomic_t to refcount_t > > tools/perf/util/cgroup.c | 6 +++--- > tools/perf/util/cgroup.h | 4 ++-- > tools/perf/util/comm.c | 13 +++++-------- > tools/perf/util/cpumap.c | 16 ++++++++-------- > tools/perf/util/cpumap.h | 4 ++-- > tools/perf/util/dso.c | 6 +++--- > tools/perf/util/dso.h | 4 ++-- > tools/perf/util/evlist.c | 18 +++++++++--------- > tools/perf/util/evlist.h | 4 ++-- > tools/perf/util/map.c | 10 +++++----- > tools/perf/util/map.h | 10 +++++----- > tools/perf/util/thread.c | 6 +++--- > tools/perf/util/thread.h | 4 ++-- > tools/perf/util/thread_map.c | 20 ++++++++++---------- > tools/perf/util/thread_map.h | 4 ++-- This is userspace code; did you build this? I see a distinct lack of adding refcount.h to the userspace headers.
[toc] | [prev] | [next] | [standalone]
| From | "Reshetova, Elena" <elena.reshetova@intel.com> |
|---|---|
| Date | 2017-02-21 17:10 +0100 |
| Message-ID | <tdlaN-4Vr-7@gated-at.bofh.it> |
| In reply to | #1585451 |
> On Tue, Feb 21, 2017 at 05:34:54PM +0200, Elena Reshetova wrote: > > Now when new refcount_t type and API are finally merged > > (see include/linux/refcount.h), the following > > patches convert various refcounters in the tools susystem from atomic_t > > to refcount_t. By doing this we prevent intentional or accidental > > underflows or overflows that can led to use-after-free vulnerabilities. > > > > The below patches are fully independent and can be cherry-picked > separately. > > Since we convert all kernel subsystems in the same fashion, resulting > > in about 300 patches, we have to group them for sending at least in some > > fashion to be manageable. Please excuse the long cc list. > > > > Elena Reshetova (9): > > tools: convert cgroup_sel.refcnt from atomic_t to refcount_t > > tools: convert cpu_map.refcnt from atomic_t to refcount_t > > tools: convert comm_str.refcnt from atomic_t to refcount_t > > tools: convert dso.refcnt from atomic_t to refcount_t > > tools: convert map.refcnt from atomic_t to refcount_t > > tools: convert map_groups.refcnt from atomic_t to refcount_t > > tools: convert perf_map.refcnt from atomic_t to refcount_t > > tools: convert thread.refcnt from atomic_t to refcount_t > > tools: convert thread_map.refcnt from atomic_t to refcount_t > > > > tools/perf/util/cgroup.c | 6 +++--- > > tools/perf/util/cgroup.h | 4 ++-- > > tools/perf/util/comm.c | 13 +++++-------- > > tools/perf/util/cpumap.c | 16 ++++++++-------- > > tools/perf/util/cpumap.h | 4 ++-- > > tools/perf/util/dso.c | 6 +++--- > > tools/perf/util/dso.h | 4 ++-- > > tools/perf/util/evlist.c | 18 +++++++++--------- > > tools/perf/util/evlist.h | 4 ++-- > > tools/perf/util/map.c | 10 +++++----- > > tools/perf/util/map.h | 10 +++++----- > > tools/perf/util/thread.c | 6 +++--- > > tools/perf/util/thread.h | 4 ++-- > > tools/perf/util/thread_map.c | 20 ++++++++++---------- > > tools/perf/util/thread_map.h | 4 ++-- > > This is userspace code; did you build this? I see a distinct lack of > adding refcount.h to the userspace headers. Oh, damn, indeed... We were approaching this in the whole kernel tree pile in the same way. I will fix, rebuild and resend. Sorry about this!
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web