Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1608288 > unrolled thread

[PATCH v2] kprobes: cleanup _kprobe_addr()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] kprobes: cleanup _kprobe_addr() Dan Carpenter <dan.carpenter@oracle.com> - 2017-03-24 11:00 +0100
    Re: [PATCH v2] kprobes: cleanup _kprobe_addr() Masami Hiramatsu <mhiramat@kernel.org> - 2017-03-27 04:30 +0200

#1608288 — [PATCH v2] kprobes: cleanup _kprobe_addr()

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.

Signed-off-by: Dan Carpenter <dan.carpenter@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)

[toc] | [next] | [standalone]

#1609430

On Fri, 24 Mar 2017 12:50:34 +0300
Dan Carpenter <dan.carpenter@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 <mhiramat@kernel.org>

> Signed-off-by: Dan Carpenter <dan.carpenter@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 <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web