Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-09 Thread Suzuki K. Poulose 

On 08/08/2012 03:05 PM, Sebastian Andrzej Siewior wrote:

On 08/08/2012 11:10 AM, Suzuki K. Poulose wrote:

--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask->active_uprobe = NULL;
utask->state = UTASK_RUNNING;
}
- if (uprobe) {
- if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
+ if (!(uprobe->flags & UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe->flags ?
i.e, shouldn't the above line be :

if (uprobe && ! (uprobe->flags & UPROBE_SKIP_SSTEP)) ?


The function starts like this:

  if (!uprobe) {
  if (is_swbp > 0) {
  send_sig(SIGTRAP, current, 0);
  } else {
  instruction_pointer_set(regs, bp_vaddr);
  }
  return;
  }

Which makes uprobe != NULL by the time we get there, no?


My bad, was looking at an older version of the function. Also,
the removal of the if (uprobe), check triggered the above question.

Thanks
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-09 Thread Suzuki K. Poulose 

On 08/08/2012 03:05 PM, Sebastian Andrzej Siewior wrote:

On 08/08/2012 11:10 AM, Suzuki K. Poulose wrote:

--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask-active_uprobe = NULL;
utask-state = UTASK_RUNNING;
}
- if (uprobe) {
- if (!(uprobe-flags  UPROBE_SKIP_SSTEP))
+ if (!(uprobe-flags  UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe-flags ?
i.e, shouldn't the above line be :

if (uprobe  ! (uprobe-flags  UPROBE_SKIP_SSTEP)) ?


The function starts like this:

  if (!uprobe) {
  if (is_swbp  0) {
  send_sig(SIGTRAP, current, 0);
  } else {
  instruction_pointer_set(regs, bp_vaddr);
  }
  return;
  }

Which makes uprobe != NULL by the time we get there, no?


My bad, was looking at an older version of the function. Also,
the removal of the if (uprobe), check triggered the above question.

Thanks
Suzuki

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Oleg Nesterov 
On 08/07, Sebastian Andrzej Siewior wrote:
>
> by the time we get here (after we pass cleanup_ret) uprobe is always is
> set. If it is NULL we leave very early in the code.

Reviewed-by: Oleg Nesterov 

> Signed-off-by: Sebastian Andrzej Siewior 
> ---
>  kernel/events/uprobes.c |   16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 41a2555..c8e5204 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1528,17 +1528,15 @@ cleanup_ret:
>   utask->active_uprobe = NULL;
>   utask->state = UTASK_RUNNING;
>   }
> - if (uprobe) {
> - if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> + if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
>  
> - /*
> -  * cannot singlestep; cannot skip instruction;
> -  * re-execute the instruction.
> -  */
> - instruction_pointer_set(regs, bp_vaddr);
> + /*
> +  * cannot singlestep; cannot skip instruction;
> +  * re-execute the instruction.
> +  */
> + instruction_pointer_set(regs, bp_vaddr);
>  
> - put_uprobe(uprobe);
> - }
> + put_uprobe(uprobe);
>  }
>  
>  /*
> -- 
> 1.7.10.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Sebastian Andrzej Siewior 

On 08/08/2012 11:10 AM, Suzuki K. Poulose wrote:

--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask->active_uprobe = NULL;
utask->state = UTASK_RUNNING;
}
- if (uprobe) {
- if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
+ if (!(uprobe->flags & UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe->flags ?
i.e, shouldn't the above line be :

if (uprobe && ! (uprobe->flags & UPROBE_SKIP_SSTEP)) ?


The function starts like this:

 if (!uprobe) {
 if (is_swbp > 0) {
 send_sig(SIGTRAP, current, 0);
 } else {
 instruction_pointer_set(regs, bp_vaddr);
 }
 return;
 }

Which makes uprobe != NULL by the time we get there, no?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Suzuki K. Poulose 

On 08/07/2012 09:42 PM, Sebastian Andrzej Siewior wrote:

by the time we get here (after we pass cleanup_ret) uprobe is always is
set. If it is NULL we leave very early in the code.

Signed-off-by: Sebastian Andrzej Siewior 
---
  kernel/events/uprobes.c |   16 +++-
  1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 41a2555..c8e5204 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask->active_uprobe = NULL;
utask->state = UTASK_RUNNING;
}
-   if (uprobe) {
-   if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
+   if (!(uprobe->flags & UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe->flags ?
i.e, shouldn't the above line be :

   if (uprobe && ! (uprobe->flags & UPROBE_SKIP_SSTEP)) ?

-   /*
-* cannot singlestep; cannot skip instruction;
-* re-execute the instruction.
-*/
-   instruction_pointer_set(regs, bp_vaddr);
+   /*
+* cannot singlestep; cannot skip instruction;
+* re-execute the instruction.
+*/
+   instruction_pointer_set(regs, bp_vaddr);

-   put_uprobe(uprobe);
-   }
+   put_uprobe(uprobe);
  }


Thanks
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Suzuki K. Poulose 

On 08/07/2012 09:42 PM, Sebastian Andrzej Siewior wrote:

by the time we get here (after we pass cleanup_ret) uprobe is always is
set. If it is NULL we leave very early in the code.

Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
  kernel/events/uprobes.c |   16 +++-
  1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 41a2555..c8e5204 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask-active_uprobe = NULL;
utask-state = UTASK_RUNNING;
}
-   if (uprobe) {
-   if (!(uprobe-flags  UPROBE_SKIP_SSTEP))
+   if (!(uprobe-flags  UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe-flags ?
i.e, shouldn't the above line be :

   if (uprobe  ! (uprobe-flags  UPROBE_SKIP_SSTEP)) ?

-   /*
-* cannot singlestep; cannot skip instruction;
-* re-execute the instruction.
-*/
-   instruction_pointer_set(regs, bp_vaddr);
+   /*
+* cannot singlestep; cannot skip instruction;
+* re-execute the instruction.
+*/
+   instruction_pointer_set(regs, bp_vaddr);

-   put_uprobe(uprobe);
-   }
+   put_uprobe(uprobe);
  }


Thanks
Suzuki

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Sebastian Andrzej Siewior 

On 08/08/2012 11:10 AM, Suzuki K. Poulose wrote:

--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,17 +1528,15 @@ cleanup_ret:
utask-active_uprobe = NULL;
utask-state = UTASK_RUNNING;
}
- if (uprobe) {
- if (!(uprobe-flags  UPROBE_SKIP_SSTEP))
+ if (!(uprobe-flags  UPROBE_SKIP_SSTEP))


Shouldn't we check uprobe != NULL before we check the uprobe-flags ?
i.e, shouldn't the above line be :

if (uprobe  ! (uprobe-flags  UPROBE_SKIP_SSTEP)) ?


The function starts like this:

 if (!uprobe) {
 if (is_swbp  0) {
 send_sig(SIGTRAP, current, 0);
 } else {
 instruction_pointer_set(regs, bp_vaddr);
 }
 return;
 }

Which makes uprobe != NULL by the time we get there, no?

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] uprobes: remove check for uprobe variable in handle_swbp()

2012-08-08 Thread Oleg Nesterov 
On 08/07, Sebastian Andrzej Siewior wrote:

 by the time we get here (after we pass cleanup_ret) uprobe is always is
 set. If it is NULL we leave very early in the code.

Reviewed-by: Oleg Nesterov o...@redhat.com

 Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
 ---
  kernel/events/uprobes.c |   16 +++-
  1 file changed, 7 insertions(+), 9 deletions(-)
 
 diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
 index 41a2555..c8e5204 100644
 --- a/kernel/events/uprobes.c
 +++ b/kernel/events/uprobes.c
 @@ -1528,17 +1528,15 @@ cleanup_ret:
   utask-active_uprobe = NULL;
   utask-state = UTASK_RUNNING;
   }
 - if (uprobe) {
 - if (!(uprobe-flags  UPROBE_SKIP_SSTEP))
 + if (!(uprobe-flags  UPROBE_SKIP_SSTEP))
  
 - /*
 -  * cannot singlestep; cannot skip instruction;
 -  * re-execute the instruction.
 -  */
 - instruction_pointer_set(regs, bp_vaddr);
 + /*
 +  * cannot singlestep; cannot skip instruction;
 +  * re-execute the instruction.
 +  */
 + instruction_pointer_set(regs, bp_vaddr);
  
 - put_uprobe(uprobe);
 - }
 + put_uprobe(uprobe);
  }
  
  /*
 -- 
 1.7.10.4
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/