| Message ID | 1396883078-25320-5-git-send-email-jean.pihet@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 07, 2014 at 05:04:26PM +0200, Jean Pihet wrote: > From: Robert Richter <robert.richter@linaro.org> > > This new function creates a new fd for an event. We need this later to > get a fd from a persistent event. > > Signed-off-by: Robert Richter <robert.richter@linaro.org> > Signed-off-by: Robert Richter <rric@kernel.org> > Signed-off-by: Jean Pihet <jean.pihet@linaro.org> > --- > kernel/events/core.c | 14 ++++++++------ > kernel/events/internal.h | 1 + > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 22ec8f0..9857475 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -4206,6 +4206,11 @@ static const struct file_operations perf_fops = { > .fasync = perf_fasync, > }; > > +int perf_get_fd(struct perf_event *event, int f_flags) > +{ > + return anon_inode_getfd("[perf_event]", &perf_fops, event, f_flags); > +} > + > /* > * Perf event wakeup > * > @@ -7028,7 +7033,6 @@ SYSCALL_DEFINE5(perf_event_open, > struct perf_event *event, *sibling; > struct perf_event_attr attr; > struct perf_event_context *ctx; > - struct file *event_file = NULL; > struct fd group = {NULL, 0}; > struct task_struct *task = NULL; > struct pmu *pmu; > @@ -7189,10 +7193,9 @@ SYSCALL_DEFINE5(perf_event_open, > goto err_context; > } > > - event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, > - f_flags); > - if (IS_ERR(event_file)) { > - err = PTR_ERR(event_file); > + event_fd = perf_get_fd(event, f_flags); > + if (event_fd < 0) { > + err = event_fd; > goto err_context; > } > > @@ -7257,7 +7260,6 @@ SYSCALL_DEFINE5(perf_event_open, > * perf_group_detach(). > */ > fdput(group); > - fd_install(event_fd, event_file); > return event_fd; > > err_context: ISTR there being a good reason we only install the FD at the very last moment. Of course I cannot quite recall it :/ I suspect it had something to do with not being able to fudge the state while its being set-up or somesuch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 22.04.14 16:27:59, Peter Zijlstra wrote: > On Mon, Apr 07, 2014 at 05:04:26PM +0200, Jean Pihet wrote: > > From: Robert Richter <robert.richter@linaro.org> > > > > This new function creates a new fd for an event. We need this later to > > get a fd from a persistent event. > > > > Signed-off-by: Robert Richter <robert.richter@linaro.org> > > Signed-off-by: Robert Richter <rric@kernel.org> > > Signed-off-by: Jean Pihet <jean.pihet@linaro.org> > > --- > > kernel/events/core.c | 14 ++++++++------ > > kernel/events/internal.h | 1 + > > 2 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 22ec8f0..9857475 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -4206,6 +4206,11 @@ static const struct file_operations perf_fops = { > > .fasync = perf_fasync, > > }; > > > > +int perf_get_fd(struct perf_event *event, int f_flags) > > +{ > > + return anon_inode_getfd("[perf_event]", &perf_fops, event, f_flags); > > +} > > + > > /* > > * Perf event wakeup > > * > > @@ -7028,7 +7033,6 @@ SYSCALL_DEFINE5(perf_event_open, > > struct perf_event *event, *sibling; > > struct perf_event_attr attr; > > struct perf_event_context *ctx; > > - struct file *event_file = NULL; > > struct fd group = {NULL, 0}; > > struct task_struct *task = NULL; > > struct pmu *pmu; > > @@ -7189,10 +7193,9 @@ SYSCALL_DEFINE5(perf_event_open, > > goto err_context; > > } > > > > - event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, > > - f_flags); > > - if (IS_ERR(event_file)) { > > - err = PTR_ERR(event_file); > > + event_fd = perf_get_fd(event, f_flags); > > + if (event_fd < 0) { > > + err = event_fd; > > goto err_context; > > } > > > > @@ -7257,7 +7260,6 @@ SYSCALL_DEFINE5(perf_event_open, > > * perf_group_detach(). > > */ > > fdput(group); > > - fd_install(event_fd, event_file); > > return event_fd; > > > > err_context: > > ISTR there being a good reason we only install the FD at the very last > moment. Of course I cannot quite recall it :/ > > I suspect it had something to do with not being able to fudge the state > while its being set-up or somesuch. I guess its this patch from Al you referring to: ea635c6 Fix racy use of anon_inode_getfd() in perf_event.c I think we do not introduce it back due to: a6fa941 perf_event: Switch to internal refcount, fix race with close() which removes the user of event_file. -Robert -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, Apr 25, 2014 at 03:54:13PM +0200, Robert Richter wrote: > I guess its this patch from Al you referring to: > > ea635c6 Fix racy use of anon_inode_getfd() in perf_event.c > > I think we do not introduce it back due to: > > a6fa941 perf_event: Switch to internal refcount, fix race with close() > > which removes the user of event_file. linux-2.6# git show a6fa941 error: short SHA1 a6fa941 is ambiguous. error: short SHA1 a6fa941 is ambiguous. fatal: ambiguous argument 'a6fa941': unknown revision or path not in the working tree. Which is I think why Linus makes us use 12 chars instead of the git default of 8. Of course its also entirely useless of git to not list the full IDs for those it did find to be ambiguous. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, Apr 25, 2014 at 04:43:01PM +0200, Peter Zijlstra wrote: > On Fri, Apr 25, 2014 at 03:54:13PM +0200, Robert Richter wrote: > > I guess its this patch from Al you referring to: > > > > ea635c6 Fix racy use of anon_inode_getfd() in perf_event.c > > > > I think we do not introduce it back due to: > > > > a6fa941 perf_event: Switch to internal refcount, fix race with close() > > > > which removes the user of event_file. > > linux-2.6# git show a6fa941 > error: short SHA1 a6fa941 is ambiguous. > error: short SHA1 a6fa941 is ambiguous. > fatal: ambiguous argument 'a6fa941': unknown revision or path not in the working tree. > > Which is I think why Linus makes us use 12 chars instead of the git > default of 8. > > Of course its also entirely useless of git to not list the full IDs for > those it did find to be ambiguous. ok, so its a6fa941d94b4. But no, I don't think that helps, its still true that the moment you get a fd another thread can immediately close(). That would drop the last ref and free it, meanwhile perf_event_open() is happily poking at it. Now I think you could cure this by adding an extra ref before calling your perf_get_fd() and dropping that extra ref at the end, where we used to have fd_install(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 25.04.14 16:52:05, Peter Zijlstra wrote: > But no, I don't think that helps, its still true that the moment you get > a fd another thread can immediately close(). That would drop the last > ref and free it, meanwhile perf_event_open() is happily poking at it. > > Now I think you could cure this by adding an extra ref before calling > your perf_get_fd() and dropping that extra ref at the end, where we used > to have fd_install(). Yes, right. I have a solution now which increments the event's ref count before creating the file descriptor using try_get_event()/ put_event(). The patch also does not remove get_unused_fd_flags() and the err_fd error handler. Have an update already of a rebase version but still need to test it. Would it be ok to split the patch set and send in a first step only the first 4 patches that refactor the perf mmap code? Thanks, -Robert -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/events/core.c b/kernel/events/core.c index 22ec8f0..9857475 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -4206,6 +4206,11 @@ static const struct file_operations perf_fops = { .fasync = perf_fasync, }; +int perf_get_fd(struct perf_event *event, int f_flags) +{ + return anon_inode_getfd("[perf_event]", &perf_fops, event, f_flags); +} + /* * Perf event wakeup * @@ -7028,7 +7033,6 @@ SYSCALL_DEFINE5(perf_event_open, struct perf_event *event, *sibling; struct perf_event_attr attr; struct perf_event_context *ctx; - struct file *event_file = NULL; struct fd group = {NULL, 0}; struct task_struct *task = NULL; struct pmu *pmu; @@ -7189,10 +7193,9 @@ SYSCALL_DEFINE5(perf_event_open, goto err_context; } - event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, - f_flags); - if (IS_ERR(event_file)) { - err = PTR_ERR(event_file); + event_fd = perf_get_fd(event, f_flags); + if (event_fd < 0) { + err = event_fd; goto err_context; } @@ -7257,7 +7260,6 @@ SYSCALL_DEFINE5(perf_event_open, * perf_group_detach(). */ fdput(group); - fd_install(event_fd, event_file); return event_fd; err_context: diff --git a/kernel/events/internal.h b/kernel/events/internal.h index e9007ff..5f1f92d 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -209,5 +209,6 @@ static inline void put_event(struct perf_event *event) extern int perf_alloc_rb(struct perf_event *event, int nr_pages, int flags); extern void perf_free_rb(struct perf_event *event); +extern int perf_get_fd(struct perf_event *event, int f_flags); #endif /* _KERNEL_EVENTS_INTERNAL_H */