| Message ID | e0a980a4dc7a2551183dd5cb30f46eafdbee390c.1615398265.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc: Cleanup of uaccess.h and adding asm goto for get_user() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (91966823812efbd175f904599e5cf2a854b39809) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Christophe, > Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in > __get_user/__put_user on kernel addresses") added a check to not call > might_sleep() on kernel addresses. This was to enable the use of > __get_user() in the alignment exception handler for any address. > > Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation") > added a check of the address space in might_fault(), based on > set_fs() logic. But this didn't solve the powerpc alignment exception > case as it didn't call set_fs(KERNEL_DS). > > Nowadays, set_fs() is gone, previous patch fixed the alignment > exception handler and __get_user/__put_user are not supposed to be > used anymore to read kernel memory. > > Therefore the is_kernel_addr() check has become useless and can be > removed. While I agree that __get_user/__put_user should not be used on kernel memory, I'm not sure that we have covered every case where they might be used on kernel memory yet. I did a git grep for __get_user - there are several callers in arch/powerpc and it looks like at least lib/sstep.c might be using __get_user to read kernel memory while single-stepping. I am not sure if might_sleep has got logic to cover the powerpc case - it uses uaccess_kernel, but we don't supply a definition for that on powerpc, so if we do end up calling __get_user on a kernel address, I think we might now throw a warning. (Unless we are saved by pagefault_disabled()?) (But I haven't tested this yet, so it's possible I misunderstood something.) Do you expect any consequences if we've missed a case where __(get|put)_user is called on a kernel address because it hasn't been converted to use better helpers yet? Kind regards, Daniel > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/uaccess.h | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h > index eaa828a6a419..c4bbc64758a0 100644 > --- a/arch/powerpc/include/asm/uaccess.h > +++ b/arch/powerpc/include/asm/uaccess.h > @@ -77,8 +77,7 @@ __pu_failed: \ > __typeof__(*(ptr)) __pu_val = (x); \ > __typeof__(size) __pu_size = (size); \ > \ > - if (!is_kernel_addr((unsigned long)__pu_addr)) \ > - might_fault(); \ > + might_fault(); \ > __chk_user_ptr(__pu_addr); \ > __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \ > \ > @@ -238,12 +237,12 @@ do { \ > __typeof__(size) __gu_size = (size); \ > \ > __chk_user_ptr(__gu_addr); \ > - if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \ > + if (do_allow) { \ > might_fault(); \ > - if (do_allow) \ > __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \ > - else \ > + } else { \ > __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ > + } \ > (x) = (__typeof__(*(ptr)))__gu_val; \ > \ > __gu_err; \ > -- > 2.25.0
Daniel Axtens <dja@axtens.net> writes: > Hi Christophe, > >> Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in >> __get_user/__put_user on kernel addresses") added a check to not call >> might_sleep() on kernel addresses. This was to enable the use of >> __get_user() in the alignment exception handler for any address. >> >> Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation") >> added a check of the address space in might_fault(), based on >> set_fs() logic. But this didn't solve the powerpc alignment exception >> case as it didn't call set_fs(KERNEL_DS). >> >> Nowadays, set_fs() is gone, previous patch fixed the alignment >> exception handler and __get_user/__put_user are not supposed to be >> used anymore to read kernel memory. >> >> Therefore the is_kernel_addr() check has become useless and can be >> removed. > > While I agree that __get_user/__put_user should not be used on kernel > memory, I'm not sure that we have covered every case where they might be > used on kernel memory yet. I did a git grep for __get_user - there are > several callers in arch/powerpc and it looks like at least lib/sstep.c > might be using __get_user to read kernel memory while single-stepping. > > I am not sure if might_sleep has got logic to cover the powerpc case - > it uses uaccess_kernel, but we don't supply a definition for that on > powerpc, so if we do end up calling __get_user on a kernel address, I > think we might now throw a warning. (Unless we are saved by > pagefault_disabled()?) Ah, I just re-read some of my earlier emails and was reminded that yes, if we are calling __get/put, we must have disabled page faults. So yes, this is good. > > (But I haven't tested this yet, so it's possible I misunderstood > something.) > > Do you expect any consequences if we've missed a case where > __(get|put)_user is called on a kernel address because it hasn't been > converted to use better helpers yet? > > Kind regards, > Daniel > >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/include/asm/uaccess.h | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h >> index eaa828a6a419..c4bbc64758a0 100644 >> --- a/arch/powerpc/include/asm/uaccess.h >> +++ b/arch/powerpc/include/asm/uaccess.h >> @@ -77,8 +77,7 @@ __pu_failed: \ >> __typeof__(*(ptr)) __pu_val = (x); \ >> __typeof__(size) __pu_size = (size); \ >> \ >> - if (!is_kernel_addr((unsigned long)__pu_addr)) \ >> - might_fault(); \ >> + might_fault(); \ >> __chk_user_ptr(__pu_addr); \ >> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \ >> \ >> @@ -238,12 +237,12 @@ do { \ >> __typeof__(size) __gu_size = (size); \ >> \ >> __chk_user_ptr(__gu_addr); \ >> - if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \ >> + if (do_allow) { \ >> might_fault(); \ >> - if (do_allow) \ >> __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \ >> - else \ >> + } else { \ >> __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ >> + } \ One microscopic nit: these changes throw the '\'s further out of alignment. Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel >> (x) = (__typeof__(*(ptr)))__gu_val; \ >> \ >> __gu_err; \ >> -- >> 2.25.0
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h index eaa828a6a419..c4bbc64758a0 100644 --- a/arch/powerpc/include/asm/uaccess.h +++ b/arch/powerpc/include/asm/uaccess.h @@ -77,8 +77,7 @@ __pu_failed: \ __typeof__(*(ptr)) __pu_val = (x); \ __typeof__(size) __pu_size = (size); \ \ - if (!is_kernel_addr((unsigned long)__pu_addr)) \ - might_fault(); \ + might_fault(); \ __chk_user_ptr(__pu_addr); \ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \ \ @@ -238,12 +237,12 @@ do { \ __typeof__(size) __gu_size = (size); \ \ __chk_user_ptr(__gu_addr); \ - if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \ + if (do_allow) { \ might_fault(); \ - if (do_allow) \ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \ - else \ + } else { \ __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \ + } \ (x) = (__typeof__(*(ptr)))__gu_val; \ \ __gu_err; \
Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in __get_user/__put_user on kernel addresses") added a check to not call might_sleep() on kernel addresses. This was to enable the use of __get_user() in the alignment exception handler for any address. Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation") added a check of the address space in might_fault(), based on set_fs() logic. But this didn't solve the powerpc alignment exception case as it didn't call set_fs(KERNEL_DS). Nowadays, set_fs() is gone, previous patch fixed the alignment exception handler and __get_user/__put_user are not supposed to be used anymore to read kernel memory. Therefore the is_kernel_addr() check has become useless and can be removed. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/include/asm/uaccess.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)