kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails
PATCH 1/1 kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails
rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR, -ETIMEDOUT).
Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)
kernel/futex.c +16 -14
@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we -* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) { +spin_lock (q.lock_ptr); +/* +* Fixup the pi_state owner and possibly acquire the +* lock if we haven't already. +*/ + res =fixup_owner (uaddr2, &q, !ret); +/* +* If fixup_owner() returned an error, proprogate that. +* If it acquired the lock, clear -ETIMEDOUT or -EINTR. +*/ +if (res) + ret = (res <0 ) ? res :0 ; -/* Unqueue and drop the lock. */ -unqueue_me_pi (&q); +/* Unqueue and drop the lock. */ +unqueue_me_pi (&q); + } }/*
Signature · 1 line
1.7.7.6
HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:
Hi Chen,
Quoted 19 lines
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-0
kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2381
Quoted 9 lines
kernel/futex.c +0
-6@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we -* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret);
This call catches a corner case which appears to be skipped now. Or am I missing how you accounted for that?
Quoted 18 lines
kernel/futex.c +12
-6-/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) { +spin_lock (q.lock_ptr); +/* +* Fixup the pi_state owner and possibly acquire the +* lock if we haven't already. +*/ + res =fixup_owner (uaddr2, &q, !ret); +/* +* If fixup_owner() returned an error, proprogate that. +* If it acquired the lock, clear -ETIMEDOUT or -EINTR. +*/ +if (res)
kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2395
Quoted 10 lines
kernel/futex.c +4
-2+ ret = (res <0 ) ? res :0 ; -/* Unqueue and drop the lock. */ -unqueue_me_pi (&q); +/* Unqueue and drop the lock. */ +unqueue_me_pi (&q); + } }/*
Thanks,
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Quoted 27 lines · Darren Hart
HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:
Hi Chen,
Quoted 22 lines
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-0@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter);
Quoted 12 lines
kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2381
Quoted 6 lines
kernel/futex.c +0
-6-spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we -* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret);This call catches a corner case which appears to be skipped now. Or am I missing how you accounted for that?
Pardon ?
Hmm... this patch lets related code block in "if(!ret) {...}", should not remove any code.
Please help check again for whether what I have done is correct or not.
Thanks.
Quoted 31 lines
Quoted 18 lines
kernel/futex.c +12
-6-/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) { +spin_lock (q.lock_ptr); +/* +* Fixup the pi_state owner and possibly acquire the +* lock if we haven't already. +*/ + res =fixup_owner (uaddr2, &q, !ret); +/* +* If fixup_owner() returned an error, proprogate that. +* If it acquired the lock, clear -ETIMEDOUT or -EINTR. +*/ +if (res)kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2395
Quoted 10 lines
kernel/futex.c +4
-2+ ret = (res <0 ) ? res :0 ; -/* Unqueue and drop the lock. */ -unqueue_me_pi (&q); +/* Unqueue and drop the lock. */ +unqueue_me_pi (&q); + } }/* Thanks,
Signature · 1 line
Chen Gang
Hello Maintainers:
Please help check this patch, when you have time.
Thanks.
Quoted 83 lines · Chen Gang
Quoted 27 lines · Darren Hart
HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:
Hi Chen,
Quoted 22 lines
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-0@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter);Quoted 12 lines
kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2381
Quoted 6 lines
kernel/futex.c +0
-6-spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we -* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret);This call catches a corner case which appears to be skipped now. Or am I missing how you accounted for that?
Pardon ?
Hmm... this patch lets related code block in "if(!ret) {...}", should not remove any code.
Please help check again for whether what I have done is correct or not.
Thanks.
Quoted 31 lines
Quoted 18 lines
kernel/futex.c +12
-6-/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) { +spin_lock (q.lock_ptr); +/* +* Fixup the pi_state owner and possibly acquire the +* lock if we haven't already. +*/ + res =fixup_owner (uaddr2, &q, !ret); +/* +* If fixup_owner() returned an error, proprogate that. +* If it acquired the lock, clear -ETIMEDOUT or -EINTR. +*/ +if (res)kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2395
Quoted 10 lines
kernel/futex.c +4
-2+ ret = (res <0 ) ? res :0 ; -/* Unqueue and drop the lock. */ -unqueue_me_pi (&q); +/* Unqueue and drop the lock. */ +unqueue_me_pi (&q); + } }/* Thanks,
Signature · 1 line
Chen Gang
Quoted 25 lines · Chen Gang
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-3@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we
kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2376
Quoted 10 lines
kernel/futex.c +1
-9-* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) {
Again. This is completely wrong!
We MUST call fixup_owner even if finish_proxy_lock() returned with an error code. Simply because finish_proxy_lock() is called outside of the spin_lock(q.lock_ptr) region and another thread might have modified the futex state. So we need to handle the corner cases otherwise we might leave the futex in some undefined state.
You're reintroducing a hard to decode bug, which got analyzed and fixed in futex_lock_pi() years ago. See the history for the explanation.
Sigh.
tglx
Quoted 46 lines · Thomas Gleixner
Quoted 25 lines · Chen Gang
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-3@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2376
Quoted 10 lines
kernel/futex.c +1
-9-* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) {Again. This is completely wrong!
We MUST call
fixup_ownereven iffinish_proxy_lock()returned with an error code. Simply becausefinish_proxy_lock()is called outside of thespin_lock(q.lock_ptr)region and another thread might have modified the futex state. So we need to handle the corner cases otherwise we might leave the futex in some undefined state.
Quoted 7 lines
You're reintroducing a hard to decode bug, which got analyzed and fixed in
futex_lock_pi()years ago. See the history for the explanation.Sigh.
tglx
Chen, perhaps you can let us know what the failure scenario is that you are trying to address with this patch. I only replied the once as I pointed out the corner-case and expected you to follow up with that. This region of code is very fragile to modifications as it has become more corner-cases than core logic in some places :-)
For starters, I'm not following your second sentence in the commit log. Can you elaborate on the following?
"Original implementation has already noticed about it, but not check it before next work."
Do you have a test-case that demonstrates a failure mode?
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Quoted 51 lines · Darren Hart
Quoted 50 lines · Thomas Gleixner
Quoted 25 lines · Chen Gang
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-3@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2376
Quoted 10 lines
kernel/futex.c +1
-9-* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) {Again. This is completely wrong!
We MUST call
fixup_ownereven iffinish_proxy_lock()returned with an error code. Simply becausefinish_proxy_lock()is called outside of thespin_lock(q.lock_ptr)region and another thread might have modified the futex state. So we need to handle the corner cases otherwise we might leave the futex in some undefined state.You're reintroducing a hard to decode bug, which got analyzed and fixed in
futex_lock_pi()years ago. See the history for the explanation.
Quoted 6 lines
Quoted 3 lines
Sigh.
tglx
Chen, perhaps you can let us know what the failure scenario is that you are trying to address with this patch.
No failure scenario at all.
Chen is on a self defined agenda to fix random kernel bugs in random kernel subdirectories on a given rate by all means. (Google yourself for the details.)
That crusade does not involve any failure analysis or test cases. It's just driven by mechanically checking the code for inconsistencies. Now he tripped over a non obvious return value chain in the futex code. So instead of figuring out why it is coded this way, he just mechanically decided that there is a missing check. Though:
The return value is checked and it needs deep understanding of the way how futexes work to grok why it's necessary to invoke fixup_owner() independent of the rt_mutex_finish_proxy_lock() return value.
The code in question is:
ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1); spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr2, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it * acquired the lock, clear -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;
If you can understand the comments in the code and you are able to follow the implementation of fixup_owner() and the usage of "!ret" as an argument you really should be able to figure out, why this is correct.
I'm well aware, as you are, that this code is hard to grok. BUT:
If this code in futex_wait_requeue_pi() is wrong why did Chen's correctness checker not trigger on the following code in futex_lock_pi()?:
if (!trylock)
ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1);
else {
ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
/* Fixup the trylock return value: */
ret = ret ? 0 : -EWOULDBLOCK;
}
spin_lock(q.lock_ptr);
/*
* Fixup the pi_state owner and possibly acquire the lock if we
* haven't already.
*/
res = fixup_owner(uaddr, &q, !ret);
/*
* If fixup_owner() returned an error, proprogate that. If it acquired
* the lock, clear our -ETIMEDOUT or -EINTR.
*/
if (res)
ret = (res < 0) ? res : 0;It's the very same pattern and according to Chen's logic broken as well.
As I recommended to Chen to read the history of futex.c, I just can recommend the same thing to you to figure out why the heck this is the correct way to handle it.
Hint: The relevant commit starts with: cdf
The code has changed quite a bit since then, but the issue which is described quite well in the commit log is still the same.
Just for the record:
Line 48 of futex.c says: "The futexes are also cursed."
Thanks,
tglx
Firstly, I am glad to see that you did not redirect all my mails to "/dev/null". ;-)
Quoted 26 lines · Thomas Gleixner
Quoted 25 lines · Darren Hart
Quoted 24 lines · Thomas Gleixner
Quoted 22 lines · Chen Gang
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-0@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter);
Quoted 16 lines
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
kernel/futex.c +1
-12-spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we -* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) {Quoted 2 lines
Quoted 2 lines
Again. This is completely wrong!
Yeah, really it is.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
We MUST call
fixup_ownereven iffinish_proxy_lock()returned with an error code. Simply becausefinish_proxy_lock()is called outside of thespin_lock(q.lock_ptr)region and another thread might have modified the futex state. So we need to handle the corner cases otherwise we might leave the futex in some undefined state.You're reintroducing a hard to decode bug, which got analyzed and fixed in
futex_lock_pi()years ago. See the history for the explanation.
Thank you for your details explanation.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Sigh.
tglx
Quoted 9 lines
Quoted 2 lines
Chen, perhaps you can let us know what the failure scenario is that you are trying to address with this patch.
No failure scenario at all.
Chen is on a self defined agenda to fix random kernel bugs in random kernel subdirectories on a given rate by all means. (Google yourself for the details.)
Hmm... what you said is partly correct -- it is part of my goal (at least, I feel it is valuable to kernel).
Others which you did not mention, but still related with kernel:
1. LTP (Linux Test Project), which I will start at q4 of 2013, which can let me provide more tests on kernel (also can find more kernel issues).
2. gcc/binutils: which can find more issues both for kernel and gcc/binutils (I am also communicating with gcc folks too).
3. Documents (or trivial patches): which I am trying, but seems I did not do quite well.
Quoted 66 lines
That crusade does not involve any failure analysis or test cases. It's just driven by mechanically checking the code for inconsistencies. Now he tripped over a non obvious return value chain in the futex code. So instead of figuring out why it is coded this way, he just mechanically decided that there is a missing check. Though:
The return value is checked and it needs deep understanding of the way how futexes work to grok why it's necessary to invoke
fixup_owner()independent of thert_mutex_finish_proxy_lock()return value.The code in question is:
ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1); spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr2, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it * acquired the lock, clear -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;If you can understand the comments in the code and you are able to follow the implementation of
fixup_owner()and the usage of "!ret" as an argument you really should be able to figure out, why this is correct.I'm well aware, as you are, that this code is hard to grok. BUT:
If this code in
futex_wait_requeue_pi()is wrong why did Chen's correctness checker not trigger on the following code infutex_lock_pi()?:if (!trylock) ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1); else { ret = rt_mutex_trylock(&q.pi_state->pi_mutex); /* Fixup the trylock return value: */ ret = ret ? 0 : -EWOULDBLOCK; } spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it acquired * the lock, clear our -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;It's the very same pattern and according to Chen's logic broken as well.
As I recommended to Chen to read the history of
futex.c, I just can recommend the same thing to you to figure out why the heck this is the correct way to handle it.
Quoted 9 lines
Hint: The relevant commit starts with: cdf
The code has changed quite a bit since then, but the issue which is described quite well in the commit log is still the same.
Just for the record:
Line 48 of
futex.csays: "The futexes are also cursed."
Thank you for your explanation (especially spend you expensive time resources on it).
It is my fault:
the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by the next fixup_owner().
Thanks.
Quoted 5 lines
Thanks,
tglx
Signature · 1 line
Chen Gang
After read the code again, I have addtional opinion for discussing, please check thanks.
The related contents are at bottom.
Quoted 69 lines · Chen Gang
Quoted 68 lines · Thomas Gleixner
That crusade does not involve any failure analysis or test cases. It's just driven by mechanically checking the code for inconsistencies. Now he tripped over a non obvious return value chain in the futex code. So instead of figuring out why it is coded this way, he just mechanically decided that there is a missing check. Though:
The return value is checked and it needs deep understanding of the way how futexes work to grok why it's necessary to invoke
fixup_owner()independent of thert_mutex_finish_proxy_lock()return value.The code in question is:
ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1); spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr2, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it * acquired the lock, clear -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;If you can understand the comments in the code and you are able to follow the implementation of
fixup_owner()and the usage of "!ret" as an argument you really should be able to figure out, why this is correct.I'm well aware, as you are, that this code is hard to grok. BUT:
If this code in
futex_wait_requeue_pi()is wrong why did Chen's correctness checker not trigger on the following code infutex_lock_pi()?:if (!trylock) ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1); else { ret = rt_mutex_trylock(&q.pi_state->pi_mutex); /* Fixup the trylock return value: */ ret = ret ? 0 : -EWOULDBLOCK; } spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it acquired * the lock, clear our -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;It's the very same pattern and according to Chen's logic broken as well.
As I recommended to Chen to read the history of
futex.c, I just can recommend the same thing to you to figure out why the heck this is the correct way to handle it.Hint: The relevant commit starts with: cdf
Quoted 7 lines
Quoted 7 lines
The code has changed quite a bit since then, but the issue which is described quite well in the commit log is still the same.
Just for the record:
Line 48 of
futex.csays: "The futexes are also cursed."
fixup_owner() can return 0 for "success, lock not taken".
If rt_mutex_finish_proxy_lock() fail (ret !=0), fixup_owner() may also return 0 (and may printk error message in it), 'ret' will still hold the original error code, and continue.
Is that OK? (for the next checking statement "if (ret == -EFAULT)", according to its comments near above, "if fixup_pi_state_owner() faulted ...", it seems we need skip it in our case).
Thanks.
Quoted 17 lines
Thank you for your explanation (especially spend you expensive time resources on it).
It is my fault:
the 'ret' which return from
rt_mutex_finish_proxy_lock(), is used by the nextfixup_owner().Thanks.
Quoted 5 lines
Thanks,
tglx
Signature · 1 line
Chen Gang
In futex_lock_pi(), after rt_mutex_timed_lock() fails, the followed fixup_owner() still may return 0 to express "success, lock not taken" (may printk kernel error in it).
When it happens, 'res' is zero, 'ret' is still none-zero, and "rt_mutex_owner(&q.pi_state->pi_mutex) == current", it will call rt_mutex_unlock().
At least, they are conflict with the comment near above the related checking statements. and one possible fix is below (only a demo for discussion, not real patch for applying).
--- kernel/futex.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/futex.c +1 -1
@@ -2071,7 +2071,7 @@ retry_private:* If fixup_owner() faulted and was unable to handle the fault, unlock * it and return the fault to userspace. */ -if (ret && (rt_mutex_owner (&q.pi_state->pi_mutex) == current)) +if (ret && res && (rt_mutex_owner (&q.pi_state->pi_mutex) == current))rt_mutex_unlock (&q.pi_state->pi_mutex);/* Unqueue and drop the lock */
Signature · 1 line
1.7.7.6
Quoted 116 lines · Chen Gang
After read the code again, I have addtional opinion for discussing, please check thanks.
The related contents are at bottom.
Quoted 69 lines · Chen Gang
Quoted 68 lines · Thomas Gleixner
That crusade does not involve any failure analysis or test cases. It's just driven by mechanically checking the code for inconsistencies. Now he tripped over a non obvious return value chain in the futex code. So instead of figuring out why it is coded this way, he just mechanically decided that there is a missing check. Though:
The return value is checked and it needs deep understanding of the way how futexes work to grok why it's necessary to invoke
fixup_owner()independent of thert_mutex_finish_proxy_lock()return value.The code in question is:
ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1); spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr2, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it * acquired the lock, clear -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;If you can understand the comments in the code and you are able to follow the implementation of
fixup_owner()and the usage of "!ret" as an argument you really should be able to figure out, why this is correct.I'm well aware, as you are, that this code is hard to grok. BUT:
If this code in
futex_wait_requeue_pi()is wrong why did Chen's correctness checker not trigger on the following code infutex_lock_pi()?:if (!trylock) ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1); else { ret = rt_mutex_trylock(&q.pi_state->pi_mutex); /* Fixup the trylock return value: */ ret = ret ? 0 : -EWOULDBLOCK; } spin_lock(q.lock_ptr); /* * Fixup the pi_state owner and possibly acquire the lock if we * haven't already. */ res = fixup_owner(uaddr, &q, !ret); /* * If fixup_owner() returned an error, proprogate that. If it acquired * the lock, clear our -ETIMEDOUT or -EINTR. */ if (res) ret = (res < 0) ? res : 0;It's the very same pattern and according to Chen's logic broken as well.
As I recommended to Chen to read the history of
futex.c, I just can recommend the same thing to you to figure out why the heck this is the correct way to handle it.Hint: The relevant commit starts with: cdf
Quoted 7 lines
Quoted 7 lines
The code has changed quite a bit since then, but the issue which is described quite well in the commit log is still the same.
Just for the record:
Line 48 of
futex.csays: "The futexes are also cursed."
fixup_owner()can return 0 for "success, lock not taken".If
rt_mutex_finish_proxy_lock()fail (ret !=0),fixup_owner()may also return 0 (and may printk error message in it), 'ret' will still hold the original error code, and continue.Is that OK? (for the next checking statement "if (ret ==
-EFAULT)", according to its comments near above, "iffixup_pi_state_owner()faulted ...", it seems we need skip it in our case).Thanks.
Quoted 17 lines
Thank you for your explanation (especially spend you expensive time resources on it).
It is my fault:
the 'ret' which return from
rt_mutex_finish_proxy_lock(), is used by the nextfixup_owner().Thanks.
Quoted 5 lines
Thanks,
tglx
Signature · 1 line
Chen Gang
Quoted 53 lines · Darren Hart
Quoted 52 lines · Thomas Gleixner
Quoted 25 lines · Chen Gang
rt_mutex_finish_proxy_lock()can return failure code (e.g.-EINTR,-ETIMEDOUT).Original implementation has already noticed about it, but not check it before next work.
Also let coments within 80 columns to pass "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>--- kernel/futex.c | 30 ++++++++++++++++-------------- 1 files changed, 16 insertions(+), 14 deletions(-)kernel/futex.c +0
-3@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, ret =rt_mutex_finish_proxy_lock (pi_mutex, to, &rt_waiter,1 );debug_rt_mutex_free_waiter (&rt_waiter); -spin_lock (q.lock_ptr); -/* -* Fixup the pi_state owner and possibly acquire the lock if we kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2376
Quoted 10 lines
kernel/futex.c +1
-9-* haven't already. -*/ - res =fixup_owner (uaddr2, &q, !ret); -/* -* If fixup_owner() returned an error, proprogate that. If it -* acquired the lock, clear -ETIMEDOUT or -EINTR. -*/ -if (res) - ret = (res <0 ) ? res :0 ; +if (!ret) {Again. This is completely wrong!
We MUST call
fixup_ownereven iffinish_proxy_lock()returned with an error code. Simply becausefinish_proxy_lock()is called outside of thespin_lock(q.lock_ptr)region and another thread might have modified the futex state. So we need to handle the corner cases otherwise we might leave the futex in some undefined state.You're reintroducing a hard to decode bug, which got analyzed and fixed in
futex_lock_pi()years ago. See the history for the explanation.Sigh.
Quoted 8 lines
Quoted 1 line
tglx
Chen, perhaps you can let us know what the failure scenario is that you are trying to address with this patch. I only replied the once as I pointed out the corner-case and expected you to follow up with that. This region of code is very fragile to modifications as it has become more corner-cases than core logic in some places :-)
Oh, thanks, it is my fault:
the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by
the next fixup_owner().
Hmm... excuse me, my English is not quite well, it seems you already know about it, but not say straightly and directly?
next, when find/feel something wrong, can say directly, I can/should understand it (and I need/should thank you, too), that will be more efficient (can save both of us time resources).
:-)
Quoted 8 lines
For starters, I'm not following your second sentence in the commit log. Can you elaborate on the following?
"Original implementation has already noticed about it, but not check it before next work."
Do you have a test-case that demonstrates a failure mode?
No, I just 'found' it, and give a simply 'fix' to let related experts check (and now, we know it is just a spam).
Hmm... for 'test', it is really an 'important thing' to me (not 'urgent thing'), I have plan to start to use LTP (Linux Test Project) in q4 of 2013 (start at 2013-10-01).
Thanks.
Signature · 1 line
Chen Gang