Hi Andrew,
Today's linux-next merge of the akpm-current tree got a conflict in
kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
size for better performance") from the tip tree and commit 61beee6c76e5
("futex: switch to USER_DS for futex test") from the akpm-current tree.
I fixed it up (see below) and can carry the fix as necessary (no action
is required).
--
Cheers,
Stephen Rothwell [email protected]
diff --cc kernel/futex.c
index 3666aa0017ed,66d23727c6ab..000000000000
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@@ -63,7 -63,7 +63,8 @@@
#include <linux/sched/rt.h>
#include <linux/hugetlb.h>
#include <linux/freezer.h>
+#include <linux/bootmem.h>
+ #include <linux/uaccess.h>
#include <asm/futex.h>
@@@ -2845,19 -2734,9 +2846,20 @@@ SYSCALL_DEFINE6(futex, u32 __user *, ua
static int __init futex_init(void)
{
+ mm_segment_t fs;
u32 curval;
- int i;
+ unsigned long i;
+
+#if CONFIG_BASE_SMALL
+ futex_hashsize = 16;
+#else
+ futex_hashsize = roundup_pow_of_two(256 * num_possible_cpus());
+#endif
+
+ futex_queues = alloc_large_system_hash("futex", sizeof(*futex_queues),
+ futex_hashsize, 0,
+ futex_hashsize < 256 ? HASH_SMALL : 0,
+ NULL, NULL, futex_hashsize, futex_hashsize);
/*
* This will fail and we want it. Some arch implementations do
@@@ -2869,10 -2748,13 +2871,13 @@@
* implementation, the non-functional ones will return
* -ENOSYS.
*/
+ fs = get_fs();
+ set_fs(USER_DS);
if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
futex_cmpxchg_enabled = 1;
+ set_fs(fs);
- for (i = 0; i < ARRAY_SIZE(futex_queues); i++) {
+ for (i = 0; i < futex_hashsize; i++) {
plist_head_init(&futex_queues[i].chain);
spin_lock_init(&futex_queues[i].lock);
}
On Tue, 2014-01-14 at 15:53 +1100, Stephen Rothwell wrote:
> Hi Andrew,
>
> Today's linux-next merge of the akpm-current tree got a conflict in
> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
> size for better performance") from the tip tree and commit 61beee6c76e5
> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
Thanks Stephen. At least for my bits the fix seems ok. In the future,
though, changes to this evil file should be routed only through one
tree.
Thanks,
Davidlohr
On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
> Hi Andrew,
>
> Today's linux-next merge of the akpm-current tree got a conflict in
> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
> size for better performance") from the tip tree and commit 61beee6c76e5
> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
>
> @@@ -2869,10 -2748,13 +2871,13 @@@
> * implementation, the non-functional ones will return
> * -ENOSYS.
> */
> + fs = get_fs();
> + set_fs(USER_DS);
> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
> futex_cmpxchg_enabled = 1;
> + set_fs(fs);
>
This seems terribly broken, the *futex_value*() ops should not need
that; they are supposed to access userspace without any of that.
On Tue, Jan 14, 2014 at 1:51 PM, Peter Zijlstra <[email protected]> wrote:
> On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
>> Today's linux-next merge of the akpm-current tree got a conflict in
>> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
>> size for better performance") from the tip tree and commit 61beee6c76e5
>> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
>>
>> @@@ -2869,10 -2748,13 +2871,13 @@@
>> * implementation, the non-functional ones will return
>> * -ENOSYS.
>> */
>> + fs = get_fs();
>> + set_fs(USER_DS);
>> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
>> futex_cmpxchg_enabled = 1;
>> + set_fs(fs);
>>
>
> This seems terribly broken, the *futex_value*() ops should not need
> that; they are supposed to access userspace without any of that.
Why don't they need set_fs(USER_DS)?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Jan 14, 2014 at 02:17:55PM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 14, 2014 at 1:51 PM, Peter Zijlstra <[email protected]> wrote:
> > On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
> >> Today's linux-next merge of the akpm-current tree got a conflict in
> >> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
> >> size for better performance") from the tip tree and commit 61beee6c76e5
> >> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
> >>
> >> @@@ -2869,10 -2748,13 +2871,13 @@@
> >> * implementation, the non-functional ones will return
> >> * -ENOSYS.
> >> */
> >> + fs = get_fs();
> >> + set_fs(USER_DS);
> >> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
> >> futex_cmpxchg_enabled = 1;
> >> + set_fs(fs);
> >>
> >
> > This seems terribly broken, the *futex_value*() ops should not need
> > that; they are supposed to access userspace without any of that.
>
> Why don't they need set_fs(USER_DS)?
Why would they need it? These functions explicitly take a __user pointer
and are expected to do whatever is needed to perform the operation. None
of the other futex bits use USER_DS either.
Furthermore, from the Changelog of:
e4f2dfbb5e92be4e46c0625f4f8eb101110f756f
" This patch adds that support, but only for uniprocessor machines,
which is adequate for M68K. For UP it's enough to disable preemption
to ensure mutual exclusion (futexes don't need to care about other
hardware agents), and the mandatory pagefault_disable() does just that. "
It is wrong to rely on the fact that pagefault_disable() disables
preemption. This is fact not true on -rt.
On 01/14/2014 04:51 AM, Peter Zijlstra wrote:
> On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
>> Hi Andrew,
>>
>> Today's linux-next merge of the akpm-current tree got a conflict in
>> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
>> size for better performance") from the tip tree and commit 61beee6c76e5
>> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
>>
>> @@@ -2869,10 -2748,13 +2871,13 @@@
>> * implementation, the non-functional ones will return
>> * -ENOSYS.
>> */
>> + fs = get_fs();
>> + set_fs(USER_DS);
>> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
>> futex_cmpxchg_enabled = 1;
>> + set_fs(fs);
>>
>
> This seems terribly broken, the *futex_value*() ops should not need
> that; they are supposed to access userspace without any of that.
>
I am *guessing* that m68k is has get_fs() == KERNEL_DS at the point that
futex_init() is called. This would seem a bit of a peculiarity to m68k,
and as such it would seem like it would be better for it to belong in
the m68k-specific code, but since futex_init() is init code and only
called once anyway it shouldn't cause any harm...
-hpa
On Tue, Jan 14, 2014 at 4:15 PM, H. Peter Anvin <[email protected]> wrote:
> On 01/14/2014 04:51 AM, Peter Zijlstra wrote:
>> On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
>>> Hi Andrew,
>>>
>>> Today's linux-next merge of the akpm-current tree got a conflict in
>>> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
>>> size for better performance") from the tip tree and commit 61beee6c76e5
>>> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
>>>
>>> @@@ -2869,10 -2748,13 +2871,13 @@@
>>> * implementation, the non-functional ones will return
>>> * -ENOSYS.
>>> */
>>> + fs = get_fs();
>>> + set_fs(USER_DS);
>>> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
>>> futex_cmpxchg_enabled = 1;
>>> + set_fs(fs);
>>>
>>
>> This seems terribly broken, the *futex_value*() ops should not need
>> that; they are supposed to access userspace without any of that.
>
> I am *guessing* that m68k is has get_fs() == KERNEL_DS at the point that
> futex_init() is called. This would seem a bit of a peculiarity to m68k,
> and as such it would seem like it would be better for it to belong in
> the m68k-specific code, but since futex_init() is init code and only
> called once anyway it shouldn't cause any harm...
Yes it does. So when getting the exception on 68030, we notice it's a kernel
space access error, not a user space access error, and crash.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Jan 14, 2014 at 04:20:36PM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 14, 2014 at 4:15 PM, H. Peter Anvin <[email protected]> wrote:
> > On 01/14/2014 04:51 AM, Peter Zijlstra wrote:
> >> On Tue, Jan 14, 2014 at 03:53:31PM +1100, Stephen Rothwell wrote:
> >>> Hi Andrew,
> >>>
> >>> Today's linux-next merge of the akpm-current tree got a conflict in
> >>> kernel/futex.c between commit a52b89ebb6d4 ("futexes: Increase hash table
> >>> size for better performance") from the tip tree and commit 61beee6c76e5
> >>> ("futex: switch to USER_DS for futex test") from the akpm-current tree.
> >>>
> >>> @@@ -2869,10 -2748,13 +2871,13 @@@
> >>> * implementation, the non-functional ones will return
> >>> * -ENOSYS.
> >>> */
> >>> + fs = get_fs();
> >>> + set_fs(USER_DS);
> >>> if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
> >>> futex_cmpxchg_enabled = 1;
> >>> + set_fs(fs);
> >>>
> >>
> >> This seems terribly broken, the *futex_value*() ops should not need
> >> that; they are supposed to access userspace without any of that.
> >
> > I am *guessing* that m68k is has get_fs() == KERNEL_DS at the point that
> > futex_init() is called. This would seem a bit of a peculiarity to m68k,
> > and as such it would seem like it would be better for it to belong in
> > the m68k-specific code, but since futex_init() is init code and only
> > called once anyway it shouldn't cause any harm...
>
> Yes it does. So when getting the exception on 68030, we notice it's a kernel
> space access error, not a user space access error, and crash.
Is there a good reason m68k works like this? That is, shouldn't we fix
the arch code instead of littering the generic code with weirdness like
this?
On 01/14/2014 07:41 AM, Peter Zijlstra wrote:
>>>
>>> I am *guessing* that m68k is has get_fs() == KERNEL_DS at the point that
>>> futex_init() is called. This would seem a bit of a peculiarity to m68k,
>>> and as such it would seem like it would be better for it to belong in
>>> the m68k-specific code, but since futex_init() is init code and only
>>> called once anyway it shouldn't cause any harm...
>>
>> Yes it does. So when getting the exception on 68030, we notice it's a kernel
>> space access error, not a user space access error, and crash.
>
> Is there a good reason m68k works like this? That is, shouldn't we fix
> the arch code instead of littering the generic code with weirdness like
> this?
>
Given that futex_init is called from initcall, this seems *really* weird
on the part of m68k. I agree this should be fixed where the problem sits.
-hpa
On 01/14/2014 05:17 AM, Geert Uytterhoeven wrote:
>>
>> This seems terribly broken, the *futex_value*() ops should not need
>> that; they are supposed to access userspace without any of that.
>
> Why don't they need set_fs(USER_DS)?
>
> Gr{oetje,eeting}s,
>
> Geert
Because USER_DS is the normal operating state? It would appear m68k is
the only(?) arch that calls initcalls with get_fs() == KERNEL_DS...
-hpa