Re: çå: [PATCH] kernel/kprobes: add check to avoid kprobe memory leak

[Masami Hiramatsu]

On Tue, 7 Nov 2017 03:43:01 +0000
"chenjiankang (A)" <chenjiankang1@xxxxxxxxxx> wrote:

> 
> On Thu, 26 Oct 2017 09:22:23 +0800
> chenjiankang <chenjiankang1@xxxxxxxxxx> wrote:
> 
> > > On Tue, 24 Oct 2017 20:17:02 +0800
> > > JianKang Chen <chenjiankang1@xxxxxxxxxx> wrote:
> > > 
> > >> The function register_kretprobe is used to initialize a struct 
> > >> kretprobe and allocate a list table for kprobe instance.
> > >> However,in this function, there is a memory leak.
> > >>
> > >> The test case:
> > >>
> > >> static struct kretprobe rp;
> > >> struct  kretprobe *rps[10]={&rp ,&rp ,&rp , &rp ,&rp ,&rp ,&rp ,&rp 
> > >> ,&rp,&rp};
> > > 
> > > What ? this is buggy code. you must not list same kretprobe.
> > > But, year, since register_kprobe() already has similar protection 
> > > against reusing, register_kretprobe() should do so.
> > > 
> > > [..]
> > >>  	raw_spin_lock_init(&rp->lock);
> > >> +
> > >> +	if (!hlist_empty(&rp->free_instances))
> > >> +		return -EBUSY;
> > >> +
> > > 
> > > Hmm, but can you use check_kprobe_rereg() before raw_spin_lock_init()?
> > > If user reuses rp after it starts, rp->lock can already be used.
> > 
> > Hmm, your advice is very good, we can use check_kprobe_rereg() at the 
> > beginning of the register_kretprobe();
> > 
> > For example:
> > 
> > int register_kretprobe(struct kretprobe *rp) {
> >         int ret = 0;
> >         struct kretprobe_instance *inst;
> >         int i;
> >         void *addr;
> > 
> >     +    ret = check_kprobe_rereg(&rp->kp);
> >     +    if (ret)
> >     +            return ret;
> 
> Yeah, this looks much better for me :)
> 
> Thanks,
> 
> > 
> > Thank you!
> >  
> > 
>   Hi Masmi:
>       Any other comment about this patch?

OK, for the issue that you trying to fix as you commented
on this patch is not acceptable, since that is obviously
the module bug, not kretprobe bug. kprobes has no
responsibility to handle such situation.

However, simple re-register safe check can be there.
So, you should change the patch description not to use
such test case.

>From discussion with Zhou Chengming, I realized that the
current check_kprobe_rereg() is not safe for multi-threading,
because there is a chance to re-register kretprobe
2 or more threads concurrently. See below case.

CPU0                   CPU1

check_kprobe_rereg()
INIT_LIST_HEAD()
                       check_kprobe_rereg()
register_kprobe()
                       INIT_LIST_HEAD()
                       register_kprobe()

So, to avoid this case, we have 2 options;
 - Introduce kretprobe_mutex and protect register_kretprobe()
 - Extend kprobe_mutex lock critical section to
   register_kretprobe() and introduce register_kprobe() wrapper
   and __register_kprobe() body function so that
   register_kretprobe() can call raw __register_kprobe().

Former is very simple. I think just for protecting
INIT_LIST_HEAD() from re-register, that one is better and enough.

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>