Messages in this thread | | | Date | Tue, 14 May 2019 19:10:55 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH -tip v8 3/6] tracing/probe: Add ustring type for user-space string |
| |
On Tue, 14 May 2019 09:24:26 +0200 Ingo Molnar <mingo@kernel.org> wrote:
> > * 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.
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 <mhiramat@kernel.org>
| |