kprobes: Remove an unneeded condition

We know that "addr" is non-NULL here so there is no need to check if "addr + offset" is NULL.

Signed-off-by Dan Carpenter <[email protected]>

kernel/kprobes.c +1 -3

index d733479a10ee..88617178a63e 100644
@@ -1403,9 +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;
+	return (kprobe_opcode_t *)(((char *)addr) + offset);
 
 invalid:
 	return ERR_PTR(-EINVAL);

Quoted 2 lines · On Tue, 21 Mar 2017 23:43:48 +0300 Dan Carpenter <[email protected]>…

We know that "addr" is non-NULL here so there is no need to check if "addr + offset" is NULL.

What about "UINT_MAX - addr + 1 = offset" case on 32bit arch ? :)

Quoted 3 lines · Masami Hiramatsu

On Tue, 21 Mar 2017 23:43:48 +0300
Dan Carpenter <[email protected]> wrote:

Quoted 5 lines

Quoted 2 lines

We know that "addr" is non-NULL here so there is no need to check if "addr + offset" is NULL.

What about "UINT_MAX - addr + 1 = offset" case on 32bit arch ? :)

I think if that's really possible then we are so screwed that a NULL check won't help?

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 <[email protected]>

---
v2: more cleanups and changelog

kernel/kprobes.c +2 -7

index d733479a10ee..ad30b5e22bda 100644
@@ -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)

Quoted 10 lines · On Wed, 22 Mar 2017 12:34:26 +0300 Dan Carpenter <[email protected]>…

Quoted 8 lines · Masami Hiramatsu

Quoted 2 lines · On Tue, 21 Mar 2017 23:43:48 +0300 Dan Carpenter <[email protected]>…

We know that "addr" is non-NULL here so there is no need to check if "addr + offset" is NULL.

What about "UINT_MAX - addr + 1 = offset" case on 32bit arch ? :)

Quoted 2 lines

I think if that's really possible then we are so screwed that a NULL check won't help?

Fair enough. Even if the addr is not NULL, that could be out of text area. Could you add that to patch description, and also clean up the whole function? It seems "invalid" label is no more needed.