On 11/03/2015 10:50 AM, Josh Poimboeuf wrote:
> On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
>> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
>>
>>> On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
>>>> On Mon, 2 Nov 2015, Chris J Arges wrote:
>>>>
>>>> [...]
>>>>
>>>>> +static int klp_get_func_pos_callback(void *data, const char
>>>>> *name, + struct module *mod, unsigned long addr) +{
>>>>> + struct klp_find_arg *args = data; + + if ((mod &&
>>>>> !args->objname) || (!mod && args->objname)) + return 0; + +
>>>>> if (strcmp(args->name, name)) + return 0; + + if
>>>>> (args->objname && strcmp(args->objname, mod->name)) + return
>>>>> 0; + + /* on address match, return 1 to break
>>>>> kallsyms_on_each_symbol loop */ + if (args->addr == addr) +
>>>>> return 1; + + /* if we don't match addr, count instance of
>>>>> named symbol */ + args->count++; + + return 0; +} + +static
>>>>> int klp_get_func_pos(struct klp_object *obj, struct klp_func
>>>>> *func) +{ + struct klp_find_arg args = { + .objname =
>>>>> obj->name, + .name = func->old_name, + .addr =
>>>>> func->old_addr, + .count = 0, + }; + +
>>>>> mutex_lock(&module_mutex); +
>>>>> kallsyms_on_each_symbol(klp_get_func_pos_callback, &args); +
>>>>> mutex_unlock(&module_mutex); + + return args.count; +} +
>>>>> static int klp_init_func(struct klp_object *obj, struct
>>>>> klp_func *func) { INIT_LIST_HEAD(&func->stack_node);
>>>>> func->state = KLP_DISABLED;
>>>>>
>>>>> return kobject_init_and_add(&func->kobj, &klp_ktype_func, -
>>>>> &obj->kobj, "%s", func->old_name); + &obj->kobj,
>>>>> "%s,%d", func->old_name, + klp_get_func_pos(obj,
>>>>> func)); }
>>>>
>>>> There is a problem which I missed before. klp_init_func() is
>>>> called before klp_find_verify_func_addr() in klp_init_object().
>>>> This means that func->old_addr is either not verified yet or
>>>> worse it is still 0. This means that
>>>> klp_get_func_pos_callback() never returns 1 and is thus called
>>>> on each symbol. So if you for example patched
>>>> cmdline_proc_show the resulting directory in sysfs would be
>>>> called cmdline_proc_show,1 because addr is never matched. Had
>>>> old_addr been specified the name would have been probably
>>>> correct, but not for sure.
>>>>
>>>> This should be fixed as well.
>>>
>>> Even worse, klp_init_func() can be called even if the object
>>> hasn't been loaded. In that case there's no way to know what the
>>> value of n is, and therefore no way to reliably create the sysfs
>>> entry.
>>
>> Ah, right.
>>
>>> Should we create "func,n" in klp_init_object_loaded() instead of
>>> klp_init_func()?
>>
>> So that the function entries in sysfs would be created only when
>> the object is loaded? Well, why not, but in that case it could
>> easily confuse the user.
>
> Maybe, but I think it would be fine if we document it. It should
> only be relied on by tools, anyway.
>
>> Object entry would be empty for not loaded object. I would not dare
>> to propose to remove such object entries. It would make things
>> worse.
>
> Why would removing an empty object entry make things worse?
>
>> So maybe we could introduce an attribute in sysfs object entry
>> which would say if the object is loaded or not. Or something like
>> that.
>
> Hm, I'm not sure I see how this would help.
>
>> Hm, we can easily mess this up :)
>
> Agreed 100% :-)
>
Working on v3 with these suggestions.
- Documentation fixes
- create func,n in klp_init_object_loaded
I'll test unique and non-unique functions being patched. In addition
I'll test this when the object is vmlinux or a module (to test the
object being loaded later).
--chris
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
