cgroup: bug fixes for eventfd
cover cgroup: bug fixes for eventfd
There're three bugs.
- If thread A is removing a cgroup, while thread B is closing an eventfd, the two threads might free the same cgroup event and thus crash the kernel.
This is fixed by patch #1 and patch #2.
- If there're multiple threads are blocking in read() on the same eventfd, and someone removes the cgroup, only one thread will be notified and unblocked, and others won't be unblocked until those threads are killed.
- If thread A is removing a cgroup, while thread B is registering a cgroup event and then read the eventfd, it might block until the thread is killed.
These two are fixed by patch #3.
0001-eventfd-Introduce-eventfd_signal_hangup.patch 0002-cgroup-fix-cgroup_rmdir-vs-close-eventfd-race.patch 0003-eventfd-make-operations-on-eventfd-return-EIDRM-if-i.patch 0004-cgroup-adapt-to-the-new-way-of-detecting-cgroup-remo.patch
--
fs/eventfd.c | 30 ++++++++++++++++++++++++++++++ include/linux/eventfd.h | 5 +++++ kernel/cgroup.c | 30 ++++++++++++++++++------------ tools/cgroup/cgroup_event_listener.c | 12 +++++------- 4 files changed, 58 insertions(+), 19 deletions(-)
PATCH 1/4 eventfd: introduce eventfd_signal_hangup()
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
--- fs/eventfd.c | 11 +++++++++++ include/linux/eventfd.h | 5 +++++ 2 files changed, 16 insertions(+)
fs/eventfd.c +11 -0
@@ -67,6 +67,17 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n) }EXPORT_SYMBOL_GPL (eventfd_signal); +/** +* eventfd_signal_hangup - Notify that this eventfd is hung up. +* @ctx: [in] Pointer to the eventfd context. +* +* Issue a POLLHUP wakeup. +*/ +void eventfd_signal_hangup (struct eventfd_ctx *ctx) +{ +wake_up_poll (&ctx->wqh,POLLHUP ); +} +static void eventfd_free_ctx (struct eventfd_ctx *ctx) {kfree (ctx);
include/linux/eventfd.h +5 -0
@@ -35,6 +35,7 @@ struct file *eventfd_fget(int fd);struct eventfd_ctx *eventfd_ctx_fdget (int fd);struct eventfd_ctx *eventfd_ctx_fileget (struct file *file);__u64 eventfd_signal (struct eventfd_ctx *ctx,__u64 n); +void eventfd_signal_hangup (struct eventfd_ctx *ctx);ssize_t eventfd_ctx_read (struct eventfd_ctx *ctx,int no_wait,__u64 *cnt);int eventfd_ctx_remove_wait_queue (struct eventfd_ctx *ctx,wait_queue_t *wait,__u64 *cnt);
@@ -60,6 +61,10 @@ static inline int eventfd_signal(struct eventfd_ctx *ctx, int n)return -ENOSYS ; } +static inline void eventfd_signal_hangup (struct eventfd_ctx *ctx) +{ +} +static inline void eventfd_ctx_put (struct eventfd_ctx *ctx) {
Signature · 1 line
1.8.0.2
PATCH 2/4 cgroup: fix cgroup_rmdir() vs close(eventfd) race
In reply to Li Zefancommit 205a872bd6f9a9a09ef035ef1e90185a8245cc58 ("cgroup: fix lockdep warning for event_control") sovled a deadlock by introducing a new bug.
We can't access @event without event_list_lock, otherwise we'll race with cgroup_event_wake() called when closing the eventfd, and then both threads will try to free the same @event.
CPU0 CPU1 --------------------------- ----------------------------- cgroup_rmdir() close(eventfd)
list_for_each_entry() cgroup_event_wake()
list_del(event)
list_del(event)
cgroup_event_remove(event)
cgroup_event_remove(event)
To fix this, use the new eventfd_signal_hangup() to notify userspace cgroup is removed.
--- kernel/cgroup.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
kernel/cgroup.c +19 -12
@@ -4302,9 +4302,9 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)struct dentry *d = cgrp->dentry;struct cgroup *parent = cgrp->parent;DEFINE_WAIT (wait); -struct cgroup_event *event, *tmp; +struct eventfd_ctx *ctx; +struct cgroup_event *event;struct cgroup_subsys *ss; -LIST_HEAD (tmp_list);lockdep_assert_held (&d->d_inode->i_mutex);lockdep_assert_held (&cgroup_mutex);
@@ -4359,20 +4359,27 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)/* * Unregister events and notify userspace. * Notify userspace about cgroup removing only after rmdir of cgroup -* directory to avoid race between userspace and kernelspace. Use -* a temporary list to avoid a deadlock with cgroup_event_wake(). Since +* directory to avoid race between userspace and kernelspace. Since * cgroup_event_wake() is called with the wait queue head locked, -* remove_wait_queue() cannot be called while holding event_list_lock. +* eventfd_signal() cannot be called while holding event_list_lock. */ spin_lock (&cgrp->event_list_lock); -list_splice_init (&cgrp->event_list, &tmp_list); -spin_unlock (&cgrp->event_list_lock); -list_for_each_entry_safe (event, tmp, &tmp_list, list) { -list_del_init (&event->list); -remove_wait_queue (event->wqh, &event->wait); -eventfd_signal (event->eventfd,1 ); -schedule_work (&event->remove); +while (true) { +if (list_empty (&cgrp->event_list)) +break ; + + event =list_first_entry (&cgrp->event_list, + struct cgroup_event, list); + ctx =eventfd_ctx_get (event->eventfd); +spin_unlock (&cgrp->event_list_lock); + +eventfd_signal (ctx,1 ); +eventfd_signal_hangup (ctx); +eventfd_ctx_put (ctx); + +spin_lock (&cgrp->event_list_lock); } +spin_unlock (&cgrp->event_list_lock);return 0 ; }
Signature · 1 line
1.8.0.2
PATCH 3/4 eventfd: make operations on eventfd return -EIDRM if it's hung up
In reply to Li ZefanCurrently when a cgroup is removed, it calls eventfd_signal() for each registered cgroup event, so userspace can be notified and blocked reads can be unblocked.
The problem is, if we have multiple threads blocked on the same eventfd, only one of them will be unlocked.
This patch makes sure all operations on the same eventfd can be unbocked.
There's another problem, a new cgroup event can be registered while we are removing the cgroup, and then reading the eventfd will be blocked until the thread is killed. This patch also fixes this issue.
--- fs/eventfd.c | 23 +++++++++++++++++++++-- kernel/cgroup.c | 1 - 2 files changed, 21 insertions(+), 3 deletions(-)
fs/eventfd.c +21 -2
@@ -35,6 +35,7 @@ struct eventfd_ctx {*/ __u64 count;unsigned int flags; +bool hung_up; };/**
@@ -71,11 +72,19 @@ EXPORT_SYMBOL_GPL(eventfd_signal);* eventfd_signal_hangup - Notify that this eventfd is hung up. * @ctx: [in] Pointer to the eventfd context. * -* Issue a POLLHUP wakeup. +* Issue a POLLHUP wakeup. All current blocked reads, writes and polls on +* this eventfd will return with -EIDRM. Future operations on it will also +* return with -EDIRM. */ void eventfd_signal_hangup (struct eventfd_ctx *ctx) { -wake_up_poll (&ctx->wqh,POLLHUP ); +unsigned long flags; + +spin_lock_irqsave (&ctx->wqh.lock, flags); + ctx->hung_up = true; +if (waitqueue_active (&ctx->wqh)) +wake_up_locked_poll (&ctx->wqh,POLLHUP ); +spin_unlock_irqrestore (&ctx->wqh.lock, flags); }static void eventfd_free_ctx (struct eventfd_ctx *ctx)
@@ -140,6 +149,8 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait) events |=POLLERR ;if (ULLONG_MAX -1 > ctx->count) events |=POLLOUT ; +if (ctx->hung_up) + events |=POLLHUP ;spin_unlock_irqrestore (&ctx->wqh.lock, flags);return events;
@@ -208,6 +219,10 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt)__add_wait_queue (&ctx->wqh, &wait);for (;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ; +break ; + }if (ctx->count >0 ) { res =0 ;break ;
@@ -272,6 +287,10 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c__add_wait_queue (&ctx->wqh, &wait);for (res =0 ;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ; +break ; + }if (ULLONG_MAX - ctx->count > ucnt) { res =sizeof (ucnt);break ;
kernel/cgroup.c +0 -1
@@ -4373,7 +4373,6 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) ctx =eventfd_ctx_get (event->eventfd);spin_unlock (&cgrp->event_list_lock); -eventfd_signal (ctx,1 );eventfd_signal_hangup (ctx);eventfd_ctx_put (ctx);
Signature · 1 line
1.8.0.2
PATCH 4/4 cgroup: adapt to the new way of detecting cgroup removal
In reply to Li ZefanIf read() returns with errno == EIDRM, we know the cgroup has been removed.
--- tools/cgroup/cgroup_event_listener.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
tools/cgroup/cgroup_event_listener.c +5 -7
@@ -62,18 +62,16 @@ int main(int argc, char **argv)if (ret ==-1 ) {if (errno ==EINTR )continue ; -err (1 ,"Cannot read from eventfd" ); +if (errno !=EIDRM ) +err (1 ,"Cannot read from eventfd" ); } -assert (ret ==sizeof (result)); - ret =access (event_control_path,W_OK ); -if ((ret ==-1 ) && (errno ==ENOENT )) { -puts ("The cgroup seems to have removed." ); +if (ret ==-1 && errno ==EIDRM ) { +puts ("The cgroup has been removed." );break ; } -if (ret ==-1 ) -err (1 ,"cgroup.event_control is not accessible any more" ); +assert (ret ==sizeof (result));printf ("%s %s: crossed\n" , argv[1 ], argv[2 ]); }
Signature · 1 line
1.8.0.2
(forgot to cc Kirill A. Shutemov <kirill.shutemov-VuQAYsv1563Yd54FQh9/[email protected]>, added)
Quoted 28 lines · Li Zefan
There're three bugs.
- If thread A is removing a cgroup, while thread B is closing an eventfd, the two threads might free the same cgroup event and thus crash the kernel.
This is fixed by patch #1 and patch #2.
- If there're multiple threads are blocking in
read()on the same eventfd, and someone removes the cgroup, only one thread will be notified and unblocked, and others won't be unblocked until those threads are killed.- If thread A is removing a cgroup, while thread B is registering a cgroup event and then read the eventfd, it might block until the thread is killed.
These two are fixed by patch #3.
0001-eventfd-Introduce-eventfd_signal_hangup.patch 0002-cgroup-fix-cgroup_rmdir-vs-close-eventfd-race.patch 0003-eventfd-make-operations-on-eventfd-return-EIDRM-if-i.patch 0004-cgroup-adapt-to-the-new-way-of-detecting-cgroup-remo.patch
--
fs/eventfd.c | 30 ++++++++++++++++++++++++++++++ include/linux/eventfd.h | 5 +++++ kernel/cgroup.c | 30 ++++++++++++++++++------------ tools/cgroup/cgroup_event_listener.c | 12 +++++------- 4 files changed, 58 insertions(+), 19 deletions(-)
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
In reply to Li ZefanQuoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>
Signature · 1 line
Kirill A. Shutemov
Re: [PATCH 2/4] cgroup: fix cgroup_rmdir() vs close(eventfd) race
In reply to Li ZefanQuoted 50 lines · Li Zefan
commit
205a872bd6f9a9a09ef035ef1e90185a8245cc58("cgroup: fix lockdep warning forevent_control") sovled a deadlock by introducing a new bug.We can't access @event without
event_list_lock, otherwise we'll race withcgroup_event_wake()called when closing the eventfd, and then both threads will try to free the same @event.CPU0 CPU1 --------------------------- ----------------------------- cgroup_rmdir() close(eventfd)
list_for_each_entry()cgroup_event_wake()
list_del(event)
list_del(event)
cgroup_event_remove(event)
cgroup_event_remove(event)To fix this, use the new
eventfd_signal_hangup()to notify userspace cgroup is removed.Signed-off-by Li Zefan <[email protected]>--- kernel/cgroup.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)kernel/cgroup.c +4
-2@@ -4302,9 +4302,9 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)struct dentry *d = cgrp->dentry;struct cgroup *parent = cgrp->parent;DEFINE_WAIT (wait); -struct cgroup_event *event, *tmp; +struct eventfd_ctx *ctx; +struct cgroup_event *event;struct cgroup_subsys *ss; -LIST_HEAD (tmp_list);lockdep_assert_held (&d->d_inode->i_mutex);lockdep_assert_held (&cgroup_mutex);@@ -4359,20 +4359,27 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)/* * Unregister events and notify userspace. * Notify userspace about cgroup removing only after rmdir of cgroup +* directory to avoid race between userspace and kernelspace. Since * cgroup_event_wake() is called with the wait queue head locked, +* eventfd_signal() cannot be called while holding event_list_lock. */ spin_lock (&cgrp->event_list_lock);
kernel/cgroup.c static int cgroup_destroy_locked(struct cgroup *cgrp) · line 4369
Quoted 10 lines
kernel/cgroup.c +3
-7-list_splice_init (&cgrp->event_list, &tmp_list); -spin_unlock (&cgrp->event_list_lock); -list_for_each_entry_safe (event, tmp, &tmp_list, list) { -list_del_init (&event->list); -remove_wait_queue (event->wqh, &event->wait); -eventfd_signal (event->eventfd,1 ); -schedule_work (&event->remove); +while (true) { +if (list_empty (&cgrp->event_list)) +break ;
while (!list_empty(&cgrp->event_list)) ?
Otherwise:
Quoted 22 lines
kernel/cgroup.c +12
-0+ + event =list_first_entry (&cgrp->event_list, + struct cgroup_event, list); + ctx =eventfd_ctx_get (event->eventfd); +spin_unlock (&cgrp->event_list_lock); + +eventfd_signal (ctx,1 ); +eventfd_signal_hangup (ctx); +eventfd_ctx_put (ctx); + +spin_lock (&cgrp->event_list_lock); } +spin_unlock (&cgrp->event_list_lock);return 0 ; }Signature · 5 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Kirill A. Shutemov
Re: [PATCH 3/4] eventfd: make operations on eventfd return -EIDRM if it's hung up
In reply to Li ZefanQuoted 63 lines · Li Zefan
Currently when a cgroup is removed, it calls
eventfd_signal()for each registered cgroup event, so userspace can be notified and blocked reads can be unblocked.The problem is, if we have multiple threads blocked on the same eventfd, only one of them will be unlocked.
This patch makes sure all operations on the same eventfd can be unbocked.
There's another problem, a new cgroup event can be registered while we are removing the cgroup, and then reading the eventfd will be blocked until the thread is killed. This patch also fixes this issue.
Signed-off-by Li Zefan <[email protected]>--- fs/eventfd.c | 23 +++++++++++++++++++++-- kernel/cgroup.c | 1 - 2 files changed, 21 insertions(+), 3 deletions(-)fs/eventfd.c +13
-2@@ -35,6 +35,7 @@ struct eventfd_ctx {*/ __u64 count;unsigned int flags; +bool hung_up; };/** @@ -71,11 +72,19 @@ EXPORT_SYMBOL_GPL(eventfd_signal);* eventfd_signal_hangup - Notify that this eventfd is hung up. * @ctx: [in] Pointer to the eventfd context. * -* Issue a POLLHUP wakeup. +* Issue a POLLHUP wakeup. All current blocked reads, writes and polls on +* this eventfd will return with -EIDRM. Future operations on it will also +* return with -EDIRM. */ void eventfd_signal_hangup (struct eventfd_ctx *ctx) { -wake_up_poll (&ctx->wqh,POLLHUP ); +unsigned long flags; + +spin_lock_irqsave (&ctx->wqh.lock, flags); + ctx->hung_up = true; +if (waitqueue_active (&ctx->wqh)) +wake_up_locked_poll (&ctx->wqh,POLLHUP ); +spin_unlock_irqrestore (&ctx->wqh.lock, flags); }static void eventfd_free_ctx (struct eventfd_ctx *ctx)@@ -140,6 +149,8 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait) events |=POLLERR ;if (ULLONG_MAX -1 > ctx->count) events |=POLLOUT ; +if (ctx->hung_up) + events |=POLLHUP ;spin_unlock_irqrestore (&ctx->wqh.lock, flags);return events;
fs/eventfd.c ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt) · line 225
Quoted 7 lines
fs/eventfd.c +4
-0@@ -208,6 +219,10 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt)__add_wait_queue (&ctx->wqh, &wait);for (;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ; +break ; + }
Shouldn't we just return indicate end-of-file if ctx->hung_up?
fs/eventfd.c static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c · line 291
Quoted 9 lines
fs/eventfd.c +2
-0if (ctx->count >0 ) { res =0 ;break ;@@ -272,6 +287,10 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c__add_wait_queue (&ctx->wqh, &wait);for (res =0 ;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ;
-EPIPE?
Quoted 23 lines
fs/eventfd.c +2
-0+break ; + }if (ULLONG_MAX - ctx->count > ucnt) { res =sizeof (ucnt);break ;kernel/cgroup.c +0
-1@@ -4373,7 +4373,6 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) ctx =eventfd_ctx_get (event->eventfd);spin_unlock (&cgrp->event_list_lock); -eventfd_signal (ctx,1 );eventfd_signal_hangup (ctx);eventfd_ctx_put (ctx);Signature · 5 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Kirill A. Shutemov
Re: [PATCH 3/4] eventfd: make operations on eventfd return -EIDRM if it's hung up
Quoted 62 lines · Kirill A. Shutemov
Quoted 61 lines · Li Zefan
Currently when a cgroup is removed, it calls
eventfd_signal()for each registered cgroup event, so userspace can be notified and blocked reads can be unblocked.The problem is, if we have multiple threads blocked on the same eventfd, only one of them will be unlocked.
This patch makes sure all operations on the same eventfd can be unbocked.
There's another problem, a new cgroup event can be registered while we are removing the cgroup, and then reading the eventfd will be blocked until the thread is killed. This patch also fixes this issue.
Signed-off-by Li Zefan <[email protected]>--- fs/eventfd.c | 23 +++++++++++++++++++++-- kernel/cgroup.c | 1 - 2 files changed, 21 insertions(+), 3 deletions(-)fs/eventfd.c +13
-2@@ -35,6 +35,7 @@ struct eventfd_ctx {*/ __u64 count;unsigned int flags; +bool hung_up; };/** @@ -71,11 +72,19 @@ EXPORT_SYMBOL_GPL(eventfd_signal);* eventfd_signal_hangup - Notify that this eventfd is hung up. * @ctx: [in] Pointer to the eventfd context. * -* Issue a POLLHUP wakeup. +* Issue a POLLHUP wakeup. All current blocked reads, writes and polls on +* this eventfd will return with -EIDRM. Future operations on it will also +* return with -EDIRM. */ void eventfd_signal_hangup (struct eventfd_ctx *ctx) { -wake_up_poll (&ctx->wqh,POLLHUP ); +unsigned long flags; + +spin_lock_irqsave (&ctx->wqh.lock, flags); + ctx->hung_up = true; +if (waitqueue_active (&ctx->wqh)) +wake_up_locked_poll (&ctx->wqh,POLLHUP ); +spin_unlock_irqrestore (&ctx->wqh.lock, flags); }static void eventfd_free_ctx (struct eventfd_ctx *ctx)@@ -140,6 +149,8 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait) events |=POLLERR ;if (ULLONG_MAX -1 > ctx->count) events |=POLLOUT ; +if (ctx->hung_up) + events |=POLLHUP ;spin_unlock_irqrestore (&ctx->wqh.lock, flags);
Quoted 12 lines
fs/eventfd.c ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt) · line 225
Quoted 9 lines
fs/eventfd.c +4
-0return events;@@ -208,6 +219,10 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt)__add_wait_queue (&ctx->wqh, &wait);for (;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ; +break ; + }Shouldn't we just return indicate end-of-file if
ctx->hung_up?
makes sense
Quoted 12 lines
fs/eventfd.c static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c · line 291
Quoted 9 lines
fs/eventfd.c +2
-0if (ctx->count >0 ) { res =0 ;break ;@@ -272,6 +287,10 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c__add_wait_queue (&ctx->wqh, &wait);for (res =0 ;;) {set_current_state (TASK_INTERRUPTIBLE ); +if (ctx->hung_up) { + res = -EIDRM ;
-EPIPE?
I wan't sure which errno is most appropriate, but EPIPE seems better.
Quoted 24 lines
Quoted 23 lines
fs/eventfd.c +2
-0+break ; + }if (ULLONG_MAX - ctx->count > ucnt) { res =sizeof (ucnt);break ;kernel/cgroup.c +0
-1@@ -4373,7 +4373,6 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) ctx =eventfd_ctx_get (event->eventfd);spin_unlock (&cgrp->event_list_lock); -eventfd_signal (ctx,1 );eventfd_signal_hangup (ctx);eventfd_ctx_put (ctx);Signature · 5 lines
1.8.0.2 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
In reply to Kirill A. ShutemovQuoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>
Hm.. Looks like it will break eventfd semantics:
1. One eventfd can be used for deliver more then one notification from one or more cgroups. POLLHUP on removing one of cgroups is not valid.
2. It's valid to have eventfd opened only by one userspace application. We should not close it, just because cgroup is removed.
I think problem with multiple threads waiting an event on eventfd should be handled in userspace.
Signature · 1 line
Kirill A. Shutemov
Tejun Heo
maintainerQuoted 1 line · Li Zefan
There're three bugs.
You're gonna send an updated round, right? Will wait for it.
Thanks!
Signature · 1 line
tejun
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
In reply to Kirill A. ShutemovQuoted 10 lines · Kirill A. Shutemov
Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>Hm.. Looks like it will break eventfd semantics:
Quoted 9 lines
1. One eventfd can be used for deliver more then one notification from one or more cgroups. POLLHUP on removing one of cgroups is not valid.
2. It's valid to have eventfd opened only by one userspace application. We should not close it, just because cgroup is removed.
I think problem with multiple threads waiting an event on eventfd should be handled in userspace.
I didn't realize this.. and if a cgroup is removed, the woken thread may not be the thread that is waiting on this cgroup. How crappy.. I don't know how userspace is going to deal with all these.
And another bug spotted. We can pass fd of memory.usage_in_bytes of cgroup A to cgroup.event_control of cgroup B, and then we won't get memory usage notification from A but B! What's worse, if A and B are in different mount hierarchy, boom!
--- kernel/cgroup.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
kernel/cgroup.c +11 -0
@@ -3825,6 +3825,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, const char *buffer) {struct cgroup_event *event =NULL ; +struct cgroup *cgrp_cfile;unsigned int efd, cfd;struct file *efile =NULL ;struct file *cfile =NULL ;
@@ -3880,6 +3881,16 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, goto fail; } +/* +* The file to be monitored must be in the same cgroup as +* cgroup.event_control is. +*/ + cgrp_cfile =__d_cgrp (cfile->f_dentry->d_parent); +if (cgrp_cfile != cgrp) { + ret = -EINVAL ; + goto fail; + } +if (!event->cft->register_event || !event->cft->unregister_event) { ret = -EINVAL ; goto fail;
Signature · 1 line
1.8.0.2
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
Quoted 17 lines · Li Zefan
Quoted 16 lines · Kirill A. Shutemov
Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>Hm.. Looks like it will break eventfd semantics:
1. One eventfd can be used for deliver more then one notification from one or more cgroups. POLLHUP on removing one of cgroups is not valid.
2. It's valid to have eventfd opened only by one userspace application. We should not close it, just because cgroup is removed.
Quoted 6 lines
Quoted 3 lines
I think problem with multiple threads waiting an event on eventfd should be handled in userspace.
I didn't realize this.. and if a cgroup is removed, the woken thread may not be the thread that is waiting on this cgroup.
Why? The only threads who read() or poll() the eventfd will be wake up, won't they? Do you have a code sample to demonstrate the issue?
Quoted 7 lines
How crappy.. I don't know how
userspace is going to deal with all these.And another bug spotted. We can pass fd of memory.usage_in_bytes of cgroup A to cgroup.event_control of cgroup B, and then we won't get memory usage notification from A but B! What's worse, if A and B are in different mount hierarchy, boom!
I think we can ignore which cgroup event_control is belong to, and just use cgroup of cfile as base. It also means you can use one event_control fd for registering events to different cgroups. It can be handy.
Quoted 38 lines
Signed-off-by Li Zefan <[email protected]>--- kernel/cgroup.c | 11 +++++++++++ 1 file changed, 11 insertions(+)kernel/cgroup.c +11
-0@@ -3825,6 +3825,7 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, const char *buffer) {struct cgroup_event *event =NULL ; +struct cgroup *cgrp_cfile;unsigned int efd, cfd;struct file *efile =NULL ;struct file *cfile =NULL ;@@ -3880,6 +3881,16 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft, goto fail; } +/* +* The file to be monitored must be in the same cgroup as +* cgroup.event_control is. +*/ + cgrp_cfile =__d_cgrp (cfile->f_dentry->d_parent); +if (cgrp_cfile != cgrp) { + ret = -EINVAL ; + goto fail; + } +if (!event->cft->register_event || !event->cft->unregister_event) { ret = -EINVAL ; goto fail;Signature · 1 line
1.8.0.2
Signature · 1 line
Kirill A. Shutemov
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
Quoted 22 lines · Kirill A. Shutemov
Quoted 21 lines · Li Zefan
Quoted 19 lines · Kirill A. Shutemov
Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>Hm.. Looks like it will break eventfd semantics:
1. One eventfd can be used for deliver more then one notification from one or more cgroups. POLLHUP on removing one of cgroups is not valid.
2. It's valid to have eventfd opened only by one userspace application. We should not close it, just because cgroup is removed.
I think problem with multiple threads waiting an event on eventfd should be handled in userspace.
Quoted 6 lines
Quoted 2 lines
I didn't realize this.. and if a cgroup is removed, the woken thread may not be the thread that is waiting on this cgroup.
Why? The only threads who
read()orpoll()the eventfd will be wake up, won't they? Do you have a code sample to demonstrate the issue?
All the threads will be woken up, but one of them will consume the event counter, and then all other threads will keep waiting.
Quoted 12 lines
Quoted 7 lines
How crappy.. I don't know how
userspace is going to deal with all these.And another bug spotted. We can pass fd of memory.usage_in_bytes of cgroup A to cgroup.event_control of cgroup B, and then we won't get memory usage notification from A but B! What's worse, if A and B are in different mount hierarchy, boom!
I think we can ignore which cgroup
event_controlis belong to, and just use cgroup of cfile as base. It also means you can use oneevent_controlfd for registering events to different cgroups. It can be handy.
The most reasonal usage is, cgroup.event_control exists in the root cgroup only, and it's used to register events to all cgroups. But I don't think we can change the current interface that each cgroup has a cgroup.event_control, so we'll restrict event registration as my patch does.
Re: [PATCH 1/4] eventfd: introduce eventfd_signal_hangup()
Quoted 26 lines · Li Zefan
Quoted 25 lines · Kirill A. Shutemov
Quoted 23 lines · Li Zefan
Quoted 10 lines · Kirill A. Shutemov
Quoted 6 lines · Kirill A. Shutemov
Quoted 5 lines · Li Zefan
When an eventfd is closed, a wakeup with POLLHUP will be issued, but cgroup wants to issue wakeup explicitly, so when a cgroup is removed userspace can be notified.
Signed-off-by Li Zefan <[email protected]>Hm.. Looks like it will break eventfd semantics:
Quoted 9 lines
1. One eventfd can be used for deliver more then one notification from one or more cgroups. POLLHUP on removing one of cgroups is not valid.
2. It's valid to have eventfd opened only by one userspace application. We should not close it, just because cgroup is removed.
I think problem with multiple threads waiting an event on eventfd should be handled in userspace.
I didn't realize this.. and if a cgroup is removed, the woken thread may not be the thread that is waiting on this cgroup.
Quoted 6 lines
Quoted 3 lines
Why? The only threads who
read()orpoll()the eventfd will be wake up, won't they? Do you have a code sample to demonstrate the issue?All the threads will be woken up, but one of them will consume the event counter, and then all other threads will keep waiting.
The woken up thread can identify that cgroup had disappeared and notify other threads (probably with the same eventfd).
For me it looks like userspace mess rather then kernel issue.
Quoted 13 lines
Quoted 12 lines
Quoted 7 lines
How crappy.. I don't know how
userspace is going to deal with all these.And another bug spotted. We can pass fd of memory.usage_in_bytes of cgroup A to cgroup.event_control of cgroup B, and then we won't get memory usage notification from A but B! What's worse, if A and B are in different mount hierarchy, boom!
I think we can ignore which cgroup
event_controlis belong to, and just use cgroup of cfile as base. It also means you can use oneevent_controlfd for registering events to different cgroups. It can be handy.
Quoted 4 lines
The most reasonal usage is, cgroup.event_control exists in the root cgroup only, and it's used to register events to all cgroups. But I don't think we can change the current interface that each cgroup has a cgroup.event_control, so we'll restrict event registration as my patch does.
Looks good to me.
Signature · 1 line
Kirill A. Shutemov