uaccess: better might_sleep/might_fault behavior

The only reason uaccess routines might sleep
is if they fault. Make this explicit.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/arm64/include/asm/uaccess.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The only reason uaccess routines might sleep
is if they fault. Make this explicit.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/tile/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

There are several ways to make sure might_fault
calling function does not sleep.
One is to use it on kernel or otherwise locked memory - apparently nfs/sunrpc does this. As noted by Ingo, this is handled by the migh_fault() implementation in mm/memory.c but not the one in linux/kernel.h so in the current code might_fault() schedules differently depending on CONFIG_PROVE_LOCKING, which is an undesired semantical side effect.

Another is to call pagefault_disable: in this case the page fault handler will go to fixups processing and we get an error instead of sleeping, so the might_sleep annotation is a false positive. vhost driver wants to do this now in order to reuse socket ops under a spinlock (and fall back on slower thread handler on error).

Address both issues by:
- dropping the unconditional call to might_sleep from the fast might_fault code in linux/kernel.h
- checking for pagefault_disable() in the CONFIG_PROVE_LOCKING implementation

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/kernel.h |  1 -
 mm/memory.c            | 14 +++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

include/linux/kernel.h +0 -0

index e96329c..322b065 100644

include/linux/kernel.h +0 -0

 }
 #endif
 

mm/memory.c +0 -0

index 6dc1882..1b8327b 100644
@@ -4222,13 +4222,17 @@ void might_fault(void)
 	if (segment_eq(get_fs(), KERNEL_DS))

mm/memory.c +6 -1

 	 */
-	if (!in_atomic() && current->mm)
+	if (in_atomic())
+		return;
+
+	might_sleep();
+
+	if (current->mm)
 		might_lock_read(&current->mm->mmap_sem);
 }
 EXPORT_SYMBOL(might_fault);

Signature · 1 line

MST

Quoted 9 lines · Michael S. Tsirkin

There are several ways to make sure might_fault
calling function does not sleep.
One is to use it on kernel or otherwise locked memory - apparently nfs/sunrpc does this. As noted by Ingo, this is handled by the migh_fault() implementation in mm/memory.c but not the one in linux/kernel.h so in the current code might_fault() schedules differently depending on CONFIG_PROVE_LOCKING, which is an undesired semantical side effect.

Quoted 6 lines

Another is to call pagefault_disable: in this case the page fault handler will go to fixups processing and we get an error instead of sleeping, so the might_sleep annotation is a false positive. vhost driver wants to do this now in order to reuse socket ops under a spinlock (and fall back on slower thread handler on error).

Are you using the assumption that spin_lock() implies preempt_disable() implies pagefault_disable()? Note that this assumption isn't valid for -rt where the spinlock becomes preemptible but we'll not disable pagefaults.

Quoted 12 lines

Address both issues by:
- dropping the unconditional call to might_sleep from the fast might_fault code in linux/kernel.h
- checking for pagefault_disable() in the CONFIG_PROVE_LOCKING implementation

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/kernel.h |  1 -
 mm/memory.c            | 14 +++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

include/linux/kernel.h void might_fault(void); · line 201

Quoted 9 lines

include/linux/kernel.h +0 -1

index e96329c..322b065 100644
@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

Quoted 11 lines

include/linux/kernel.h +0 -0

 }
 #endif
 

mm/memory.c +0 -0

index 6dc1882..1b8327b 100644
@@ -4222,13 +4222,17 @@ void might_fault(void)
 	if (segment_eq(get_fs(), KERNEL_DS))
 		return;
 

mm/memory.c void might_fault(void) · line 4228

Quoted 8 lines

mm/memory.c +3 -4

-	might_sleep();
 	/*
-	 * it would be nicer only to annotate paths which are not under
-	 * pagefault_disable, however that requires a larger audit and
-	 * providing helpers like get_user_atomic.
+	 * It would be nicer to annotate paths which are under preempt_disable
+	 * but not under pagefault_disable, however that requires a new flag
+	 * for differentiating between the two.

-rt has this, pagefault_disable() doesn't change the preempt count but pokes at task_struct::pagefault_disable.

Quoted 5 lines

mm/memory.c +3 -1

 	 */
-	if (!in_atomic() && current->mm)
+	if (in_atomic())
+		return;
+

Quoted 8 lines

mm/memory.c +3 -0

+	might_sleep();
+
+	if (current->mm)
 		might_lock_read(&current->mm->mmap_sem);
 }
 EXPORT_SYMBOL(might_fault);

Signature · 1 line

MST

No, I was not assuming that. What I'm trying to say is that a caller that does something like this under a spinlock:
preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable_no_resched

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

Quoted 8 lines · Michael S. Tsirkin

No, I was not assuming that. What I'm trying to say is that a caller that does something like this under a spinlock:
preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable_no_resched

Quoted 4 lines

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

What I see wrong with the above is the preempt_enable_no_resched(). The only place that should be ever used is right before a schedule(), as you don't want to schedule twice (once for the preempt_enable() and then again for the schedule itself).

Remember, in -rt, a spin lock does not disable preemption.

-- Steve

Right but we need to keep it working on upstream as well. If I do preempt_enable under a spinlock upstream won't it try to sleep under spinlock?

Quoted 3 lines · Michael S. Tsirkin

Right but we need to keep it working on upstream as well. If I do preempt_enable under a spinlock upstream won't it try to sleep under spinlock?

No it wont. A spinlock calls preempt_disable implicitly, and a preempt_enable() will not schedule unless preempt_count is zero, which it wont be under a spinlock.

If it did, there would be lots of bugs all over the place because this is done throughout the kernel (a preempt_enable() under a spinlock).

In other words, don't ever use preempt_enable_no_resched().

-- Steve

preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable

OK I get it. So let me correct myself. The simple code that does something like this under a spinlock:

Quoted 6 lines

preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

Quoted 8 lines · Michael S. Tsirkin

OK I get it. So let me correct myself. The simple code that does something like this under a spinlock:

Quoted 6 lines

preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable

Quoted 3 lines

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

I came in mid thread and I don't know the context.

Anyway, the above
looks to me as you just don't want to sleep.

If you try to copy data to
user space that happens not to be currently mapped for any reason, you will get an error. Even if the address space is completely valid. Is that what you want?

-- Steve

OK I get it. So let me correct myself. The simple code that does something like this under a spinlock:

Quoted 6 lines

preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

Quoted 17 lines · Peter Zijlstra

Quoted 9 lines · Michael S. Tsirkin

There are several ways to make sure might_fault
calling function does not sleep.
One is to use it on kernel or otherwise locked memory - apparently nfs/sunrpc does this. As noted by Ingo, this is handled by the migh_fault() implementation in mm/memory.c but not the one in linux/kernel.h so in the current code might_fault() schedules differently depending on CONFIG_PROVE_LOCKING, which is an undesired semantical side effect.

Quoted 6 lines

Another is to call pagefault_disable: in this case the page fault handler will go to fixups processing and we get an error instead of sleeping, so the might_sleep annotation is a false positive. vhost driver wants to do this now in order to reuse socket ops under a spinlock (and fall back on slower thread handler on error).

Quoted 3 lines

Are you using the assumption that spin_lock() implies preempt_disable() implies pagefault_disable()? Note that this assumption isn't valid for -rt where the spinlock becomes preemptible but we'll not disable pagefaults.

No, I was not assuming that. What I'm trying to say is that a caller that does something like this under a spinlock:
preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable_no_resched

is not doing anything wrong and should not get a warning, as long as error is handled correctly later. Right?

Quoted 10 lines

include/linux/kernel.h void might_fault(void); · line 201

Quoted 7 lines

include/linux/kernel.h +0 -1

@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

No it's a bug. Thanks for pointing this out.
OK so I guess it should be might_sleep_if(!in_atomic()) and this means might_fault would have to move from linux/kernel.h to linux/uaccess.h, since in_atomic() is in linux/hardirq.h

Makes sense?

Quoted 16 lines · Michael S. Tsirkin

Quoted 10 lines

include/linux/kernel.h void might_fault(void); · line 201

Quoted 7 lines

include/linux/kernel.h +0 -1

@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

No it's a bug. Thanks for pointing this out.
OK so I guess it should be might_sleep_if(!in_atomic()) and this means might_fault would have to move from linux/kernel.h to linux/uaccess.h, since in_atomic() is in linux/hardirq.h

Quoted 1 line

Makes sense?

So the only difference between PROVE_LOCKING and not should be the might_lock_read() thing; so how about something like this?

---
 include/linux/kernel.h  |  7 ++-----
 include/linux/uaccess.h | 26 ++++++++++++++++++++++++++
 mm/memory.c             | 14 ++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

include/linux/kernel.h +2 -5

index e96329c..70812f4 100644
@@ -194,12 +194,9 @@ extern int _cond_resched(void);
 	})
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void);
+void might_fault_lockdep(void);
 #else
-static inline void might_fault(void)
-{
-	might_sleep();
-}
+static inline void might_fault_lockdep(void) { }
 #endif
 
 extern struct atomic_notifier_head panic_notifier_list;

include/linux/uaccess.h +26 -0

index 5ca0951..50a2cc9 100644
@@ -38,6 +38,32 @@ static inline void pagefault_enable(void)
 	preempt_check_resched();
 }
 
+static inline bool __can_fault(void)
+{
+	/*
+	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
+	 * holding the mmap_sem, this is safe because kernel memory doesn't
+	 * get paged out, therefore we'll never actually fault, and the
+	 * below annotations will generate false positives.
+	 */
+	if (segment_eq(get_fs(), KERNEL_DS))
+		return false;
+
+	if (in_atomic() /* || pagefault_disabled() */)
+		return false;
+
+	return true;
+}
+
+static inline void might_fault(void)
+{
+	if (!__can_fault())
+		return;
+
+	might_sleep();
+	might_fault_lockdep();
+}
+
 #ifndef ARCH_HAS_NOCACHE_UACCESS
 
 static inline unsigned long __copy_from_user_inatomic_nocache(void *to,

mm/memory.c +1 -11

index 6dc1882..266610c 100644
@@ -4211,19 +4211,9 @@ void print_vma_addr(char *prefix, unsigned long ip)
 }
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void)
+void might_fault_lockdep(void)
 {
 	/*
-	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
-	 * holding the mmap_sem, this is safe because kernel memory doesn't
-	 * get paged out, therefore we'll never actually fault, and the
-	 * below annotations will generate false positives.
-	 */
-	if (segment_eq(get_fs(), KERNEL_DS))
-		return;
-
-	might_sleep();
-	/*
 	 * it would be nicer only to annotate paths which are not under
 	 * pagefault_disable, however that requires a larger audit and
 	 * providing helpers like get_user_atomic.

This improves the might_fault annotations used
by uaccess routines:

1. The only reason uaccess routines might sleep is if they fault. Make this explicit for
all architectures.
2. Accesses (e.g through socket ops) to kernel memory with KERNEL_DS like net/sunrpc does will never sleep. Remove an unconditinal might_sleep in the inline might_fault in kernel.h
(used when PROVE_LOCKING is not set).
3. Accesses with pagefault_disable return EFAULT but won't cause caller to sleep.
Check for that and avoid might_sleep when PROVE_LOCKING is set.

I'd like these changes to go in for the benefit of the vhost driver where we want to call socket ops under a spinlock, and fall back on slower thread handler on error.

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines

include/linux/kernel.h void might_fault(void); · line 201

Quoted 7 lines

include/linux/kernel.h +0 -1

@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

Quoted 6 lines

No it's a bug. Thanks for pointing this out.
OK so I guess it should be might_sleep_if(!in_atomic()) and this means might_fault would have to move from linux/kernel.h to linux/uaccess.h, since in_atomic() is in linux/hardirq.h

Makes sense?

So the only difference between PROVE_LOCKING and not should be the might_lock_read() thing; so how about something like this?

---
 include/linux/kernel.h  |  7 ++-----
 include/linux/uaccess.h | 26 ++++++++++++++++++++++++++
 mm/memory.c             | 14 ++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

include/linux/kernel.h +2 -5

index e96329c..70812f4 100644
@@ -194,12 +194,9 @@ extern int _cond_resched(void);
 	})
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void);
+void might_fault_lockdep(void);
 #else
-static inline void might_fault(void)
-{
-	might_sleep();
-}
+static inline void might_fault_lockdep(void) { }
 #endif
 
 extern struct atomic_notifier_head panic_notifier_list;

include/linux/uaccess.h +2 -0

index 5ca0951..50a2cc9 100644
@@ -38,6 +38,32 @@ static inline void pagefault_enable(void)
 	preempt_check_resched();
 }
 
+static inline bool __can_fault(void)
+{

include/linux/uaccess.h +14 -0

+		return false;
+
+	return true;
+}
+
+static inline void might_fault(void)
+{
+	if (!__can_fault())
+		return;
+
+	might_sleep();
+	might_fault_lockdep();
+}
+
 #ifndef ARCH_HAS_NOCACHE_UACCESS
 
 static inline unsigned long __copy_from_user_inatomic_nocache(void *to,

mm/memory.c +1 -11

index 6dc1882..266610c 100644
@@ -4211,19 +4211,9 @@ void print_vma_addr(char *prefix, unsigned long ip)
 }
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void)
+void might_fault_lockdep(void)
 {
 	/*
-	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
-	 * holding the mmap_sem, this is safe because kernel memory doesn't
-	 * get paged out, therefore we'll never actually fault, and the
-	 * below annotations will generate false positives.
-	 */
-	if (segment_eq(get_fs(), KERNEL_DS))
-		return;
-
-	might_sleep();
-	/*
 	 * it would be nicer only to annotate paths which are not under
 	 * pagefault_disable, however that requires a larger audit and
 	 * providing helpers like get_user_atomic.

Quoted 16 lines · Michael S. Tsirkin

This improves the might_fault annotations used
by uaccess routines:

1. The only reason uaccess routines might sleep is if they fault. Make this explicit for
all architectures.
2. Accesses (e.g through socket ops) to kernel memory with KERNEL_DS like net/sunrpc does will never sleep. Remove an unconditinal might_sleep in the inline might_fault in kernel.h
(used when PROVE_LOCKING is not set).
3. Accesses with pagefault_disable return EFAULT but won't cause caller to sleep.
Check for that and avoid might_sleep when PROVE_LOCKING is set.

Quoted 4 lines

I'd like these changes to go in for the benefit of the vhost driver where we want to call socket ops under a spinlock, and fall back on slower thread handler on error.

Hi Michael,

I have recently stumbled over a related topic, which is the highly inconsistent placement of might_fault() or might_sleep() in certain classes of uaccess functions. Your patches seem completely reasonable, but it would be good to also fix the other problem, at least on the architectures we most care about.

Given the most commonly used functions and a couple of architectures I'm familiar with, these are the ones that currently call might_fault()

			x86-32	x86-64	arm	arm64	powerpc	s390	generic

copy_to_user - x - - - x x
copy_from_user - x - - - x x
put_user x x x x x x x
get_user x x x x x x x
__copy_to_user x x - - x - -
__copy_from_user x x - - x - -
__put_user - - x - x - -
__get_user - - x - x - -

WTF?

Calling might_fault() for every __get_user/__put_user is rather expensive because it turns what should be a single instruction (plus fixup) into an external function call.

My feeling is that we should do might_fault() only in access_ok() to get the right balance.

Arnd

Quoted 10 lines

include/linux/uaccess.h +9 -0

+static inline bool __can_fault(void)
+{
+	/*
+	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
+	 * holding the mmap_sem, this is safe because kernel memory doesn't
+	 * get paged out, therefore we'll never actually fault, and the
+	 * below annotations will generate false positives.
+	 */
+	if (segment_eq(get_fs(), KERNEL_DS))

include/linux/uaccess.h static inline void pagefault_enable(void) · line 52

Quoted 3 lines

include/linux/uaccess.h +3 -0

+		return false;
+
+	if (in_atomic() /* || pagefault_disabled() */)

One question here: I'm guessing you put this comment here for illustrative purposes, implying code that will be enabled in -rt?
We don't want it upstream I think, right?

Calling might_fault() for every __get_user/__put_user is rather expensive because it turns what should be a single instruction (plus fixup) into an external function call.

Quoted 3 lines · Arnd Bergmann

Calling might_fault() for every __get_user/__put_user is rather expensive because it turns what should be a single instruction (plus fixup) into an external function call.

We could hide it all behind CONFIG_DEBUG_ATOMIC_SLEEP just like might_sleep() is. I'm not sure there's a point to might_fault() when might_sleep() is a NOP.

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Arnd Bergmann

Calling might_fault() for every __get_user/__put_user is rather expensive because it turns what should be a single instruction (plus fixup) into an external function call.

We could hide it all behind CONFIG_DEBUG_ATOMIC_SLEEP just like might_sleep() is. I'm not sure there's a point to might_fault() when might_sleep() is a NOP.

The patch that you posted gets pretty close.
E.g. I'm testing this now:

(quoted diff) +5 -0

+#define might_fault() do { \
+       if (_might_fault()) \
+               __might_sleep(__FILE__, __LINE__, 0); \
+       might_resched(); \
+} while(0)

So if might_sleep is a NOP, __might_sleep and might_resched are NOPs so compiler will optimize this all out.

However, in a related thread, you pointed out that might_sleep is not a NOP if CONFIG_PREEMPT_VOLUNTARY is set, even without CONFIG_DEBUG_ATOMIC_SLEEP.

Given the most commonly used functions and a couple of architectures I'm familiar with, these are the ones that currently call might_fault()

			x86-32	x86-64	arm	arm64	powerpc	s390	generic

copy_to_user - x - - - x x
copy_from_user - x - - - x x
put_user x x x x x x x
get_user x x x x x x x
__copy_to_user x x - - x - -
__copy_from_user x x - - x - -
__put_user - - x - x - -
__get_user - - x - x - -

WTF?

Quoted 13 lines · Arnd Bergmann

Given the most commonly used functions and a couple of architectures I'm familiar with, these are the ones that currently call might_fault()

                      x86-32  x86-64  arm     arm64   powerpc s390    generic
copy_to_user          -       x       -       -       -       x       x
copy_from_user        -       x       -       -       -       x       x
put_user              x       x       x       x       x       x       x
get_user              x       x       x       x       x       x       x
__copy_to_user        x       x       -       -       x       -       -
__copy_from_user      x       x       -       -       x       -       -
__put_user            -       -       x       -       x       -       -
__get_user            -       -       x       -       x       -       -

Quoted 1 line

WTF?

I think your table is rather screwed - especially on ARM. Tell me - how can __copy_to_user() use might_fault() but copy_to_user() not when copy_to_user() is implemented using __copy_to_user() ? Same for copy_from_user() but the reverse argument - there's nothing special in our copy_from_user() which would make it do might_fault() when __copy_from_user() wouldn't.

The correct position for ARM is: our (__)?(pu|ge)t_user all use might_fault(), but (__)?copy_(to|from)_user do not. Neither does (__)?clear_user. We might want to fix those to use might_fault().

arch/powerpc/include/asm/uaccess.h do { \ · line 184

Quoted 10 lines · Michael S. Tsirkin

arch/powerpc/include/asm/uaccess.h +1 -1

@@ -178,7 +178,7 @@ do {                                                                \
        long __pu_err;                                          \
        __typeof__(*(ptr)) __user *__pu_addr = (ptr);           \
        if (!is_kernel_addr((unsigned long)__pu_addr))          \
-               might_sleep();                                  \
+               might_fault();                                  \
        __chk_user_ptr(ptr);                                    \
        __put_user_size((x), __pu_addr, (size), __pu_err);      \
        __pu_err;                                               \

Another observation:

	if (!is_kernel_addr((unsigned long)__pu_addr))
		might_sleep();

is almost the same as

	might_fault();

except that it does not call might_lock_read().

The version above may have been put there intentionally and correctly, but if you want to replace it with might_fault(), you should remove the "if ()" condition.

Arnd

Quoted 14 lines · Russell King - ARM Linux

Quoted 13 lines · Arnd Bergmann

Given the most commonly used functions and a couple of architectures I'm familiar with, these are the ones that currently call might_fault()

                      x86-32  x86-64  arm     arm64   powerpc s390    generic
copy_to_user          -       x       -       -       -       x       x
copy_from_user        -       x       -       -       -       x       x
put_user              x       x       x       x       x       x       x
get_user              x       x       x       x       x       x       x
__copy_to_user        x       x       -       -       x       -       -
__copy_from_user      x       x       -       -       x       -       -
__put_user            -       -       x       -       x       -       -
__get_user            -       -       x       -       x       -       -

Quoted 8 lines

Quoted 1 line

WTF?

I think your table is rather screwed - especially on ARM. Tell me - how can __copy_to_user() use might_fault() but copy_to_user() not when copy_to_user() is implemented using __copy_to_user() ? Same for copy_from_user() but the reverse argument - there's nothing special in our copy_from_user() which would make it do might_fault() when __copy_from_user() wouldn't.

I think something went wrong with formatting of the tabstobs in the table. I've tried to correct it above to the same version I see on the mailing list.

Quoted 3 lines

The correct position for ARM is: our (__)?(pu|ge)t_user all use might_fault(), but (__)?copy_(to|from)_user do not. Neither does (__)?clear_user. We might want to fix those to use might_fault().

Yes, that sounds like a good idea, especially since they are all implemented out-of-line.

For __get_user()/__put_user(), I would probably do the reverse and make them not call might_fault() though, like we do on most other architectures:

Look at the object code produced for setup_sigframe for instance, it calls might_fault() around 25 times where one should really be enough.

Using
__put_user() instead of put_user() is normally an indication that the author of that function has made performance considerations and move the (trivial) access_ok() call out, but now we add a more expensive call instead.

Arnd

Quoted 9 lines · Michael S. Tsirkin

There are several ways to make sure might_fault
calling function does not sleep.
One is to use it on kernel or otherwise locked memory - apparently nfs/sunrpc does this. As noted by Ingo, this is handled by the migh_fault() implementation in mm/memory.c but not the one in linux/kernel.h so in the current code might_fault() schedules differently depending on CONFIG_PROVE_LOCKING, which is an undesired semantical side effect.

Quoted 6 lines

Another is to call pagefault_disable: in this case the page fault handler will go to fixups processing and we get an error instead of sleeping, so the might_sleep annotation is a false positive. vhost driver wants to do this now in order to reuse socket ops under a spinlock (and fall back on slower thread handler on error).

Are you using the assumption that spin_lock() implies preempt_disable() implies pagefault_disable()? Note that this assumption isn't valid for -rt where the spinlock becomes preemptible but we'll not disable pagefaults.

Quoted 12 lines

Address both issues by:
- dropping the unconditional call to might_sleep from the fast might_fault code in linux/kernel.h
- checking for pagefault_disable() in the CONFIG_PROVE_LOCKING implementation

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/kernel.h |  1 -
 mm/memory.c            | 14 +++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

include/linux/kernel.h void might_fault(void); · line 201

Quoted 9 lines

include/linux/kernel.h +0 -1

index e96329c..322b065 100644
@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

Quoted 9 lines

include/linux/kernel.h +0 -0

 }
 #endif
 

mm/memory.c +0 -0

index 6dc1882..1b8327b 100644
@@ -4222,13 +4222,17 @@ void might_fault(void)
 	if (segment_eq(get_fs(), KERNEL_DS))

mm/memory.c void might_fault(void) · line 4228

Quoted 10 lines

mm/memory.c +3 -4

 		return;
 
-	might_sleep();
 	/*
-	 * it would be nicer only to annotate paths which are not under
-	 * pagefault_disable, however that requires a larger audit and
-	 * providing helpers like get_user_atomic.
+	 * It would be nicer to annotate paths which are under preempt_disable
+	 * but not under pagefault_disable, however that requires a new flag
+	 * for differentiating between the two.

-rt has this, pagefault_disable() doesn't change the preempt count but pokes at task_struct::pagefault_disable.

Quoted 13 lines

mm/memory.c +6 -1

 	 */
-	if (!in_atomic() && current->mm)
+	if (in_atomic())
+		return;
+
+	might_sleep();
+
+	if (current->mm)
 		might_lock_read(&current->mm->mmap_sem);
 }
 EXPORT_SYMBOL(might_fault);

Signature · 1 line

MST

Quoted 16 lines · Michael S. Tsirkin

Quoted 10 lines

include/linux/kernel.h void might_fault(void); · line 201

Quoted 7 lines

include/linux/kernel.h +0 -1

@@ -198,7 +198,6 @@ void might_fault(void);
 #else
 static inline void might_fault(void)
 {
-	might_sleep();

This removes potential resched points for PREEMPT_VOLUNTARY -- was that intentional?

No it's a bug. Thanks for pointing this out.
OK so I guess it should be might_sleep_if(!in_atomic()) and this means might_fault would have to move from linux/kernel.h to linux/uaccess.h, since in_atomic() is in linux/hardirq.h

Quoted 1 line

Makes sense?

So the only difference between PROVE_LOCKING and not should be the might_lock_read() thing; so how about something like this?

---
 include/linux/kernel.h  |  7 ++-----
 include/linux/uaccess.h | 26 ++++++++++++++++++++++++++
 mm/memory.c             | 14 ++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

include/linux/kernel.h +2 -5

index e96329c..70812f4 100644
@@ -194,12 +194,9 @@ extern int _cond_resched(void);
 	})
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void);
+void might_fault_lockdep(void);
 #else
-static inline void might_fault(void)
-{
-	might_sleep();
-}
+static inline void might_fault_lockdep(void) { }
 #endif
 
 extern struct atomic_notifier_head panic_notifier_list;

include/linux/uaccess.h +26 -0

index 5ca0951..50a2cc9 100644
@@ -38,6 +38,32 @@ static inline void pagefault_enable(void)
 	preempt_check_resched();
 }
 
+static inline bool __can_fault(void)
+{
+	/*
+	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
+	 * holding the mmap_sem, this is safe because kernel memory doesn't
+	 * get paged out, therefore we'll never actually fault, and the
+	 * below annotations will generate false positives.
+	 */
+	if (segment_eq(get_fs(), KERNEL_DS))
+		return false;
+
+	if (in_atomic() /* || pagefault_disabled() */)
+		return false;
+
+	return true;
+}
+
+static inline void might_fault(void)
+{
+	if (!__can_fault())
+		return;
+
+	might_sleep();
+	might_fault_lockdep();
+}
+
 #ifndef ARCH_HAS_NOCACHE_UACCESS
 
 static inline unsigned long __copy_from_user_inatomic_nocache(void *to,

mm/memory.c +1 -11

index 6dc1882..266610c 100644
@@ -4211,19 +4211,9 @@ void print_vma_addr(char *prefix, unsigned long ip)
 }
 
 #ifdef CONFIG_PROVE_LOCKING
-void might_fault(void)
+void might_fault_lockdep(void)
 {
 	/*
-	 * Some code (nfs/sunrpc) uses socket ops on kernel memory while
-	 * holding the mmap_sem, this is safe because kernel memory doesn't
-	 * get paged out, therefore we'll never actually fault, and the
-	 * below annotations will generate false positives.
-	 */
-	if (segment_eq(get_fs(), KERNEL_DS))
-		return;
-
-	might_sleep();
-	/*
 	 * it would be nicer only to annotate paths which are not under
 	 * pagefault_disable, however that requires a larger audit and
 	 * providing helpers like get_user_atomic.

arch/powerpc/include/asm/uaccess.h do { \ · line 184

Quoted 10 lines · Michael S. Tsirkin

arch/powerpc/include/asm/uaccess.h +1 -1

@@ -178,7 +178,7 @@ do {                                                                \
        long __pu_err;                                          \
        __typeof__(*(ptr)) __user *__pu_addr = (ptr);           \
        if (!is_kernel_addr((unsigned long)__pu_addr))          \
-               might_sleep();                                  \
+               might_fault();                                  \
        __chk_user_ptr(ptr);                                    \
        __put_user_size((x), __pu_addr, (size), __pu_err);      \
        __pu_err;                                               \

Another observation:

	if (!is_kernel_addr((unsigned long)__pu_addr))
		might_sleep();

is almost the same as

	might_fault();

except that it does not call might_lock_read().

The version above may have been put there intentionally and correctly, but if you want to replace it with might_fault(), you should remove the "if ()" condition.

Arnd

Quoted 21 lines · Arnd Bergmann

arch/powerpc/include/asm/uaccess.h do { \ · line 184

Quoted 10 lines · Michael S. Tsirkin

arch/powerpc/include/asm/uaccess.h +1 -1

@@ -178,7 +178,7 @@ do {                                                                \
        long __pu_err;                                          \
        __typeof__(*(ptr)) __user *__pu_addr = (ptr);           \
        if (!is_kernel_addr((unsigned long)__pu_addr))          \
-               might_sleep();                                  \
+               might_fault();                                  \
        __chk_user_ptr(ptr);                                    \
        __put_user_size((x), __pu_addr, (size), __pu_err);      \
        __pu_err;                                               \

Another observation:

	if (!is_kernel_addr((unsigned long)__pu_addr))
		might_sleep();

is almost the same as

	might_fault();

Quoted 7 lines

except that it does not call might_lock_read().

The version above may have been put there intentionally and correctly, but if you want to replace it with might_fault(), you should remove the "if ()" condition.

Arnd

Well not exactly. The non-inline might_fault checks the current segment, not the address.
I'm guessing this is trying to do the same just without pulling in segment_eq, but I'd like a confirmation from more PPC maintainers.

Guys would you ack

(quoted diff) +1 -2

- 	if (!is_kernel_addr((unsigned long)__pu_addr))
- 		might_fault();
+ 	might_fault();

on top of this patch?

Also, any volunteer to test this (not just test-build)?

Signature · 1 line

MST

So this won't work, unless we add the is_kernel_addr check to might_fault. That will become possible on top of this patchset but let's consider this carefully, and make this a separate patchset, OK?