Hi Ingo, On Thu, 9 May 2019 11:14:08 +0200 Ingo Molnar <[email protected]> wrote:
> * Masami Hiramatsu <[email protected]> wrote: > > > +static __always_inline long > > +probe_read_common(void *dst, const void __user *src, size_t size) > > +{ > > + long ret; > > + > > + pagefault_disable(); > > + ret = __copy_from_user_inatomic(dst, src, size); > > + pagefault_enable(); > > + > > + return ret ? -EFAULT : 0; > > +} > > Empty line before return statement: good. > > > +long __weak probe_user_read(void *dst, const void __user *src, size_t size) > > + __attribute__((alias("__probe_user_read"))); > > + > > +long __probe_user_read(void *dst, const void __user *src, size_t size) > > +{ > > + long ret = -EFAULT; > > + mm_segment_t old_fs = get_fs(); > > + > > + set_fs(USER_DS); > > + if (access_ok(src, size)) > > + ret = probe_read_common(dst, src, size); > > + set_fs(old_fs); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(probe_user_read); > > No empty line before return statement: not good. OK, I'll fix that. > > > +long strncpy_from_unsafe_user(char *dst, const void __user *unsafe_addr, > > + long count) > > +{ > > + mm_segment_t old_fs = get_fs(); > > + long ret; > > + > > + if (unlikely(count <= 0)) > > + return 0; > > + > > + set_fs(USER_DS); > > + pagefault_disable(); > > + ret = strncpy_from_user(dst, unsafe_addr, count); > > + pagefault_enable(); > > + set_fs(old_fs); > > + if (ret >= count) { > > + ret = count; > > + dst[ret - 1] = '\0'; > > + } else if (ret > 0) > > + ret++; > > + return ret; > > +} > > Ditto. Also unbalanced curly braces. Yeah, thanks! > > > + > > +/** > > + * strnlen_unsafe_user: - Get the size of a user string INCLUDING final > > NUL. > > + * @unsafe_addr: The string to measure. > > + * @count: Maximum count (including NUL character) > > + * > > + * Get the size of a NUL-terminated string in user space without pagefault. > > + * > > + * Returns the size of the string INCLUDING the terminating NUL. > > These phrases exist: > > 'Terminating NULL' > 'NULL character' > > And we also sometimes talk about 'nil' - but I don't think there's such > thing as a 'NUL character'? OK, it should be "NUL" or "Null character" ( https://en.wikipedia.org/wiki/Null_character ) > > I realize that this was probably cloned from existing lib/strnlen_user.c > code, but still. ;-) > > > + * > > + * If the string is too long, returns a number larger than @count. User > > + * has to check the return value against "> count". > > + * On exception (or invalid count), returns 0. > > + * > > + * Unlike strnlen_user, this can be used from IRQ handler etc. because > > + * it disables pagefaults. > > 'can be used from IRQ handlers' OK. > > > + */ > > +long strnlen_unsafe_user(const void __user *unsafe_addr, long count) > > +{ > > + mm_segment_t old_fs = get_fs(); > > + int ret; > > + > > + set_fs(USER_DS); > > + pagefault_disable(); > > + ret = strnlen_user(unsafe_addr, count); > > + pagefault_enable(); > > + set_fs(old_fs); > > + return ret; > > +} > > Same problem as before. OK, I'll fix that. Thank you! > > Thanks, > > Ingo -- Masami Hiramatsu <[email protected]>
