Re: [PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines
Change utrace_reset() to do user_disable_single_step(). Alternatively we could change ptrace layer, but I think this is not ptrace specific. Yes, it's right to fix this in the utrace layer. But I'm not sure that's the right place in the code to fix it. The expectation is that either we'll get to finish_resume_report, which calls user_*_step, or that utrace_control is resuming us without any expectation of a report. For UTRACE_RESUME in that case, utrace_control calls user_disable_single_step. So probably the UTRACE_DETACH case there should just do it to. Thanks, Roland
Re: [PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines
On 09/21, Roland McGrath wrote: Change utrace_reset() to do user_disable_single_step(). Alternatively we could change ptrace layer, but I think this is not ptrace specific. Yes, it's right to fix this in the utrace layer. But I'm not sure that's the right place in the code to fix it. The expectation is that either we'll get to finish_resume_report, which calls user_*_step, or that utrace_control is resuming us without any expectation of a report. For UTRACE_RESUME in that case, utrace_control calls user_disable_single_step. So probably the UTRACE_DETACH case there should just do it to. Yes, initially I was going to change utrace_control(DETACH), and this certainly looks more clean. I was worried about the case when the TIF_SINGLESTEP tracee detaches itself and escapes finish_resume_report()-user_disable_single_step(), say, utrace_report_exec(). But probably we can ignore this. Another concern was implicit detach, but thinking more I do not see why utrace_resume() is better. OK, I'll do some testing and resend right now. In UTRACE_DETACH case reset can be true but the tracee is running. But I don't think it makes sense to check target-exit_state == 0, correct? Oleg.
Re: [PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines
On 09/21, Roland McGrath wrote: I was worried about the case when the TIF_SINGLESTEP tracee detaches itself and escapes finish_resume_report()-user_disable_single_step(), say, utrace_report_exec(). But probably we can ignore this. No, I think that is indeed a problem. If no engine is still attached whose last callback returned UTRACE_SINGLESTEP, we should never return to user mode with single-step enabled. OK, so what should we do for now? Without the multitracing utrace_resume()-user_disable_single_step() fixes the problem. Otherwise we probably need ENGINE_WANTS_SINGLESTEP. Another concern was implicit detach, but thinking more I do not see why utrace_resume() is better. OK, I'll do some testing and resend right now. In UTRACE_DETACH case reset can be true but the tracee is running. But I don't think it makes sense to check target-exit_state == 0, correct? I think it's OK if it's running with -exit_state set (or even with just PF_EXITING set), i.e. already in kernel mode and never going back to user mode. (Then it's essentially equivalent to calling user_*_step while racing with a SIGKILL death, which has to be OK.) Any other kind of running would not be OK. Yes, my concern was running and !current, not exiting. This is OK on x86 but user_disable_single_step() is arch specific. Oleg.
Re: [PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines
On 09/22, Oleg Nesterov wrote:
On 09/21, Roland McGrath wrote:
I was worried about the case when the TIF_SINGLESTEP tracee detaches
itself and escapes finish_resume_report()-user_disable_single_step(),
say, utrace_report_exec(). But probably we can ignore this.
No, I think that is indeed a problem. If no engine is still attached
whose last callback returned UTRACE_SINGLESTEP, we should never return
to user mode with single-step enabled.
OK, so what should we do for now?
Without the multitracing utrace_resume()-user_disable_single_step()
fixes the problem. Otherwise we probably need ENGINE_WANTS_SINGLESTEP.
As expected, the patch below equally fixes the problem and passes
ptrace-tests.
Which one do you prefer?
Note that, since we are going to change UTRACE_RESUME to remove
ENGINE_STOP from utrace_flags, we can probably unify DETACH/RESUME
case UTRACE_DETACH:
mark_engine_detached(engine);
reset = reset || utrace_do_stop(target, utrace);
if (!reset) {
/*
* As in utrace_set_events(), this barrier ensures
* that our engine-flags changes have hit before we
* examine utrace-reporting, pairing with the barrier
* in start_callback(). If @target has not yet hit
* finish_callback() to clear utrace-reporting, we
* might be in the middle of a callback to @engine.
*/
smp_mb();
if (utrace-reporting == engine)
ret = -EINPROGRESS;
}
/* fall */
case UTRACE_RESUME:
clear_engine_wants_stop(engine);
target-utrace_flags = ~ENGINE_STOP;
/*
* This and all other cases imply resuming if stopped.
* There might not be another report before it just
* resumes, so make sure single-step is not left set.
*/
if (likely(reset))
user_disable_single_step(target);
break;
--- kstub/kernel/utrace.c~10_utrace_reset_should_clear_ss 2010-09-20
03:55:12.0 +0200
+++ kstub/kernel/utrace.c 2010-09-22 01:54:24.0 +0200
@@ -1139,7 +1139,9 @@ int utrace_control(struct task_struct *t
target-utrace_flags = ~ENGINE_STOP;
mark_engine_detached(engine);
reset = reset || utrace_do_stop(target, utrace);
- if (!reset) {
+ if (reset) {
+ user_disable_single_step(target);
+ } else {
/*
* As in utrace_set_events(), this barrier ensures
* that our engine-flags changes have hit before we
Re: [PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines
OK, so what should we do for now? If we can't come to a straightforward answer for the general case fairly quickly, then we can do the simple change to start with. Without the multitracing utrace_resume()-user_disable_single_step() fixes the problem. Otherwise we probably need ENGINE_WANTS_SINGLESTEP. No, no, we don't want that. We don't need more state. And, remember, we really don't need to microoptimize cases when single-step was used recently--the cost of taking one more single-step trap and percolating through the signal paths was going to be pretty large anyway. It's better to have a spurious report (preferably just spurious TIF_NOTIFY_RESUME with no actual callbacks) following any detach, or whatever it takes to ensure user_disable_single_step() always runs if user_enable_*_step did before and there is no engine around to see a report_quiesce callback pass. If there is a report_quiesce or report_signal callback pass where nobody returns UTRACE_*STEP, then we should never leave stepping enabled when we return to user mode. Yes, my concern was running and !current, not exiting. This is OK on x86 but user_disable_single_step() is arch specific. It's not really OK on x86 either, with either SMP or preemption. Thanks, Roland
gdbstub initial code, v11
Changes: syscall stepping + minor cleanups.
Seems to work, more or less, but surely there are some bugs.
Honestly, I don't really know how do the right thing here.
Anyway, most probably this code will be changed. Like ptrace,
ugdb uses -report_syscall_exit() to synthesize a trap. Unlike
ptrace, ugdb_report_signal() doesn't send SIGTRAP to itself
but reports SIGTRAP using siginfo_t we have. In any case,
whatever we do, multiple tracers can confuse each other.
Next: fully implement g/G/p/P, currently I am a bit confused...
But what about features? What should I do next? all-stop,
thread-specific breakpoints (currently I have no idea what
this means), or what?
Oleg.
#include linux/module.h
#include linux/proc_fs.h
#include linux/utrace.h
#include linux/poll.h
#include linux/mm.h
#include linux/regset.h
#include asm/uaccess.h
static int o_remote_debug;
module_param_named(echo, o_remote_debug, bool, 0);
#define BUFFER_SIZE 1024
#define PACKET_SIZE 1024
struct pbuf {
char*cur, *pkt;
charbuf[BUFFER_SIZE];
};
static inline void pb_init(struct pbuf *pb)
{
pb-cur = pb-buf;
pb-pkt = NULL;
}
enum {
U_STOP_IDLE = 0,
U_STOP_PENDING,
U_STOP_SENT,
};
struct ugdb {
struct list_headu_processes;
struct list_headu_stopped;
int u_stop_state;
struct mutexu_mutex;
spinlock_t u_slock;
struct ugdb_thread
*u_cur_tinfo,
*u_cur_hg,
*u_cur_hc;
wait_queue_head_t u_wait;
int u_err;
struct pbuf u_pbuf;
charu_cbuf[PACKET_SIZE];
int u_clen;
sigset_tu_sig_ign;
unsigned int
u_no_ack:1,
u_allstop:1;
};
static inline void ugdb_ck_stopped(struct ugdb *ugdb)
{
spin_lock(ugdb-u_slock);
WARN_ON(!list_empty(ugdb-u_stopped)
ugdb-u_stop_state == U_STOP_IDLE);
WARN_ON(list_empty(ugdb-u_stopped)
ugdb-u_stop_state == U_STOP_PENDING);
spin_unlock(ugdb-u_slock);
}
static struct ugdb *ugdb_create(void)
{
struct ugdb *ugdb;
int err;
err = -ENODEV;
// XXX: ugly. proc_reg_open() should take care.
if (!try_module_get(THIS_MODULE))
goto out;
err = -ENOMEM;
ugdb = kzalloc(sizeof(*ugdb), GFP_KERNEL);
if (!ugdb)
goto put_module;
INIT_LIST_HEAD(ugdb-u_processes);
INIT_LIST_HEAD(ugdb-u_stopped);
mutex_init(ugdb-u_mutex);
spin_lock_init(ugdb-u_slock);
init_waitqueue_head(ugdb-u_wait);
pb_init(ugdb-u_pbuf);
return ugdb;
put_module:
module_put(THIS_MODULE);
out:
return ERR_PTR(err);
}
#define P_DETACHING (1 1)
#define P_ZOMBIE(1 2)
struct ugdb_process {
int p_pid;
int p_state;
struct list_headp_threads;
struct ugdb *p_ugdb;
struct list_headp_processes;
};
static inline bool process_alive(struct ugdb_process *process)
{
return !(process-p_state P_ZOMBIE);
}
static inline void mark_process_dead(struct ugdb_process *process)
{
process-p_state |= P_ZOMBIE;
}
static struct ugdb_process *ugdb_create_process(struct ugdb *ugdb, int pid_nr)
{
struct ugdb_process *process;
process = kzalloc(sizeof(*process), GFP_KERNEL);
if (!process)
return NULL;
process-p_pid = pid_nr;
process-p_ugdb = ugdb;
INIT_LIST_HEAD(process-p_threads);
list_add_tail(process-p_processes, ugdb-u_processes);
return process;
}
#define T_STOP_RUN 0
#define T_STOP_REQ (1 0)/* requested by gdb */
#define T_STOP_ALL (1 1)/* vCont;c:pX.-1, for report_clone */
#define T_STOP_ACK (1 2)/* visible to vStopped */
#define T_STOP_STOPPED (1 3)/* reported as stopped to gdb */
/* TASK_TRACED + deactivated ? */
#define T_EV_NONE 0
#define T_EV_EXIT (1 24)
#define T_EV_SIGN (2 24)
#define T_EV_TYPE(event)((0xff 24) (event))
#define T_EV_DATA(event)(~(0xff 24) (event))
struct ugdb_thread {
int t_tid;
int t_stop_state;
int t_stop_event;
boolt_step; // XXX: move me into
t_stop_event
siginfo_t *t_siginfo;
struct ugdb *t_ugdb;
struct ugdb_process *t_process;
struct
