Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 29a55513 authored by Oleg Nesterov's avatar Oleg Nesterov Committed by Linus Torvalds
Browse files

ptrace/x86: dont delay "disable" till second pass in ptrace_write_dr7() 



ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
unless second_pass, this buys nothing but complicates the code and means
that we always do the main loop twice even if "disabled" was never true.

The comment says:

	Don't unregister the breakpoints right-away,
	unless all register_user_hw_breakpoint()
	requests have succeeded.

Firstly, we do not do register_user_hw_breakpoint(), it was removed by
commit 24f1e32c ("hw-breakpoints: Rewrite the hw-breakpoints layer
on top of perf events").

We are going to restore register_user_hw_breakpoint() (see the next
patch) but this doesn't matter: after commit 44234adc
("hw-breakpoints: Modify breakpoints without unregistering them")
perf_event_disable() can not hurt, hw_breakpoint_del() does not free the
slot.

Remove the "second_pass" check from the main loop and simplify the code.
Since we have to check "bp != NULL" anyway, the patch also removes the
same check in ptrace_modify_breakpoint() and moves the comment into
ptrace_write_dr7().

With this patch the second pass is only needed to restore the saved
old_dr7.  This should never fail, so the patch adds WARN_ON() to catch
the potential problems as Frederic suggested.

Signed-off-by: default avatarOleg Nesterov <oleg@redhat.com>
Acked-by: default avatarFrederic Weisbecker <fweisbec@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jan Kratochvil <jan.kratochvil@redhat.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent e6a7d607

arch/x86/kernel/ptrace.c

+20 −33
Original line number Original line Diff line number Diff line
@@ -609,14 +609,6 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type, 
	int gen_len, gen_type;

	int gen_len, gen_type;

	struct perf_event_attr attr;

	struct perf_event_attr attr;





	/*

	 * We should have at least an inactive breakpoint at this

	 * slot. It means the user is writing dr7 without having

	 * written the address register first

	 */

	if (!bp)

		return -EINVAL;



	err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);

	err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);

	if (err)

	if (err)

		return err;

		return err;
@@ -634,52 +626,47 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type, 
 */

 */

static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)

static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)

{

{

	struct thread_struct *thread = &(tsk->thread);

	struct thread_struct *thread = &tsk->thread;

	unsigned long old_dr7;

	unsigned long old_dr7;

	int i, orig_ret = 0, rc = 0;

	bool second_pass = false;

	int second_pass = 0;

	int i, rc, ret = 0;





	data &= ~DR_CONTROL_RESERVED;

	data &= ~DR_CONTROL_RESERVED;

	old_dr7 = ptrace_get_dr7(thread->ptrace_bps);

	old_dr7 = ptrace_get_dr7(thread->ptrace_bps);



restore:

restore:

	/*

	rc = 0;

	 * Loop through all the hardware breakpoints, making the

	 * appropriate changes to each.

	 */

	for (i = 0; i < HBP_NUM; i++) {

	for (i = 0; i < HBP_NUM; i++) {

		unsigned len, type;

		unsigned len, type;

		bool disabled = !decode_dr7(data, i, &len, &type);

		bool disabled = !decode_dr7(data, i, &len, &type);

		struct perf_event *bp = thread->ptrace_bps[i];

		struct perf_event *bp = thread->ptrace_bps[i];





		if (disabled) {

		if (!bp) {

			if (disabled)

				continue;

			/*

			/*

			 * Don't unregister the breakpoints right-away, unless

			 * We should have at least an inactive breakpoint at

			 * all register_user_hw_breakpoint() requests have

			 * this slot. It means the user is writing dr7 without

			 * succeeded. This prevents any window of opportunity

			 * having written the address register first.

			 * for debug register grabbing by other users.

			 */

			 */

			if (!bp || !second_pass)

			rc = -EINVAL;

				continue;

			break;

		}

		}





		rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);

		rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);

		if (rc)

		if (rc)

			break;

			break;

	}

	}

	/*



	 * Make a second pass to free the remaining unused breakpoints

	/* Restore if the first pass failed, second_pass shouldn't fail. */

	 * or to restore the original breakpoints if an error occurred.

	if (rc && !WARN_ON(second_pass)) {

	 */

		ret = rc;

	if (!second_pass) {

		second_pass = 1;

		if (rc < 0) {

			orig_ret = rc;

		data = old_dr7;

		data = old_dr7;

		}

		second_pass = true;

		goto restore;

		goto restore;

	}

	}





	return orig_ret < 0 ? orig_ret : rc;

	return ret;

}

}





/*

/*