On Tue, 14 May 2019 09:24:26 +0200
Ingo Molnar <mi...@kernel.org> wrote:
>
> * 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.
OK.
>
> > +/*
> > + * 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.
Agreed. My bad habit, maybe '__dest' would better.
> 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;
OK.
>
> Third, hiding the get_loc_data() call within variable initialization is
> bad style - we usually only put 'trivial' (constant) initializations
> there.
Hmm, it just decodes the location address from offset and start address.
Shouldn't it a trivial?
> Fourth, 'dst' is independent of 'maxlen', so it should probably
> calculated *after* maxlen.
Ah, OK. I see what you pointed.
>
> I.e. the whole sequence should be:
>
>
> maxlen = get_loc_len(*(u32 *)dest);
> if (unlikely(!maxlen))
> return -ENOMEM;
>
> dst = get_loc_data(dest, base);
OK, in this case we can skip this conversion if maxlen == 0.
>
> 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 *.
OK, I'll use void* for that.
>
> 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.
OK, I'll update others too.
Thank you,
>
> Thanks,
>
> Ingo
--
Masami Hiramatsu <mhira...@kernel.org>
