* Masami Hiramatsu <mhira...@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
