Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
Hi,
Indeed, eventpoll_release_file() is on the 'write' side of the rcu
here (the read-side being in reverse_path_check_proc()). It's a race,
in that 'struct epitem' has to be freed by rcu before the next loop
iteration such that epi->fllink.next pointer is no longer valid. So,
I suspect that makes this more difficult to hit, but obviously it
happens.
Thanks for finding/fixing this!
Acked-by: Jason Baron
On 06/16/2014 10:58 PM, Konstantin Khlebnikov wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't
> essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov
> Reported-by: Cyrill Gorcunov
> Cc: Stable # 3.13+
> Cc: Sasha Levin
> Cc: Jason Baron
> ---
> fs/eventpoll.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index b73e062..b10b48c 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
> void eventpoll_release_file(struct file *file)
> {
> struct eventpoll *ep;
> - struct epitem *epi;
> + struct epitem *epi, *next;
>
> /*
>* We don't want to get "file->f_lock" because it is not
> @@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
>* Besides, ep_remove() acquires the lock, so we can't hold it here.
>*/
> mutex_lock();
> - list_for_each_entry_rcu(epi, >f_ep_links, fllink) {
> + list_for_each_entry_safe(epi, next, >f_ep_links, fllink) {
> ep = epi->ep;
> mutex_lock_nested(>mtx, 0);
> ep_remove(ep, epi);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
Hi,
Indeed, eventpoll_release_file() is on the 'write' side of the rcu
here (the read-side being in reverse_path_check_proc()). It's a race,
in that 'struct epitem' has to be freed by rcu before the next loop
iteration such that epi-fllink.next pointer is no longer valid. So,
I suspect that makes this more difficult to hit, but obviously it
happens.
Thanks for finding/fixing this!
Acked-by: Jason Baron jba...@akamai.com
On 06/16/2014 10:58 PM, Konstantin Khlebnikov wrote:
This fixes use-after-free of epi-fllink.next inside list loop macro.
This loop actually releases elements in the body. List is rcu-protected
but here we cannot hold rcu_read_lock because we need to lock mutex inside.
Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't
essential
because nobody can change this list under us, it's final fput for this file.
Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
(epoll: optimize EPOLL_CTL_DEL using rcu)
Signed-off-by: Konstantin Khlebnikov koc...@gmail.com
Reported-by: Cyrill Gorcunov gorcu...@openvz.org
Cc: Stable sta...@vger.kernel.org # 3.13+
Cc: Sasha Levin sasha.le...@oracle.com
Cc: Jason Baron jba...@akamai.com
---
fs/eventpoll.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index b73e062..b10b48c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
void eventpoll_release_file(struct file *file)
{
struct eventpoll *ep;
- struct epitem *epi;
+ struct epitem *epi, *next;
/*
* We don't want to get file-f_lock because it is not
@@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
* Besides, ep_remove() acquires the lock, so we can't hold it here.
*/
mutex_lock(epmutex);
- list_for_each_entry_rcu(epi, file-f_ep_links, fllink) {
+ list_for_each_entry_safe(epi, next, file-f_ep_links, fllink) {
ep = epi-ep;
mutex_lock_nested(ep-mtx, 0);
ep_remove(ep, epi);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
On Tue, Jun 17, 2014 at 06:58:05AM +0400, Konstantin Khlebnikov wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't
> essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov
> Reported-by: Cyrill Gorcunov
> Cc: Stable # 3.13+
> Cc: Sasha Levin
> Cc: Jason Baron
Acked-by: Cyrill Gorcunov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
On Tue, Jun 17, 2014 at 6:58 AM, Konstantin Khlebnikov wrote:
> This fixes use-after-free of epi->fllink.next inside list loop macro.
> This loop actually releases elements in the body. List is rcu-protected
> but here we cannot hold rcu_read_lock because we need to lock mutex inside.
>
> Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't
> essential
> because nobody can change this list under us, it's final fput for this file.
>
> Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
> ("epoll: optimize EPOLL_CTL_DEL using rcu")
>
> Signed-off-by: Konstantin Khlebnikov
> Reported-by: Cyrill Gorcunov
> Cc: Stable # 3.13+
> Cc: Sasha Levin
> Cc: Jason Baron
> ---
oops from Cyrill's report
https://plus.google.com/+CyrillGorcunov/posts/RPGi1ECc3Tk
[ 14.448483] general protection fault: [#1] [ 14.448483]
general protection fault: [#1] PREEMPT PREEMPT SMP SMP
[ 14.448533] Modules linked in:[ 14.448533] Modules linked in:
[ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463
[ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463
[ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 14.448533] task: 880037a28f50 ti: 880037a2c000 task.ti:
880037a2c000
[ 14.448533] task: 880037a28f50 ti: 880037a2c000 task.ti:
880037a2c000
[ 14.448533] RIP: 0010:[] [ 14.448533] RIP:
0010:[] []
__lock_acquire+0xeb/0xf07
[] __lock_acquire+0xeb/0xf07
[ 14.448533] RSP: 0018:880037a2fcf0 EFLAGS: 00010002
[ 14.448533] RSP: 0018:880037a2fcf0 EFLAGS: 00010002
[ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: 880037a28f50 RCX:
[ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: 880037a28f50 RCX:
[ 14.448533] RDX: RSI: RDI: 88003b960200
[ 14.448533] RDX: RSI: RDI: 88003b960200
[ 14.448533] RBP: 880037a2fd88 R08: 0001 R09:
[ 14.448533] RBP: 880037a2fd88 R08: 0001 R09:
[ 14.448533] R10: R11: R12: 880037a28f50
[ 14.448533] R10: R11: R12: 880037a28f50
[ 14.448533] R13: R14: 0001 R15: 88003b960200
[ 14.448533] R13: R14: 0001 R15: 88003b960200
[ 14.448533] FS: 7fafb700d740() GS:88003fc0()
knlGS:
[ 14.448533] FS: 7fafb700d740() GS:88003fc0()
knlGS:
[ 14.448533] CS: 0010 DS: ES: CR0: 80050033
[ 14.448533] CS: 0010 DS: ES: CR0: 80050033
[ 14.448533] CR2: 7fd6cc054414 CR3: 378ee000 CR4: 06f0
[ 14.448533] CR2: 7fd6cc054414 CR3: 378ee000 CR4: 06f0
[ 14.448533] Stack:
[ 14.448533] Stack:
[ 14.448533] 8800[ 14.448533] 8800
8801 8801
8800 8800
[ 14.448533] [ 14.448533]
88003fdcd2b0 88003fdcd2b0 81c41d80 81c41d80
880037a2fe30 880037a2fe30
[ 14.448533] 81303a7e[ 14.448533] 81303a7e
009c0496 009c0496
[ 14.448533] Call Trace:
[ 14.448533] Call Trace:
[ 14.448533] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 14.448533] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 14.448533] [] lock_acquire+0xfe/0x146
[ 14.448533] [] lock_acquire+0xfe/0x146
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] mutex_lock_nested+0x77/0x3f0
[ 14.448533] [] mutex_lock_nested+0x77/0x3f0
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] ? eventpoll_release_file+0x68/0xc1
[ 14.448533] [] eventpoll_release_file+0x68/0xc1
[ 14.448533] [] eventpoll_release_file+0x68/0xc1
[ 14.448533] [] __fput+0xc5/0x1b6
[ 14.448533] [] __fput+0xc5/0x1b6
[ 14.448533] [] fput+0xe/0x10
[ 14.448533] [] fput+0xe/0x10
[ 14.448533] [] task_work_run+0x7d/0xa6
[ 14.448533] [] task_work_run+0x7d/0xa6
[ 14.448533] [] do_notify_resume+0x63/0x68
[ 14.448533] [] do_notify_resume+0x63/0x68
[ 14.448533] [] int_signal+0x12/0x17
[ 14.448533] [] int_signal+0x12/0x17
[ 14.448533] Code: [ 14.448533] Code: 85 85 c0 c0 75 75 27 27 31
31 d2 d2 4c 4c 89 89 ff ff 44 44 89 89 4c 4c 24 24 08 08 44 44 89 89
44 44 24 24 10 10 e8 e8 10 10 f7 f7 ff ff ff ff 48 48 85 85 c0 c0
[PATCH] epoll: fix use-after-free in eventpoll_release_file
This fixes use-after-free of epi->fllink.next inside list loop macro.
This loop actually releases elements in the body. List is rcu-protected
but here we cannot hold rcu_read_lock because we need to lock mutex inside.
Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
because nobody can change this list under us, it's final fput for this file.
Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
("epoll: optimize EPOLL_CTL_DEL using rcu")
Signed-off-by: Konstantin Khlebnikov
Reported-by: Cyrill Gorcunov
Cc: Stable # 3.13+
Cc: Sasha Levin
Cc: Jason Baron
---
fs/eventpoll.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index b73e062..b10b48c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
void eventpoll_release_file(struct file *file)
{
struct eventpoll *ep;
- struct epitem *epi;
+ struct epitem *epi, *next;
/*
* We don't want to get "file->f_lock" because it is not
@@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
* Besides, ep_remove() acquires the lock, so we can't hold it here.
*/
mutex_lock();
- list_for_each_entry_rcu(epi, >f_ep_links, fllink) {
+ list_for_each_entry_safe(epi, next, >f_ep_links, fllink) {
ep = epi->ep;
mutex_lock_nested(>mtx, 0);
ep_remove(ep, epi);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] epoll: fix use-after-free in eventpoll_release_file
This fixes use-after-free of epi-fllink.next inside list loop macro.
This loop actually releases elements in the body. List is rcu-protected
but here we cannot hold rcu_read_lock because we need to lock mutex inside.
Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential
because nobody can change this list under us, it's final fput for this file.
Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40
(epoll: optimize EPOLL_CTL_DEL using rcu)
Signed-off-by: Konstantin Khlebnikov koc...@gmail.com
Reported-by: Cyrill Gorcunov gorcu...@openvz.org
Cc: Stable sta...@vger.kernel.org # 3.13+
Cc: Sasha Levin sasha.le...@oracle.com
Cc: Jason Baron jba...@akamai.com
---
fs/eventpoll.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index b73e062..b10b48c 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -910,7 +910,7 @@ static const struct file_operations eventpoll_fops = {
void eventpoll_release_file(struct file *file)
{
struct eventpoll *ep;
- struct epitem *epi;
+ struct epitem *epi, *next;
/*
* We don't want to get file-f_lock because it is not
@@ -926,7 +926,7 @@ void eventpoll_release_file(struct file *file)
* Besides, ep_remove() acquires the lock, so we can't hold it here.
*/
mutex_lock(epmutex);
- list_for_each_entry_rcu(epi, file-f_ep_links, fllink) {
+ list_for_each_entry_safe(epi, next, file-f_ep_links, fllink) {
ep = epi-ep;
mutex_lock_nested(ep-mtx, 0);
ep_remove(ep, epi);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
On Tue, Jun 17, 2014 at 6:58 AM, Konstantin Khlebnikov koc...@gmail.com wrote: This fixes use-after-free of epi-fllink.next inside list loop macro. This loop actually releases elements in the body. List is rcu-protected but here we cannot hold rcu_read_lock because we need to lock mutex inside. Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential because nobody can change this list under us, it's final fput for this file. Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40 (epoll: optimize EPOLL_CTL_DEL using rcu) Signed-off-by: Konstantin Khlebnikov koc...@gmail.com Reported-by: Cyrill Gorcunov gorcu...@openvz.org Cc: Stable sta...@vger.kernel.org # 3.13+ Cc: Sasha Levin sasha.le...@oracle.com Cc: Jason Baron jba...@akamai.com --- oops from Cyrill's report https://plus.google.com/+CyrillGorcunov/posts/RPGi1ECc3Tk [ 14.448483] general protection fault: [#1] [ 14.448483] general protection fault: [#1] PREEMPT PREEMPT SMP SMP [ 14.448533] Modules linked in:[ 14.448533] Modules linked in: [ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463 [ 14.448533] CPU: 0 PID: 1153 Comm: plymouthd Not tainted 3.16.0-rc1 #463 [ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 14.448533] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 14.448533] task: 880037a28f50 ti: 880037a2c000 task.ti: 880037a2c000 [ 14.448533] task: 880037a28f50 ti: 880037a2c000 task.ti: 880037a2c000 [ 14.448533] RIP: 0010:[810abee9] [ 14.448533] RIP: 0010:[810abee9] [810abee9] __lock_acquire+0xeb/0xf07 [810abee9] __lock_acquire+0xeb/0xf07 [ 14.448533] RSP: 0018:880037a2fcf0 EFLAGS: 00010002 [ 14.448533] RSP: 0018:880037a2fcf0 EFLAGS: 00010002 [ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: 880037a28f50 RCX: [ 14.448533] RAX: 6b6b6b6b6b6b6b6b RBX: 880037a28f50 RCX: [ 14.448533] RDX: RSI: RDI: 88003b960200 [ 14.448533] RDX: RSI: RDI: 88003b960200 [ 14.448533] RBP: 880037a2fd88 R08: 0001 R09: [ 14.448533] RBP: 880037a2fd88 R08: 0001 R09: [ 14.448533] R10: R11: R12: 880037a28f50 [ 14.448533] R10: R11: R12: 880037a28f50 [ 14.448533] R13: R14: 0001 R15: 88003b960200 [ 14.448533] R13: R14: 0001 R15: 88003b960200 [ 14.448533] FS: 7fafb700d740() GS:88003fc0() knlGS: [ 14.448533] FS: 7fafb700d740() GS:88003fc0() knlGS: [ 14.448533] CS: 0010 DS: ES: CR0: 80050033 [ 14.448533] CS: 0010 DS: ES: CR0: 80050033 [ 14.448533] CR2: 7fd6cc054414 CR3: 378ee000 CR4: 06f0 [ 14.448533] CR2: 7fd6cc054414 CR3: 378ee000 CR4: 06f0 [ 14.448533] Stack: [ 14.448533] Stack: [ 14.448533] 8800[ 14.448533] 8800 8801 8801 8800 8800 [ 14.448533] [ 14.448533] 88003fdcd2b0 88003fdcd2b0 81c41d80 81c41d80 880037a2fe30 880037a2fe30 [ 14.448533] 81303a7e[ 14.448533] 81303a7e 009c0496 009c0496 [ 14.448533] Call Trace: [ 14.448533] Call Trace: [ 14.448533] [81303a7e] ? trace_hardirqs_on_thunk+0x3a/0x3f [ 14.448533] [81303a7e] ? trace_hardirqs_on_thunk+0x3a/0x3f [ 14.448533] [810ace03] lock_acquire+0xfe/0x146 [ 14.448533] [810ace03] lock_acquire+0xfe/0x146 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [81639ffa] mutex_lock_nested+0x77/0x3f0 [ 14.448533] [81639ffa] mutex_lock_nested+0x77/0x3f0 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] ? eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] eventpoll_release_file+0x68/0xc1 [ 14.448533] [811a97c5] eventpoll_release_file+0x68/0xc1 [ 14.448533] [8117451c] __fput+0xc5/0x1b6 [ 14.448533] [8117451c] __fput+0xc5/0x1b6 [ 14.448533] [81174643] fput+0xe/0x10 [ 14.448533] [81174643]
Re: [PATCH] epoll: fix use-after-free in eventpoll_release_file
On Tue, Jun 17, 2014 at 06:58:05AM +0400, Konstantin Khlebnikov wrote: This fixes use-after-free of epi-fllink.next inside list loop macro. This loop actually releases elements in the body. List is rcu-protected but here we cannot hold rcu_read_lock because we need to lock mutex inside. Obvious solution is to use list_for_each_entry_safe(). RCU-ness isn't essential because nobody can change this list under us, it's final fput for this file. Bug is here since ae10b2b4eb01bedc91d29d5c5bb9e416fd806c40 (epoll: optimize EPOLL_CTL_DEL using rcu) Signed-off-by: Konstantin Khlebnikov koc...@gmail.com Reported-by: Cyrill Gorcunov gorcu...@openvz.org Cc: Stable sta...@vger.kernel.org # 3.13+ Cc: Sasha Levin sasha.le...@oracle.com Cc: Jason Baron jba...@akamai.com Acked-by: Cyrill Gorcunov gorcu...@openvz.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
