| Message ID | 20200506062223.30032-16-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/15] maccess: unexport probe_kernel_write and probe_user_write | expand |
On Tue, May 5, 2020 at 11:23 PM Christoph Hellwig <hch@lst.de> wrote: > > +#define arch_kernel_read(dst, src, type, err_label) \ > + __get_user_size(*((type *)dst), (__force type __user *)src, \ > + sizeof(type), __kr_err); \ .. > +#define arch_kernel_write(dst, src, type, err_label) \ > + __put_user_size(*((type *)(src)), (__force type __user *)(dst), \ > + sizeof(type), err_label) My private tree no longer has those __get/put_user_size() things, because "unsafe_get/put_user()" is the only thing that remains with my conversion to asm goto. And we're actively trying to get rid of the whole __get_user() mess. Admittedly "__get_user_size()" is just the internal helper that doesn't have the problem, but it really is an internal helper for a legacy operation, and the new op that uses it is that "unsafe_get_user()". Also, because you use __get_user_size(), you then have to duplicate the error handling logic that we already have in unsafe_get_user(). IOW - is there some reason why you didn't just make these use "unsafe_get/put_user()" directly, and avoid both of those issues? Linus
On Wed, May 06, 2020 at 10:51:51AM -0700, Linus Torvalds wrote: > My private tree no longer has those __get/put_user_size() things, > because "unsafe_get/put_user()" is the only thing that remains with my > conversion to asm goto. > > And we're actively trying to get rid of the whole __get_user() mess. > Admittedly "__get_user_size()" is just the internal helper that > doesn't have the problem, but it really is an internal helper for a > legacy operation, and the new op that uses it is that > "unsafe_get_user()". > > Also, because you use __get_user_size(), you then have to duplicate > the error handling logic that we already have in unsafe_get_user(). > > IOW - is there some reason why you didn't just make these use > "unsafe_get/put_user()" directly, and avoid both of those issues? That was the first prototype, and or x86 it works great, just the __user cases in maccess.c are a little ugly. And they point to the real problem - for architectures like sparc and s390 that use an entirely separate address space for the kernel vs userspace I dont think just use unsafe_{get,put}_user will work, as they need different instructions. Btw, where is you magic private tree and what is the plan for it?
On Wed, May 6, 2020 at 11:15 AM Christoph Hellwig <hch@lst.de> wrote: > > That was the first prototype, and or x86 it works great, just the > __user cases in maccess.c are a little ugly. And they point to > the real problem - for architectures like sparc and s390 that use > an entirely separate address space for the kernel vs userspace > I dont think just use unsafe_{get,put}_user will work, as they need > different instructions. Oh, absolutely. I did *NOT* mean that you'd use "unsafe_get_user()" as the actual interface. I just meant that as an implementation detail on x86, using "unsafe_get_user()" instead of "__get_user_size()" internally both simplifies the implementation, and means that it doesn't clash horribly with my local changes. Btw, that brings up another issue: so that people can't mis-use those kernel accessors and use them for user addresses, they probably should actually do something like if ((long)addr >= 0) goto error_label; on x86. IOW, have the "strict" kernel pointer behavior. Otherwise somebody will start using them for user pointers, and it will happen to work on old x86 without CLAC/STAC support. Of course, maybe CLAC/STAC is so common these days (at least with developers) that we don't have to worry about it. > Btw, where is you magic private tree and what is the plan for it? I don't want to make it a public branch, but here's a private bundle. It's based on top of my current -git tree - I just maintain a separate tree that I keep up-to-date locally for testing. My "normal" tree I do build tests on (allmodconfig etc), this separate tree I keep around to actually do boot tests on, and I end up using "current Linus' tree plus this" as the code I actually run om my main desktop. But this *ONLY* works with clang, and only with current HEAD of the clang development tree. So it's almosty entirely useless to anybody else right now. You literally have to clone the llvm tree, build your own clang, and install it to even _build_ this. I'm not planning on going any further than my local testing until the whole thing calms down. The llvm tree still has some known bugs in the asm goto with output area, and I want there to be an actual release of it before I actually merge anything like this (and I need to do the small extra work to then have that conditional "does the compiler support asm goto with outputs" so that it works with gcc too). But here you see what it is, if you want to. __get_user_size() technically still exists, but it has the "target branch" semantics in here, so your patch clashes badly with it. (Well, those are the semantics you want, so "badly" may not be the right word, but basically it means that if you _had_ used unsafe_get_user(), there wouldn't have been those kinds of semantic conflicts). Linus
On Wed, May 06, 2020 at 12:01:32PM -0700, Linus Torvalds wrote: > Oh, absolutely. I did *NOT* mean that you'd use "unsafe_get_user()" as > the actual interface. I just meant that as an implementation detail on > x86, using "unsafe_get_user()" instead of "__get_user_size()" > internally both simplifies the implementation, and means that it > doesn't clash horribly with my local changes. I had a version that just wrapped them, but somehow wasn't able to make it work due to all the side effects vs macros issues. Maybe I need to try again, the current version seemed like a nice way out as it avoided a lot of the silly casting. > Btw, that brings up another issue: so that people can't mis-use those > kernel accessors and use them for user addresses, they probably should > actually do something like > > if ((long)addr >= 0) > goto error_label; > > on x86. IOW, have the "strict" kernel pointer behavior. > > Otherwise somebody will start using them for user pointers, and it > will happen to work on old x86 without CLAC/STAC support. > > Of course, maybe CLAC/STAC is so common these days (at least with > developers) that we don't have to worry about it. The actual public routines (probe_kernel_read and co) get these checks through probe_kernel_read_allowed, which is implemented by the x86 code. Doing this for every 1-8 byte access might be a little slow, though. Do you really fear drivers starting to use the low-level helper? Maybe we need to move those into a different header than <asm/uaccess.h> that makes it more clear that they are internal? > But here you see what it is, if you want to. __get_user_size() > technically still exists, but it has the "target branch" semantics in > here, so your patch clashes badly with it. The target branch semantics actually are what I want, that is how the maccess code is structured. This is the diff I'd need for the calling conventions in your bundle: diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 765e18417b3ba..d1c8aacedade1 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -526,14 +526,8 @@ do { \ #define HAVE_ARCH_PROBE_KERNEL #define arch_kernel_read(dst, src, type, err_label) \ -do { \ - int __kr_err; \ - \ __get_user_size(*((type *)dst), (__force type __user *)src, \ - sizeof(type), __kr_err); \ - if (unlikely(__kr_err)) \ - goto err_label; \ -} while (0) + sizeof(type), err_label); \ #define arch_kernel_write(dst, src, type, err_label) \ __put_user_size(*((type *)(src)), (__force type __user *)(dst), \
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index d8f283b9a569c..765e18417b3ba 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -523,5 +523,21 @@ do { \ unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label); \ } while (0) +#define HAVE_ARCH_PROBE_KERNEL + +#define arch_kernel_read(dst, src, type, err_label) \ +do { \ + int __kr_err; \ + \ + __get_user_size(*((type *)dst), (__force type __user *)src, \ + sizeof(type), __kr_err); \ + if (unlikely(__kr_err)) \ + goto err_label; \ +} while (0) + +#define arch_kernel_write(dst, src, type, err_label) \ + __put_user_size(*((type *)(src)), (__force type __user *)(dst), \ + sizeof(type), err_label) + #endif /* _ASM_X86_UACCESS_H */
Provide arch_kernel_read and arch_kernel_write routines to implement the maccess routines without messing with set_fs and without stac/clac that opens up access to user space. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/x86/include/asm/uaccess.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)