Add a function that allows external users (such as live patching
mechanisms) to check whether a given function (identified by symbol name)
has a kprobe installed in it.
Signed-off-by: Jiri Kosina <[email protected]>
---
include/linux/kprobes.h | 5 +++++
kernel/kprobes.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index f7296e5..f760555 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -384,6 +384,7 @@ int disable_kprobe(struct kprobe *kp);
int enable_kprobe(struct kprobe *kp);
void dump_kprobe(struct kprobe *kp);
+bool kprobe_is_function_probed(const char *name);
#else /* !CONFIG_KPROBES: */
@@ -459,6 +460,10 @@ static inline int enable_kprobe(struct kprobe *kp)
{
return -ENOSYS;
}
+static inline bool kprobe_is_function_probed(const char *name)
+{
+ return false;
+}
#endif /* CONFIG_KPROBES */
static inline int disable_kretprobe(struct kretprobe *rp)
{
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3995f54..27e8ddb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2036,6 +2036,34 @@ void dump_kprobe(struct kprobe *kp)
NOKPROBE_SYMBOL(dump_kprobe);
/*
+ * Check whether a given symbol is a probed function
+ */
+bool kprobe_is_function_probed(const char *name)
+{
+ struct hlist_head *head;
+ struct kprobe *p, *kp;
+ const char *sym = NULL;
+ unsigned long offset = 0;
+ unsigned int i;
+ char *modname, namebuf[KSYM_NAME_LEN];
+
+ preempt_disable();
+ for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+ head = &kprobe_table[i];
+ hlist_for_each_entry_rcu(p, head, hlist) {
+ sym = kallsyms_lookup((unsigned long)p->addr,
+ NULL, &offset, &modname,
+ namebuf);
+ if (!strcmp(sym, name))
+ return true;
+ }
+ }
+ preempt_enable();
+ return false;
+}
+EXPORT_SYMBOL_GPL(kprobe_is_function_probed);
+
+/*
* Lookup and populate the kprobe_blacklist.
*
* Unlike the kretprobe blacklist, we'll need to determine
--
1.9.2
On Tue, Oct 21, 2014 at 05:48:30PM +0200, Jiri Kosina wrote:
> kernel/kprobes.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index f7296e5..f760555 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -384,6 +384,7 @@ int disable_kprobe(struct kprobe *kp);
> int enable_kprobe(struct kprobe *kp);
>
> void dump_kprobe(struct kprobe *kp);
> +bool kprobe_is_function_probed(const char *name);
Better name? function_is_kprobed() or such?
...
> +bool kprobe_is_function_probed(const char *name)
> +{
> + struct hlist_head *head;
> + struct kprobe *p, *kp;
kp not used
> + const char *sym = NULL;
> + unsigned long offset = 0;
> + unsigned int i;
> + char *modname, namebuf[KSYM_NAME_LEN];
> +
> + preempt_disable();
> + for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> + head = &kprobe_table[i];
> + hlist_for_each_entry_rcu(p, head, hlist) {
> + sym = kallsyms_lookup((unsigned long)p->addr,
> + NULL, &offset, &modname,
> + namebuf);
> + if (!strcmp(sym, name))
Don't you want a strncmp instead?
Ananth
On Tue, Oct 21, 2014 at 05:48:30PM +0200, Jiri Kosina wrote:
> Add a function that allows external users (such as live patching
> mechanisms) to check whether a given function (identified by symbol name)
> has a kprobe installed in it.
Functions aren't uniquely identifiable by name. Perhaps it should be
something like kprobe_is_addr_range_probed()?
Should we refuse to patch a function which has a kprobe installed inside
it? Is there a race-free way to do that?
>
> Signed-off-by: Jiri Kosina <[email protected]>
> ---
> include/linux/kprobes.h | 5 +++++
> kernel/kprobes.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index f7296e5..f760555 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -384,6 +384,7 @@ int disable_kprobe(struct kprobe *kp);
> int enable_kprobe(struct kprobe *kp);
>
> void dump_kprobe(struct kprobe *kp);
> +bool kprobe_is_function_probed(const char *name);
>
> #else /* !CONFIG_KPROBES: */
>
> @@ -459,6 +460,10 @@ static inline int enable_kprobe(struct kprobe *kp)
> {
> return -ENOSYS;
> }
> +static inline bool kprobe_is_function_probed(const char *name)
> +{
> + return false;
> +}
> #endif /* CONFIG_KPROBES */
> static inline int disable_kretprobe(struct kretprobe *rp)
> {
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 3995f54..27e8ddb 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2036,6 +2036,34 @@ void dump_kprobe(struct kprobe *kp)
> NOKPROBE_SYMBOL(dump_kprobe);
>
> /*
> + * Check whether a given symbol is a probed function
> + */
> +bool kprobe_is_function_probed(const char *name)
> +{
> + struct hlist_head *head;
> + struct kprobe *p, *kp;
> + const char *sym = NULL;
> + unsigned long offset = 0;
> + unsigned int i;
> + char *modname, namebuf[KSYM_NAME_LEN];
> +
> + preempt_disable();
> + for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> + head = &kprobe_table[i];
> + hlist_for_each_entry_rcu(p, head, hlist) {
> + sym = kallsyms_lookup((unsigned long)p->addr,
> + NULL, &offset, &modname,
> + namebuf);
> + if (!strcmp(sym, name))
> + return true;
> + }
> + }
> + preempt_enable();
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(kprobe_is_function_probed);
> +
> +/*
> * Lookup and populate the kprobe_blacklist.
> *
> * Unlike the kretprobe blacklist, we'll need to determine
> --
> 1.9.2
On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
> > Add a function that allows external users (such as live patching
> > mechanisms) to check whether a given function (identified by symbol name)
> > has a kprobe installed in it.
>
> Functions aren't uniquely identifiable by name. Perhaps it should be
> something like kprobe_is_addr_range_probed()?
Hi Josh,
first, thanks a lot for the review.
This is a rather difficult call actually. I am of course aware of the fact
that kernel fucntions can't be uniquely identified by name, but when
thinking about this, one has to consider:
- ftrace primary userspace interface (set_ftrace_filter) is based on
function names
- kprobe tracer and uprobe trace primary userspace interface
(/sys/kernel/debug/tracing/kprobe_events and uprobe_events respectively)
are primarily based on function names
- the number of conflicts is probably zero, or very close to zero. Try to
run this
cut -f3 -d' ' /proc/kallsyms | sort | uniq -c
So it's questionable whether all the back and forth name->address->name
translations all over the place are worth all the trouble.
I do agree though that we should make it obvious that the lookup
interface works on symbol names only ... perhaps by adding '_by_name()'
or so?
> Should we refuse to patch a function which has a kprobe installed inside
> it?
I think warning about it is a good thing to do.
> Is there a race-free way to do that?
Do we need to be race-free here? If userspace is both instantiating new
krpobe and initiating live kernel patching at the "same time", I don't
think kernel should try to solve such race ... it's simply there by
definition, depending on, for example, scheduling decisions.
Thanks,
--
Jiri Kosina
SUSE Labs
On Tue, Oct 21, 2014 at 10:19:30PM +0200, Jiri Kosina wrote:
> On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
>
> > > Add a function that allows external users (such as live patching
> > > mechanisms) to check whether a given function (identified by symbol name)
> > > has a kprobe installed in it.
> >
> > Functions aren't uniquely identifiable by name. Perhaps it should be
> > something like kprobe_is_addr_range_probed()?
>
> Hi Josh,
>
> first, thanks a lot for the review.
>
> This is a rather difficult call actually. I am of course aware of the fact
> that kernel fucntions can't be uniquely identified by name, but when
> thinking about this, one has to consider:
>
> - ftrace primary userspace interface (set_ftrace_filter) is based on
> function names
> - kprobe tracer and uprobe trace primary userspace interface
> (/sys/kernel/debug/tracing/kprobe_events and uprobe_events respectively)
> are primarily based on function names
True, the user space interfaces are done by name, but the kernel
interfaces aren't necessarily so (see ftrace_set_filter_ip and struct
kprobe.addr). This is a kernel interface so we can be more precise.
> - the number of conflicts is probably zero, or very close to zero. Try to
> run this
>
> cut -f3 -d' ' /proc/kallsyms | sort | uniq -c
>
> So it's questionable whether all the back and forth name->address->name
> translations all over the place are worth all the trouble.
On my kernel:
$ grep ' t \| T ' /proc/kallsyms | awk '{print $3}' |sort |uniq -d |wc -l
395
So there are at least 395 places where this could go wrong...
With kpatch, we're setting the ftrace filter by address so we don't
patch the wrong function. So we already have the address. We also have
the function length, which is needed for our backtrace safety checks.
I'm guessing kGraft doesn't have the address + length? I think you
could call kallsyms_lookup() to get both values.
Maybe we should see what our unified live patching code ends up looking
like before deciding what interface(s) we need here?
>
> I do agree though that we should make it obvious that the lookup
> interface works on symbol names only ... perhaps by adding '_by_name()'
> or so?
>
> > Should we refuse to patch a function which has a kprobe installed inside
> > it?
>
> I think warning about it is a good thing to do.
I think that's probably ok.
>
> > Is there a race-free way to do that?
>
> Do we need to be race-free here? If userspace is both instantiating new
> krpobe and initiating live kernel patching at the "same time", I don't
> think kernel should try to solve such race ... it's simply there by
> definition, depending on, for example, scheduling decisions.
If we're not preventing it, but instead just printing a warning, then
yeah, that sounds fine to me.
I think it would also be nice to do the reverse, i.e. have kprobes emit
a warning if someone tries to probe a replaced function.
--
Josh
On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
> > This is a rather difficult call actually. I am of course aware of the fact
> > that kernel fucntions can't be uniquely identified by name, but when
> > thinking about this, one has to consider:
> >
> > - ftrace primary userspace interface (set_ftrace_filter) is based on
> > function names
> > - kprobe tracer and uprobe trace primary userspace interface
> > (/sys/kernel/debug/tracing/kprobe_events and uprobe_events respectively)
> > are primarily based on function names
>
> True, the user space interfaces are done by name, but the kernel
> interfaces aren't necessarily so (see ftrace_set_filter_ip and struct
> kprobe.addr). This is a kernel interface so we can be more precise.
We could probably have both interfaces available (i.e. allowing lookup by
name or by address range, and let the caller decide/handle the
potential duplicates).
> > - the number of conflicts is probably zero, or very close to zero. Try to
> > run this
> >
> > cut -f3 -d' ' /proc/kallsyms | sort | uniq -c
> >
> > So it's questionable whether all the back and forth name->address->name
> > translations all over the place are worth all the trouble.
>
> On my kernel:
>
> $ grep ' t \| T ' /proc/kallsyms | awk '{print $3}' |sort |uniq -d |wc -l
> 395
>
> So there are at least 395 places where this could go wrong...
This is broken anyway ... this will add up a lot of unrelated things
together (umask_show, _iommu_cpumask_show, __uncore_umask_show,
wq_cpumask_show, etc). I believe looking at
cut -f3 -d' ' /proc/kallsyms | sort | uniq -c | sort -nr -k1
gives slightly better picture.
Also keep in mind that a *lot* of the hits are not functions.
> With kpatch, we're setting the ftrace filter by address so we don't
> patch the wrong function. So we already have the address. We also have
> the function length, which is needed for our backtrace safety checks.
>
> I'm guessing kGraft doesn't have the address + length? I think you
> could call kallsyms_lookup() to get both values.
>
> Maybe we should see what our unified live patching code ends up looking
> like before deciding what interface(s) we need here?
Yes, that probably makes sense indeed. I am talking to David Miller wrt.
mailinglist creation on vger.kernel.org as we speak, hopefully it'll
materialize soon.
> > Do we need to be race-free here? If userspace is both instantiating new
> > krpobe and initiating live kernel patching at the "same time", I don't
> > think kernel should try to solve such race ... it's simply there by
> > definition, depending on, for example, scheduling decisions.
>
> If we're not preventing it, but instead just printing a warning, then
> yeah, that sounds fine to me.
>
> I think it would also be nice to do the reverse, i.e. have kprobes emit
> a warning if someone tries to probe a replaced function.
Indeed, good idea!
Thanks,
--
Jiri Kosina
SUSE Labs
(2014/10/22 0:48), Jiri Kosina wrote:
> Add a function that allows external users (such as live patching
> mechanisms) to check whether a given function (identified by symbol name)
> has a kprobe installed in it.
Actually, we've already exported the list of kprobes with probe points
(symbols) via debugfs. Please check /sys/kernel/debug/kprobes/list :)
Thank you,
>
> Signed-off-by: Jiri Kosina <[email protected]>
> ---
> include/linux/kprobes.h | 5 +++++
> kernel/kprobes.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index f7296e5..f760555 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -384,6 +384,7 @@ int disable_kprobe(struct kprobe *kp);
> int enable_kprobe(struct kprobe *kp);
>
> void dump_kprobe(struct kprobe *kp);
> +bool kprobe_is_function_probed(const char *name);
>
> #else /* !CONFIG_KPROBES: */
>
> @@ -459,6 +460,10 @@ static inline int enable_kprobe(struct kprobe *kp)
> {
> return -ENOSYS;
> }
> +static inline bool kprobe_is_function_probed(const char *name)
> +{
> + return false;
> +}
> #endif /* CONFIG_KPROBES */
> static inline int disable_kretprobe(struct kretprobe *rp)
> {
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 3995f54..27e8ddb 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2036,6 +2036,34 @@ void dump_kprobe(struct kprobe *kp)
> NOKPROBE_SYMBOL(dump_kprobe);
>
> /*
> + * Check whether a given symbol is a probed function
> + */
> +bool kprobe_is_function_probed(const char *name)
> +{
> + struct hlist_head *head;
> + struct kprobe *p, *kp;
> + const char *sym = NULL;
> + unsigned long offset = 0;
> + unsigned int i;
> + char *modname, namebuf[KSYM_NAME_LEN];
> +
> + preempt_disable();
> + for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> + head = &kprobe_table[i];
> + hlist_for_each_entry_rcu(p, head, hlist) {
> + sym = kallsyms_lookup((unsigned long)p->addr,
> + NULL, &offset, &modname,
> + namebuf);
> + if (!strcmp(sym, name))
> + return true;
> + }
> + }
> + preempt_enable();
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(kprobe_is_function_probed);
> +
> +/*
> * Lookup and populate the kprobe_blacklist.
> *
> * Unlike the kretprobe blacklist, we'll need to determine
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On Tue, Oct 21, 2014 at 11:25:56PM +0200, Jiri Kosina wrote:
> On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
>
> > > This is a rather difficult call actually. I am of course aware of the fact
> > > that kernel fucntions can't be uniquely identified by name, but when
> > > thinking about this, one has to consider:
> > >
> > > - ftrace primary userspace interface (set_ftrace_filter) is based on
> > > function names
> > > - kprobe tracer and uprobe trace primary userspace interface
> > > (/sys/kernel/debug/tracing/kprobe_events and uprobe_events respectively)
> > > are primarily based on function names
> >
> > True, the user space interfaces are done by name, but the kernel
> > interfaces aren't necessarily so (see ftrace_set_filter_ip and struct
> > kprobe.addr). This is a kernel interface so we can be more precise.
>
> We could probably have both interfaces available (i.e. allowing lookup by
> name or by address range, and let the caller decide/handle the
> potential duplicates).
>
> > > - the number of conflicts is probably zero, or very close to zero. Try to
> > > run this
> > >
> > > cut -f3 -d' ' /proc/kallsyms | sort | uniq -c
> > >
> > > So it's questionable whether all the back and forth name->address->name
> > > translations all over the place are worth all the trouble.
> >
> > On my kernel:
> >
> > $ grep ' t \| T ' /proc/kallsyms | awk '{print $3}' |sort |uniq -d |wc -l
> > 395
> >
> > So there are at least 395 places where this could go wrong...
>
> This is broken anyway ... this will add up a lot of unrelated things
> together (umask_show, _iommu_cpumask_show, __uncore_umask_show,
> wq_cpumask_show, etc).
Can you clarify what you mean here? I don't see how umask_show would
get lumped together with _iommu_cpumask_show. Not trying to be
pedantic, just trying to get a good idea of how many duplicate function
names there are.
> I believe looking at
>
> cut -f3 -d' ' /proc/kallsyms | sort | uniq -c | sort -nr -k1
>
> gives slightly better picture.
>
> Also keep in mind that a *lot* of the hits are not functions.
How is that giving a better picture? I had used "grep ' t \| T ' above
to try to filter out non-function symbols.
>
> > With kpatch, we're setting the ftrace filter by address so we don't
> > patch the wrong function. So we already have the address. We also have
> > the function length, which is needed for our backtrace safety checks.
> >
> > I'm guessing kGraft doesn't have the address + length? I think you
> > could call kallsyms_lookup() to get both values.
> >
> > Maybe we should see what our unified live patching code ends up looking
> > like before deciding what interface(s) we need here?
>
> Yes, that probably makes sense indeed. I am talking to David Miller wrt.
> mailinglist creation on vger.kernel.org as we speak, hopefully it'll
> materialize soon.
Ok, thanks! Seth is currently slaving away on the code :-)
>
> > > Do we need to be race-free here? If userspace is both instantiating new
> > > krpobe and initiating live kernel patching at the "same time", I don't
> > > think kernel should try to solve such race ... it's simply there by
> > > definition, depending on, for example, scheduling decisions.
> >
> > If we're not preventing it, but instead just printing a warning, then
> > yeah, that sounds fine to me.
> >
> > I think it would also be nice to do the reverse, i.e. have kprobes emit
> > a warning if someone tries to probe a replaced function.
>
> Indeed, good idea!
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
--
Josh
On Wed, 22 Oct 2014, Masami Hiramatsu wrote:
> > Add a function that allows external users (such as live patching
> > mechanisms) to check whether a given function (identified by symbol name)
> > has a kprobe installed in it.
>
> Actually, we've already exported the list of kprobes with probe points
> (symbols) via debugfs. Please check /sys/kernel/debug/kprobes/list :)
Yes, I know, and kprobe_is_function_probed() is performing very similar
thing that show_kprobe_addr() is doing.
But we'd like to be able to check this from within a kernel module (the
patch module) and issue WARN().
Otherwise we'll need to have a machinery in userspace before insmod which
will look up the functions in the module and compare them to the
debufs-provided interface.
Also, I don't think we want to be dependent on debugfs being mounted and
discoverable, etc ...
Thanks Masami,
--
Jiri Kosina
SUSE Labs
(2014/10/22 15:02), Jiri Kosina wrote:
> On Wed, 22 Oct 2014, Masami Hiramatsu wrote:
>
>>> Add a function that allows external users (such as live patching
>>> mechanisms) to check whether a given function (identified by symbol name)
>>> has a kprobe installed in it.
>>
>> Actually, we've already exported the list of kprobes with probe points
>> (symbols) via debugfs. Please check /sys/kernel/debug/kprobes/list :)
>
> Yes, I know, and kprobe_is_function_probed() is performing very similar
> thing that show_kprobe_addr() is doing.
> But we'd like to be able to check this from within a kernel module (the
> patch module) and issue WARN().
>
> Otherwise we'll need to have a machinery in userspace before insmod which
> will look up the functions in the module and compare them to the
> debufs-provided interface.
>
> Also, I don't think we want to be dependent on debugfs being mounted and
> discoverable, etc ...
OK, and even if so, since there is no user of this function, at this point
we can not merge this. I'd like to wait until live patching.
BTW, if your tool is in kernel, you can use get_kprobe(addr) to check
there is kprobes on a given address. And of course you can use kallsyms
to get the address range of given function. Moreover, as Josh pointed,
there are several same name functions in kernel. I think this kind of
function should better provide address-base interface.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On Tue, Oct 21, 2014 at 09:40:32PM -0500, Josh Poimboeuf wrote:
> On Tue, Oct 21, 2014 at 11:25:56PM +0200, Jiri Kosina wrote:
> > On Tue, 21 Oct 2014, Josh Poimboeuf wrote:
> > >
> > > I'm guessing kGraft doesn't have the address + length? I think you
> > > could call kallsyms_lookup() to get both values.
> > >
> > > Maybe we should see what our unified live patching code ends up looking
> > > like before deciding what interface(s) we need here?
> >
> > Yes, that probably makes sense indeed. I am talking to David Miller wrt.
> > mailinglist creation on vger.kernel.org as we speak, hopefully it'll
> > materialize soon.
>
> Ok, thanks! Seth is currently slaving away on the code :-)
Yes, I am :) Let me know if this impacts the information we need to
pass via the *_register() call to the core module.
Currently, I pass the old function name (char *), new function pointer
(void *), and the old_addr (optional, unsigned long). The old_addr
serves to identify the old function by address instead of name if that
information is provided.
Thanks,
Seth