On Fri, 24 Mar 2017 12:50:34 +0300
Dan Carpenter <dan.carpen...@oracle.com> wrote:
> The important bit here is that at the end of the function we know that
> since "addr" isn't be NULL, it means we don't need to test
> "addr + offset" for NULL.
>
> The NULL test generates a static checker warning because often something
> else is intended. For example, a common bug looks like:
>
> addr = strchr(buf, ':') + 1;
> if (addr) {
>
> In that example, we intended to test the return of strchr() instead of
> "strchr() + 1".
>
> Techinically, with these static checker warnings you could worry about
> the addition wrapping but the NULL check wouldn't prevent "addr" from
> pointing to some other invalid memory outside the text area. Also
> pointer wrapping is undefined behavior in C.
>
Looks good to me :)
Acked-by: Masami Hiramatsu <mhira...@kernel.org>
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> ---
> v2: more cleanups and changelog
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index d733479a10ee..ad30b5e22bda 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1395,7 +1395,7 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t
> *addr,
> const char *symbol_name, unsigned int offset)
> {
> if ((symbol_name && addr) || (!symbol_name && !addr))
> - goto invalid;
> + return ERR_PTR(-EINVAL);
>
> if (symbol_name) {
> kprobe_lookup_name(symbol_name, addr);
> @@ -1403,12 +1403,7 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t
> *addr,
> return ERR_PTR(-ENOENT);
> }
>
> - addr = (kprobe_opcode_t *)(((char *)addr) + offset);
> - if (addr)
> - return addr;
> -
> -invalid:
> - return ERR_PTR(-EINVAL);
> + return (kprobe_opcode_t *)(((char *)addr) + offset);
> }
>
> static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
--
Masami Hiramatsu <mhira...@kernel.org>
