[RFC] [PATCH 0/2] Futex fault injection

[RFC] [PATCH 0/2] Futex fault injection

[Sripathi Kodi]

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.

[RFC] [PATCH 1/2] Futex fault injection: Add fault points

[Sripathi Kodi]

Hi,

This patch adds fault points into futex code.

Thanks,
Sripathi.


Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>

---

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);

[RFC] [PATCH 2/2] Futex fault injection: Config option

[Sripathi Kodi]

Hi,

This patch adds config option to control futex fault injection.

Thanks,
Sripathi.

Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>

---

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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Ingo Molnar]

* Sripathi Kodi <sripathik@in.ibm.com> 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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Peter Zijlstra]

On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
> * Sripathi Kodi <sripathik@in.ibm.com> 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.

Re: [RFC] [PATCH 0/2] Futex fault injection

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
> > * Sripathi Kodi <sripathik@in.ibm.com> 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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Darren Hart]

Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Tue, 2009-12-01 at 11:33 +0100, Ingo Molnar wrote:
>>> * Sripathi Kodi <sripathik@in.ibm.com> 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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Ingo Molnar]

* Darren Hart <dvhltc@us.ibm.com> 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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Sripathi Kodi]

On Tue, 1 Dec 2009 17:23:59 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Darren Hart <dvhltc@us.ibm.com> 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

Re: [RFC] [PATCH 0/2] Futex fault injection

[Ingo Molnar]

* Sripathi Kodi <sripathik@in.ibm.com> wrote:

> On Tue, 1 Dec 2009 17:23:59 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Darren Hart <dvhltc@us.ibm.com> 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