Hi,
This patch set adds fault injection for futex subsystem. It adds faults at
places where reading/writing from user space can return EFAULT. This will
be useful in testing any significant change to futex subsystem.
Thanks,
Sripathi.
Hi,
This patch adds fault points into futex code.
Thanks,
Sripathi.
Signed-off-by: Sripathi Kodi <[email protected]>
---
Index: linux-2.6.32-rc8/kernel/futex.c
===================================================================
--- linux-2.6.32-rc8.orig/kernel/futex.c 2009-11-30 12:21:49.000000000 +0530
+++ linux-2.6.32-rc8/kernel/futex.c 2009-11-30 14:06:05.000000000 +0530
@@ -59,6 +59,7 @@
#include <linux/magic.h>
#include <linux/pid.h>
#include <linux/nsproxy.h>
+#include <linux/fault-inject.h>
#include <asm/futex.h>
@@ -198,6 +199,42 @@
}
}
+DECLARE_FAULT_ATTR(fail_futex_efault);
+
+#ifdef CONFIG_FAIL_FUTEX_EFAULT
+
+static int __init setup_fail_futex_efault(char *str)
+{
+ return setup_fault_attr(&fail_futex_efault, str);
+}
+__setup("fail_futex_efault=", setup_fail_futex_efault);
+
+#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
+
+static int __init fail_futex_efault_debugfs(void)
+{
+ return init_fault_attr_dentries(&fail_futex_efault, "fail_futex_efault");
+}
+late_initcall(fail_futex_efault_debugfs);
+
+#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
+
+bool futex_should_fail(struct fault_attr *attr, ssize_t size)
+{
+ if (should_fail(attr, 1))
+ return true;
+
+ return false;
+}
+
+#else /* CONFIG_FAIL_FUTEX_EFAULT */
+inline bool futex_should_fail(struct fault_attr *attr, ssize_t size)
+{
+ return false;
+}
+
+#endif /* CONFIG_FAIL_FUTEX_EFAULT */
+
/**
* get_futex_key() - Get parameters which are the keys for a futex
* @uaddr: virtual address of the futex
@@ -239,6 +276,10 @@
* but access_ok() should be faster than find_vma()
*/
if (!fshared) {
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
return -EFAULT;
key->private.mm = mm;
@@ -248,6 +289,10 @@
}
again:
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
err = get_user_pages_fast(address, 1, rw == VERIFY_WRITE, &page);
if (err < 0)
return err;
@@ -304,7 +349,12 @@
*/
static int fault_in_user_writeable(u32 __user *uaddr)
{
- int ret = get_user_pages(current, current->mm, (unsigned long)uaddr,
+ int ret;
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
+ ret = get_user_pages(current, current->mm, (unsigned long)uaddr,
1, 1, 0, NULL, NULL);
return ret < 0 ? ret : 0;
}
@@ -332,6 +382,9 @@
{
u32 curval;
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
pagefault_disable();
curval = futex_atomic_cmpxchg_inatomic(uaddr, uval, newval);
pagefault_enable();
@@ -343,6 +396,9 @@
{
int ret;
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
pagefault_disable();
ret = __copy_from_user_inatomic(dest, from, sizeof(u32));
pagefault_enable();
@@ -919,7 +975,15 @@
retry_private:
double_lock_hb(hb1, hb2);
- op_ret = futex_atomic_op_inuser(op, uaddr2);
+
+ op_ret = 0;
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ op_ret = -EFAULT;
+
+ if (op_ret == 0)
+ op_ret = futex_atomic_op_inuser(op, uaddr2);
+
if (unlikely(op_ret < 0)) {
double_unlock_hb(hb1, hb2);
@@ -2009,6 +2073,10 @@
int ret;
retry:
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (get_user(uval, uaddr))
return -EFAULT;
/*
@@ -2383,6 +2451,10 @@
rcu_read_unlock();
}
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
@@ -2417,6 +2489,10 @@
* userspace.
*/
mval = (uval & FUTEX_WAITERS) | FUTEX_OWNER_DIED;
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -1;
+
nval = futex_atomic_cmpxchg_inatomic(uaddr, uval, mval);
if (nval == -EFAULT)
@@ -2444,6 +2520,9 @@
{
unsigned long uentry;
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (get_user(uentry, (unsigned long __user *)head))
return -EFAULT;
@@ -2479,6 +2558,10 @@
/*
* Fetch the relative futex offset:
*/
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return;
+
if (get_user(futex_offset, &head->futex_offset))
return;
/*
@@ -2596,6 +2679,10 @@
if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
cmd == FUTEX_WAIT_BITSET ||
cmd == FUTEX_WAIT_REQUEUE_PI)) {
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
return -EFAULT;
if (!timespec_valid(&ts))
Index: linux-2.6.32-rc8/kernel/futex_compat.c
===================================================================
--- linux-2.6.32-rc8.orig/kernel/futex_compat.c 2009-11-30 12:21:49.000000000 +0530
+++ linux-2.6.32-rc8/kernel/futex_compat.c 2009-11-30 14:06:05.000000000 +0530
@@ -10,6 +10,7 @@
#include <linux/compat.h>
#include <linux/nsproxy.h>
#include <linux/futex.h>
+#include <linux/fault-inject.h>
#include <asm/uaccess.h>
@@ -21,6 +22,10 @@
fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
compat_uptr_t __user *head, int *pi)
{
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (get_user(*uentry, head))
return -EFAULT;
@@ -66,6 +71,10 @@
/*
* Fetch the relative futex offset:
*/
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return;
+
if (get_user(futex_offset, &head->futex_offset))
return;
/*
@@ -160,6 +169,9 @@
read_unlock(&tasklist_lock);
}
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
@@ -182,6 +194,10 @@
if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
cmd == FUTEX_WAIT_BITSET ||
cmd == FUTEX_WAIT_REQUEUE_PI)) {
+
+ if (futex_should_fail(&fail_futex_efault, 1))
+ return -EFAULT;
+
if (get_compat_timespec(&ts, utime))
return -EFAULT;
if (!timespec_valid(&ts))
Index: linux-2.6.32-rc8/include/linux/futex.h
===================================================================
--- linux-2.6.32-rc8.orig/include/linux/futex.h 2009-11-30 12:21:49.000000000 +0530
+++ linux-2.6.32-rc8/include/linux/futex.h 2009-11-30 12:43:14.000000000 +0530
@@ -213,3 +213,6 @@
| ((oparg & 0xfff) << 12) | (cmparg & 0xfff))
#endif
+
+extern struct fault_attr fail_futex_efault;
+extern bool futex_should_fail(struct fault_attr *attr, ssize_t size);
Hi,
This patch adds config option to control futex fault injection.
Thanks,
Sripathi.
Signed-off-by: Sripathi Kodi <[email protected]>
---
Index: linux-2.6.32-rc8/lib/Kconfig.debug
===================================================================
--- linux-2.6.32-rc8.orig/lib/Kconfig.debug 2009-11-30 12:21:44.000000000 +0530
+++ linux-2.6.32-rc8/lib/Kconfig.debug 2009-11-30 15:24:42.000000000 +0530
@@ -882,6 +882,14 @@
Only works with drivers that use the generic timeout handling,
for others it wont do anything.
+config FAIL_FUTEX_EFAULT
+ bool "Fault injection capability for futex subsystem"
+ depends on FAULT_INJECTION
+ help
+ Provide fault injection capability for user-space read-write
+ in futex subsystem. Injects EFAULT errors while read/writing
+ values from user space
+
config FAULT_INJECTION_DEBUG_FS
bool "Debugfs entries for fault-injection capabilities"
depends on FAULT_INJECTION && SYSFS && DEBUG_FS
* Sripathi Kodi <[email protected]> wrote:
> Hi,
>
> This patch set adds fault injection for futex subsystem. It adds
> faults at places where reading/writing from user space can return
> EFAULT. This will be useful in testing any significant change to futex
> subsystem.
Instead of this unacceptably ugly and special-purpose debugfs interface,
please extend perf events to allow event injection. Some other places in
the kernel (which deal with rare events) want/need this capability too.
A good way to do it would be to define tracepoints in these places via a
new kind of TRACE_EVENT(), which would also define an event_injected_*()
callback to use.
So, for example, instead of:
if (futex_should_fail(&fail_futex_efault, 1))
return -EFAULT;
if (unlikely(!access_ok(rw, uaddr, sizeof(u32)))) {
trace_get_futex_key_efault(uaddr);
return -EFAULT;
}
We'd have something like:
if (unlikely(!access_ok(rw, uaddr, sizeof(u32))) ||
event_injected_get_futex_key_efault()) {
trace_get_futex_key_efault(uaddr);
return -EFAULT;
}
And each separate event injection point could thus be triggered
individually.
To use this there would be a separate facility to inject events - via
the perf events ioctl for example.
Ingo
On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
> * Sripathi Kodi <[email protected]> wrote:
>
> > Hi,
> >
> > This patch set adds fault injection for futex subsystem. It adds
> > faults at places where reading/writing from user space can return
> > EFAULT. This will be useful in testing any significant change to futex
> > subsystem.
>
> Instead of this unacceptably ugly and special-purpose debugfs interface,
> please extend perf events to allow event injection. Some other places in
> the kernel (which deal with rare events) want/need this capability too.
Thing is, he's using the 'normal' fault injection code to do this, I see
no objection to doing that.
If you want to redo the fault injection subsystem, then that's another
story, but then we need to convert all of its users over.
* Peter Zijlstra <[email protected]> wrote:
> On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
> > * Sripathi Kodi <[email protected]> wrote:
> >
> > > Hi,
> > >
> > > This patch set adds fault injection for futex subsystem. It adds
> > > faults at places where reading/writing from user space can return
> > > EFAULT. This will be useful in testing any significant change to futex
> > > subsystem.
> >
> > Instead of this unacceptably ugly and special-purpose debugfs
> > interface, please extend perf events to allow event injection. Some
> > other places in the kernel (which deal with rare events) want/need
> > this capability too.
>
> Thing is, he's using the 'normal' fault injection code to do this, I
> see no objection to doing that.
Yes - but its impact to the futex code is butt-ugly. That some facility
is in the kernel does not mean it gets a free pass to be applied
everywhere and anywhere.
An example of that would be tracepoints - there's no free pass to add
tracepoints in new places and some maintainers elect to use different
facilities. (or reject all current facilities)
> If you want to redo the fault injection subsystem, then that's another
> story, but then we need to convert all of its users over.
What i want to see is sane code in futex.c. If we add hooks/callbacks
i'd like it to be a complete solution helping a lot of usecases not some
limited approach helping testability only.
Thanks,
Ingo
Ingo Molnar wrote:
> * Peter Zijlstra <[email protected]> wrote:
>
>> On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
>>> * Sripathi Kodi <[email protected]> wrote:
>>>
>>>> Hi,
>>>>
>>>> This patch set adds fault injection for futex subsystem. It adds
>>>> faults at places where reading/writing from user space can return
>>>> EFAULT. This will be useful in testing any significant change to futex
>>>> subsystem.
>>> Instead of this unacceptably ugly and special-purpose debugfs
>>> interface, please extend perf events to allow event injection. Some
>>> other places in the kernel (which deal with rare events) want/need
>>> this capability too.
>> Thing is, he's using the 'normal' fault injection code to do this, I
>> see no objection to doing that.
>
> Yes - but its impact to the futex code is butt-ugly. That some facility
> is in the kernel does not mean it gets a free pass to be applied
> everywhere and anywhere.
I don't think the "butt-ugly" argument is enough to reject the patch.
It's a fairly subjective metric and I don't think the proposed solution
results in "pretty" code either. In fact the super long function names
and multi-line conditionals are arguably "ugly" (maybe not "butt-ugly"
though). :-)
However, the arguments are solid and I understand wanting to introduce a
new feature in a particular way. Has there been any work done on perf
event injection up to this point or would this be a completely new perf
feature?
--
Darren
>
> An example of that would be tracepoints - there's no free pass to add
> tracepoints in new places and some maintainers elect to use different
> facilities. (or reject all current facilities)
>
>> If you want to redo the fault injection subsystem, then that's another
>> story, but then we need to convert all of its users over.
>
> What i want to see is sane code in futex.c. If we add hooks/callbacks
> i'd like it to be a complete solution helping a lot of usecases not some
> limited approach helping testability only.
>
> Thanks,
>
> Ingo
--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
* Darren Hart <[email protected]> wrote:
> I don't think the "butt-ugly" argument is enough to reject the patch.
It is in my book - i dont ever apply ugly patches intentionally.
> It's a fairly subjective metric and I don't think the proposed
> solution results in "pretty" code either. In fact the super long
> function names and multi-line conditionals are arguably "ugly" (maybe
> not "butt-ugly" though). :-)
>
> However, the arguments are solid and I understand wanting to introduce
> a new feature in a particular way. Has there been any work done on
> perf event injection up to this point or would this be a completely
> new perf feature?
Yeah, it would be a brand new one.
There's a couple of other usecases as well:
- User space logging: apps want to define tracepoints and want to
inject events as they happen - mixed properly into the regular perf
events flow.
- MCE logging: hw faults are so rare that injection is desired to make
sure the policy action chain is working properly.
- Some of the other fault injection sites could be converted to
tracepoints + injection-conditions as well, perhaps. That would give
a more programmable interface and a generic event logging framework.
So it's nice and important work (and by no means trivial - that comes
with the territory) - in case you are interested.
Thanks,
Ingo
On Tue, 1 Dec 2009 17:23:59 +0100
Ingo Molnar <[email protected]> wrote:
>
> * Darren Hart <[email protected]> wrote:
>
> > I don't think the "butt-ugly" argument is enough to reject the patch.
>
> It is in my book - i dont ever apply ugly patches intentionally.
>
> > It's a fairly subjective metric and I don't think the proposed
> > solution results in "pretty" code either. In fact the super long
> > function names and multi-line conditionals are arguably "ugly" (maybe
> > not "butt-ugly" though). :-)
> >
> > However, the arguments are solid and I understand wanting to introduce
> > a new feature in a particular way. Has there been any work done on
> > perf event injection up to this point or would this be a completely
> > new perf feature?
>
> Yeah, it would be a brand new one.
>
Fault injection framework currently in the kernel provides an
infrastructure to set parameters like 'probability', 'interval',
'times' as well as a task filter. I think a fault injection mechanism
using tracepoints-perf will also need to provide such a framework,
because without that the faults become too predictable. For example, if
there are 20 fault points in the kernel, we should be able to trigger
any one of them with a given probability, possibly for a particular
task alone. This infrastructure will have to be built in perf tools in
user space. Do you agree?
Thanks,
Sripathi.
> There's a couple of other usecases as well:
>
> - User space logging: apps want to define tracepoints and want to
> inject events as they happen - mixed properly into the regular perf
> events flow.
>
> - MCE logging: hw faults are so rare that injection is desired to make
> sure the policy action chain is working properly.
>
> - Some of the other fault injection sites could be converted to
> tracepoints + injection-conditions as well, perhaps. That would give
> a more programmable interface and a generic event logging framework.
>
> So it's nice and important work (and by no means trivial - that comes
> with the territory) - in case you are interested.
>
> Thanks,
>
> Ingo
* Sripathi Kodi <[email protected]> wrote:
> On Tue, 1 Dec 2009 17:23:59 +0100
> Ingo Molnar <[email protected]> wrote:
>
> >
> > * Darren Hart <[email protected]> wrote:
> >
> > > I don't think the "butt-ugly" argument is enough to reject the patch.
> >
> > It is in my book - i dont ever apply ugly patches intentionally.
> >
> > > It's a fairly subjective metric and I don't think the proposed
> > > solution results in "pretty" code either. In fact the super long
> > > function names and multi-line conditionals are arguably "ugly" (maybe
> > > not "butt-ugly" though). :-)
> > >
> > > However, the arguments are solid and I understand wanting to introduce
> > > a new feature in a particular way. Has there been any work done on
> > > perf event injection up to this point or would this be a completely
> > > new perf feature?
> >
> > Yeah, it would be a brand new one.
> >
>
> Fault injection framework currently in the kernel provides an
> infrastructure to set parameters like 'probability', 'interval',
> 'times' as well as a task filter. I think a fault injection mechanism
> using tracepoints-perf will also need to provide such a framework,
> because without that the faults become too predictable. For example,
> if there are 20 fault points in the kernel, we should be able to
> trigger any one of them with a given probability, possibly for a
> particular task alone. This infrastructure will have to be built in
> perf tools in user space. Do you agree?
Yeah, definitely so. I think event injection is ultimately useful and
should/could graduate/extend from its current rather limited debugfs
based API to something syscall based (which perf could offer). App
testsuites could programmatically inject faults, etc.
The act of logging/tracing/profiling events and the act of injecting
events is ultimately connected.
Btw., 'perf task filter' is something inherent in perf events: you can
define per task (or per cpu, or per task hierarchy) events.
Ingo