Messages in this thread | | | Date | Tue, 14 May 2019 09:24:26 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH -tip v8 3/6] tracing/probe: Add ustring type for user-space string |
| |
* Masami Hiramatsu <mhiramat@kernel.org> wrote:
> +/* Return the length of string -- including null terminal byte */ > +static nokprobe_inline int > +fetch_store_strlen_user(unsigned long addr) > +{ > + return strnlen_unsafe_user((__force const void __user *)addr, > + MAX_STRING_SIZE);
Pointless line break that doesn't improve readability.
> +/* > + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf > + * with max length and relative data location. > + */ > +static nokprobe_inline int > +fetch_store_string_user(unsigned long addr, void *dest, void *base) > +{ > + const void __user *uaddr = (__force const void __user *)addr; > + int maxlen = get_loc_len(*(u32 *)dest); > + u8 *dst = get_loc_data(dest, base); > + long ret; > + > + if (unlikely(!maxlen)) > + return -ENOMEM; > + ret = strncpy_from_unsafe_user(dst, uaddr, maxlen); > + > + if (ret >= 0) > + *(u32 *)dest = make_data_loc(ret, (void *)dst - base); > + > return ret;
Firstly, why is there a 'dest' and a 'dst' variable name as well - the two are very similar and the difference not explained at all.
Secondly, a style nit: if you group statements then please group statements based on the usual logic - which is the group them by the flow of logic. In the above case you grouped the 'maxlen' check with the strncpy_from_unsafe_user() call, while the grouping should be the other way around:
if (unlikely(!maxlen)) return -ENOMEM;
ret = strncpy_from_unsafe_user(dst, uaddr, maxlen); if (ret >= 0) *(u32 *)dest = make_data_loc(ret, (void *)dst - base);
return ret;
Third, hiding the get_loc_data() call within variable initialization is bad style - we usually only put 'trivial' (constant) initializations there.
Fourth, 'dst' is independent of 'maxlen', so it should probably calculated *after* maxlen.
I.e. the whole sequence should be:
maxlen = get_loc_len(*(u32 *)dest); if (unlikely(!maxlen)) return -ENOMEM;
dst = get_loc_data(dest, base);
ret = strncpy_from_unsafe_user(dst, uaddr, maxlen); if (ret >= 0) *(u32 *)dest = make_data_loc(ret, (void *)dst - base);
return ret;
Fifth, we don't actually dereference 'dst', do we? So the whole type casting to 'void *' could be avoided by declaring 'dst' (or whatever its new, clearer name is) not as u8 *, but as void *.
I.e. these are five problems in a short sequence of code, which it sad to see in a v8 submission. :-/
Please review the other patches and the whole code base for similar mishaps and small details as well.
Thanks,
Ingo
| |