| Message ID | 20200506062223.30032-9-hch@lst.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [01/15] maccess: unexport probe_kernel_write and probe_user_write | expand |
On Tue, May 5, 2020 at 11:22 PM Christoph Hellwig <hch@lst.de> wrote: > > This matches the convention of always having _unsafe as a suffix, as > well as match the naming of strncpy_from_user_unsafe. Hmm. While renaming them, can we perhaps clarify what the rules are? We now have two different kinds of "unsafe". We have the "unsafe_get_user()" kind of unsafe: the user pointer itself is unsafe because it isn't checked, and you need to use a "user_access_begin()" to verify. It's the new form of "__get_user()". And then we have the strncpy_from_user_unsafe(), which is really more like the "probe_kernel_read()" kind of funtion, in that it's about the context, and not faulting. Honestly, I don't like the "unsafe" in the second case, because there's nothing "unsafe" about the function. It's used in odd contexts. I guess to some degree those are "unsafe" contexts, but I think it might be better to clarify. So while I think using a consistent convention is good, and it's true that there is a difference in the convention between the two cases ("unsafe" at the beginning vs end), one of them is actually about the safety and security of the operation (and we have automated logic these days to verify it on x86), the other has nothing to do with "safety", really. Would it be better to standardize around a "probe_xyz()" naming? Or perhaps a "xyz_nofault()" naming? I'm not a huge fan of the "probe" naming, but it sure is descriptive, which is a really good thing. Another option would be to make it explicitly about _what_ is "unsafe", ie that it's about not having a process context that can be faulted in. But "xyz_unsafe_context()" is much too verbose. "xyz_noctx()" might work. I realize this is nit-picky, and I think the patch series as-is is already an improvement, but I do think our naming in this area is really quite bad. The fact that we have "probe_kernel_read()" but then "strncpy_from_user_unsafe()" for the _same_ conceptual difference really tells me how inconsistent the naming for these kinds of "we can't take page faults" is. No? Linus
On Wed, May 06, 2020 at 10:44:15AM -0700, Linus Torvalds wrote: > So while I think using a consistent convention is good, and it's true > that there is a difference in the convention between the two cases > ("unsafe" at the beginning vs end), one of them is actually about the > safety and security of the operation (and we have automated logic > these days to verify it on x86), the other has nothing to do with > "safety", really. > > Would it be better to standardize around a "probe_xyz()" naming? So: probe_strncpy, probe_strncpy_user, probe_strnlen_user? Sounds weird, but at least it is consistent. > Or perhaps a "xyz_nofault()" naming? That sounds a little better: strncpy_nofault, strncpy_user_nofault, strnlen_user_nofault > I realize this is nit-picky, and I think the patch series as-is is > already an improvement, but I do think our naming in this area is > really quite bad. Always open for improvements :) > The fact that we have "probe_kernel_read()" but then > "strncpy_from_user_unsafe()" for the _same_ conceptual difference > really tells me how inconsistent the naming for these kinds of "we > can't take page faults" is. No? True. If we wanted to do _nofaul, what would the basic read/write versions be?
On Wed, May 6, 2020 at 10:47 AM Christoph Hellwig <hch@lst.de> wrote: > > > The fact that we have "probe_kernel_read()" but then > > "strncpy_from_user_unsafe()" for the _same_ conceptual difference > > really tells me how inconsistent the naming for these kinds of "we > > can't take page faults" is. No? > > True. If we wanted to do _nofaul, what would the basic read/write > versions be? I think "copy_to/from_kernel_nofault()" might be the most consistent model, if we are looking to be kind of consistent with the user access functions.. Unless we want to make "memcpy" be part of the name, but I think that we really want to have that 'from/to' part anyway, which forces the "copy_from/to_xyz" kind of naming. I dunno. I don't want to be too nit-picky, I just would like us to be more consistent and have the naming say what's up without having multiple different versions of the same thing. We've had this same discussion with the nvdimm case, but there the issues are somewhat different (faulting is ok on user addresses - you can sleep - but kernel address faults aren't about the _context_ any more, they are about the data not being safe to access any more) Anybody else? Linus
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 221d703480b6e..77909cafde5a8 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -315,7 +315,7 @@ extern long strncpy_from_kernel_unsafe(char *dst, const void *unsafe_addr, extern long __strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count); extern long strncpy_from_user_unsafe(char *dst, const void __user *unsafe_addr, long count); -extern long strnlen_unsafe_user(const void __user *unsafe_addr, long count); +extern long strnlen_user_unsafe(const void __user *unsafe_addr, long count); /** * probe_kernel_address(): safely attempt to read from a location diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 67ed9655afc51..a7f43c7ec9880 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -1215,7 +1215,7 @@ fetch_store_strlen_user(unsigned long addr) { const void __user *uaddr = (__force const void __user *)addr; - return strnlen_unsafe_user(uaddr, MAX_STRING_SIZE); + return strnlen_user_unsafe(uaddr, MAX_STRING_SIZE); } /* diff --git a/mm/maccess.c b/mm/maccess.c index 683b70518a896..95f2bf97e5ad7 100644 --- a/mm/maccess.c +++ b/mm/maccess.c @@ -258,7 +258,7 @@ long strncpy_from_user_unsafe(char *dst, const void __user *unsafe_addr, } /** - * strnlen_unsafe_user: - Get the size of a user string INCLUDING final NUL. + * strnlen_user_unsafe: - Get the size of a user string INCLUDING final NUL. * @unsafe_addr: The string to measure. * @count: Maximum count (including NUL) * @@ -273,7 +273,7 @@ long strncpy_from_user_unsafe(char *dst, const void __user *unsafe_addr, * Unlike strnlen_user, this can be used from IRQ handler etc. because * it disables pagefaults. */ -long strnlen_unsafe_user(const void __user *unsafe_addr, long count) +long strnlen_user_unsafe(const void __user *unsafe_addr, long count) { mm_segment_t old_fs = get_fs(); int ret;
This matches the convention of always having _unsafe as a suffix, as well as match the naming of strncpy_from_user_unsafe. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/uaccess.h | 2 +- kernel/trace/trace_kprobe.c | 2 +- mm/maccess.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)