* Re: [PATCH] kprobes: Remove an unneeded condition
2017-03-21 20:43 [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
@ 2017-03-21 22:55 ` Masami Hiramatsu
2017-03-22 9:34 ` Dan Carpenter
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2017-03-21 22:55 UTC (permalink / raw)
To: kernel-janitors
On Tue, 21 Mar 2017 23:43:48 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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 ? :)
Thanks,
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index d733479a10ee..88617178a63e 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -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);
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kprobes: Remove an unneeded condition
2017-03-21 20:43 [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
2017-03-21 22:55 ` Masami Hiramatsu
@ 2017-03-22 9:34 ` Dan Carpenter
2017-03-23 0:15 ` Masami Hiramatsu
2017-03-23 9:22 ` [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
3 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2017-03-22 9:34 UTC (permalink / raw)
To: kernel-janitors
On Wed, Mar 22, 2017 at 07:55:38AM +0900, Masami Hiramatsu wrote:
> On Tue, 21 Mar 2017 23:43:48 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > 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?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobes: Remove an unneeded condition
2017-03-21 20:43 [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
2017-03-21 22:55 ` Masami Hiramatsu
2017-03-22 9:34 ` Dan Carpenter
@ 2017-03-23 0:15 ` Masami Hiramatsu
2017-03-24 9:50 ` Dan Carpenter
2017-03-23 9:22 ` [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
3 siblings, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2017-03-23 0:15 UTC (permalink / raw)
To: kernel-janitors
On Wed, 22 Mar 2017 12:34:26 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Mar 22, 2017 at 07:55:38AM +0900, Masami Hiramatsu wrote:
> > On Tue, 21 Mar 2017 23:43:48 +0300
> > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > > 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?
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.
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] kprobes: cleanup _kprobe_addr()
2017-03-23 0:15 ` Masami Hiramatsu
@ 2017-03-24 9:50 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2017-03-24 9:50 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Masami Hiramatsu
Cc: Anil S Keshavamurthy, David S. Miller, linux-kernel,
kernel-janitors
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)
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2] kprobes: cleanup _kprobe_addr()
@ 2017-03-24 9:50 ` Dan Carpenter
0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2017-03-24 9:50 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Masami Hiramatsu
Cc: Anil S Keshavamurthy, David S. Miller, linux-kernel,
kernel-janitors
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)
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] kprobes: cleanup _kprobe_addr()
2017-03-24 9:50 ` Dan Carpenter
@ 2017-03-27 2:19 ` Masami Hiramatsu
-1 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2017-03-27 2:19 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ananth N Mavinakayanahalli, Anil S Keshavamurthy, David S. Miller,
linux-kernel, kernel-janitors
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] kprobes: cleanup _kprobe_addr()
@ 2017-03-27 2:19 ` Masami Hiramatsu
0 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2017-03-27 2:19 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ananth N Mavinakayanahalli, Anil S Keshavamurthy, David S. Miller,
linux-kernel, kernel-janitors
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobes: Remove an unneeded condition
2017-03-21 20:43 [PATCH] kprobes: Remove an unneeded condition Dan Carpenter
` (2 preceding siblings ...)
2017-03-23 0:15 ` Masami Hiramatsu
@ 2017-03-23 9:22 ` Dan Carpenter
3 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2017-03-23 9:22 UTC (permalink / raw)
To: kernel-janitors
On Thu, Mar 23, 2017 at 09:15:16AM +0900, Masami Hiramatsu wrote:
> On Wed, 22 Mar 2017 12:34:26 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > On Wed, Mar 22, 2017 at 07:55:38AM +0900, Masami Hiramatsu wrote:
> > > On Tue, 21 Mar 2017 23:43:48 +0300
> > > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > > 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?
>
> 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.
Sure. I'll resend.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread