Re: [RFC 03/16] kgr: initial code

[Aravinda Prasad <aravinda@linux.vnet.ibm.com>]

On Wednesday 30 April 2014 08:00 PM, Jiri Slaby wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 
> Provide initial implementation. We are now able to do ftrace-based
> runtime patching of the kernel code.
> 
> In addition to that, we will provide a kgr_patcher module in the next
> patch to test the functionality.

Hi Jiri,

Interesting! I have couple of comments:

I think with kgraft (also with kpatch, though have not looked into
it yet), the patched function cannot be dynamically ftraced.
Though dynamic ftrace can be enabled on the new code, the user is
required to know the function label of the new code. This could
potentially break existing scripts. I think this should be documented.

Rest of the comments in-line.

> +/*
> + * The stub needs to modify the RIP value stored in struct pt_regs
> + * so that ftrace redirects the execution properly.
> + */
> +#define KGR_STUB_ARCH_SLOW(_name, _new_function)			\
> +static void _new_function ##_stub_slow (unsigned long ip, unsigned long parent_ip,	\
> +		struct ftrace_ops *ops, struct pt_regs *regs)		\
> +{									\
> +	struct kgr_loc_caches *c = ops->private;			\
> +									\
> +	if (task_thread_info(current)->kgr_in_progress && current->mm) {\

Is there a race here? The per task kgr_in_progress is set after
the slow stub is registered in register_ftrace_function(). If the
patched function is called in between it will be redirected to new code.


> +		pr_info("kgr: slow stub: calling old code at %lx\n",	\
> +				c->old);				\
> +		regs->ip = c->old + MCOUNT_INSN_SIZE;			\
> +	} else {							\
> +		pr_info("kgr: slow stub: calling new code at %lx\n",	\
> +				c->new);				\
> +		regs->ip = c->new;					\
> +	}								\

[...]

> +static void kgr_mark_processes(void)
> +{
> +	struct task_struct *p;
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process(p)
> +		task_thread_info(p)->kgr_in_progress = true;

Is there a need for memory barrier here (or in slow stub) to avoid
the race if the slow stub is about to be called from a thread executing
on another CPU?

> +	read_unlock(&tasklist_lock);
> +}
> +

[...]

> + * kgr_start_patching -- the entry for a kgraft patch
> + * @patch: patch to be applied
> + *
> + * Start patching of code that is neither running in IRQ context nor
> + * kernel thread.
> + */
> +int kgr_start_patching(const struct kgr_patch *patch)
> +{
> +	const struct kgr_patch_fun *const *patch_fun;
> +
> +	if (!kgr_initialized) {
> +		pr_err("kgr: can't patch, not initialized\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&kgr_in_progress_lock);
> +	if (kgr_in_progress) {
> +		pr_err("kgr: can't patch, another patching not yet finalized\n");
> +		mutex_unlock(&kgr_in_progress_lock);
> +		return -EAGAIN;
> +	}
> +
> +	for (patch_fun = patch->patches; *patch_fun; patch_fun++) {
> +		int ret;
> +
> +		ret = kgr_patch_code(*patch_fun, false);
> +		/*
> +		 * In case any of the symbol resolutions in the set
> +		 * has failed, patch all the previously replaced fentry
> +		 * callsites back to nops and fail with grace
> +		 */
> +		if (ret < 0) {
> +			for (; patch_fun >= patch->patches; patch_fun--)
> +				unregister_ftrace_function((*patch_fun)->ftrace_ops_slow);
> +			mutex_unlock(&kgr_in_progress_lock);
> +			return ret;
> +		}
> +	}
> +	kgr_in_progress = true;
> +	kgr_patch = patch;
> +	mutex_unlock(&kgr_in_progress_lock);
> +
> +	kgr_mark_processes();
> +
> +	/*
> +	 * give everyone time to exit kernel, and check after a while
> +	 */

I understand that the main intention of kgraft is to apply simple
security fixes. However, if the patch changes the locking order,
I think, there is a possibility of deadlock.

A thread which has not yet returned to user space calls the old
code (not redirected to new code in slow stub) which might acquire
the lock in the old order say lock1 followed by lock2. Meanwhile
another thread which re-enters the kernel space, with kgr_in_progress
unset, is redirected to the new code which acquires the lock in reverse
order, say lock2 and lock1. This can cause deadlock.

Thanks,
Aravinda

> +	queue_delayed_work(kgr_wq, &kgr_work, KGR_TIMEOUT * HZ);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kgr_start_patching);
> +
> 

-- 
Regards,
Aravinda