Re: [PATCH 03/11] jump label: Base patch for jump label

[Mathieu Desnoyers <compudj@krystal.dyndns.org>]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> base patch to implement 'jump labeling'. Based on a new 'asm goto' inline
> assembly gcc mechanism, we can now branch to labels from an 'asm goto'
> statment. This allows us to create a 'no-op' fastpath, which can subsequently
> be patched with a jump to the slowpath code. This is useful for code which
> might be rarely used, but which we'd like to be able to call, if needed.
> Tracepoints are the current usecase that these are being implemented for.
> 
[...]
> +/***
> + * jump_label_update - update jump label text
> + * @key -  key value associated with a a jump label
> + * @type - enum set to JUMP_LABEL_ENABLE or JUMP_LABEL_DISABLE
> + *
> + * Will enable/disable the jump for jump label @key, depending on the
> + * value of @type.
> + *
> + */
> +
> +void jump_label_update(unsigned long key, enum jump_label_type type)
> +{
> +	struct jump_entry *iter;
> +	struct jump_label_entry *entry;
> +	struct hlist_node *module_node;
> +	struct jump_label_module_entry *e_module;
> +	int count;
> +
> +	mutex_lock(&jump_label_mutex);
> +	entry = get_jump_label_entry((jump_label_t)key);
> +	if (entry) {
> +		count = entry->nr_entries;
> +		iter = entry->table;
> +		while (count--) {
> +			if (kernel_text_address(iter->code))

As I pointed out in another thread, I'm concerned about the use of
kernel_text_address without module mutex here. kernel_text_address calls
is_module_text_address(), which calls __module_text_address() with
preemption off.

__module_text_address() looks like:

struct module *__module_address(unsigned long addr)
{
        struct module *mod;

        if (addr < module_addr_min || addr > module_addr_max)
                return NULL;

        list_for_each_entry_rcu(mod, &modules, list)
                if (within_module_core(addr, mod)
                    || within_module_init(addr, mod))
                        return mod;
        return NULL;
}

struct module *__module_text_address(unsigned long addr)
{
        struct module *mod = __module_address(addr);
        if (mod) {
                /* Make sure it's within the text section. */
                if (!within(addr, mod->module_init, mod->init_text_size)
                    && !within(addr, mod->module_core, mod->core_text_size))
                        mod = NULL;
        }
        return mod;
}

So the test for the address being in the module core is already
problematic, since we hold preempt off only within
is_module_text_address(). The is_module_text_address() caller is then
free to write to this address even after the module has been unloaded
and the module unload grace period ended.

Even worse, such grace period is not waited for at module load time
within:

init_module()
       module_free(mod, mod->module_init);
       mod->module_init = NULL;
       mod->init_size = 0;
       mod->init_text_size = 0;
  (done with module_mutex held, while the module is already in the
   module list)

We'd probably have to hold the module mutex around the
is_module_text_address() call and address use (which can be a pain), or
to correctly address this part of init_module() with RCU and require
that preempt off is held across both __module_text_address() call site
and the actual use of that pointer (which does not fit with jump label,
which need to sleep, so we'd have to move module.c to a preemptable
rcu_read_lock/synchronize_rcu() C.S.).

Thoughts ?

Mathieu


> +				arch_jump_label_transform(iter, type);
> +			iter++;
> +		}
> +		/* eanble/disable jump labels in modules */
> +		hlist_for_each_entry(e_module, module_node, &(entry->modules),
> +							hlist) {
> +			count = e_module->nr_entries;
> +			iter = e_module->table;
> +			while (count--) {
> +				if (kernel_text_address(iter->code))
> +					arch_jump_label_transform(iter, type);
> +				iter++;
> +			}
> +		}
> +	}
> +	mutex_unlock(&jump_label_mutex);
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com