Re: [PATCH] kprobes: avoid the kprobe being re-registered

[zhouchengming]

On 2017/10/26 22:39, Masami Hiramatsu wrote:
> On Thu, 26 Oct 2017 20:11:25 +0800
> Zhou Chengming<zhouchengming1@xxxxxxxxxx>  wrote:
>
> > Old code use check_kprobe_rereg() to check if the kprobe has been
> > registered already, but check_kprobe_rereg() will release the
> > kprobe_mutex then, so maybe two paths will pass the check and
> > register the same kprobe. This patch put the check inside the mutex.
> Still no good, see below comment.
>
> > Signed-off-by: Zhou Chengming<zhouchengming1@xxxxxxxxxx>
> > ---
> >   kernel/kprobes.c | 23 ++++++-----------------
> >   1 file changed, 6 insertions(+), 17 deletions(-)
> >
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index a1606a4..2a4873a 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1443,19 +1443,6 @@ static struct kprobe *__get_valid_kprobe(struct kprobe *p)
> >   	return ap;
> >   }
> >
> > -/* Return error if the kprobe is being re-registered */
> > -static inline int check_kprobe_rereg(struct kprobe *p)
> > -{
> > -	int ret = 0;
> > -
> > -	mutex_lock(&kprobe_mutex);
> > -	if (__get_valid_kprobe(p))
> > -		ret = -EINVAL;
> > -	mutex_unlock(&kprobe_mutex);
> > -
> > -	return ret;
> > -}
> > -
> >   int __weak arch_check_ftrace_location(struct kprobe *p)
> >   {
> >   	unsigned long ftrace_addr;
> > @@ -1536,10 +1523,6 @@ int register_kprobe(struct kprobe *p)
> >   		return PTR_ERR(addr);
> >   	p->addr = addr;
> >
> > -	ret = check_kprobe_rereg(p);
> > -	if (ret)
> > -		return ret;
> > -
> >   	/* User can pass only KPROBE_FLAG_DISABLED to register_kprobe */
> >   	p->flags&= KPROBE_FLAG_DISABLED;
> >   	p->nmissed = 0;
> here, we already modifies the kprobe. We need to check and reject before modifying it.
>
> Thank you,

Ah, right. We should put the modifies after the re-reg check. I will send a patch-v2.

Thank you.

> > @@ -1551,6 +1534,12 @@ int register_kprobe(struct kprobe *p)
> >
> >   	mutex_lock(&kprobe_mutex);
> >
> > +	/* Return error if the kprobe is being re-registered */
> > +	if (__get_valid_kprobe(p)) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >   	old_p = get_kprobe(p->addr);
> >   	if (old_p) {
> >   		/* Since this may unoptimize old_p, locking text_mutex. */
> > --
> > 1.8.3.1