[PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wang ShaoBo]

Our system encountered a re-init error when re-registering same kretprobe,
where the kretprobe_instance in rp->free_instances is illegally accessed
after re-init.

Implementation to avoid re-registration has been introduced for kprobe
before, but lags for register_kretprobe(). We must check if kprobe has
been re-registered before re-initializing kretprobe, otherwise it will
destroy the data struct of kretprobe registered, which can lead to memory
leak, system crash, also some unexpected behaviors.

we use check_kprobe_rereg() to check if kprobe has been re-registered
before calling register_kretprobe(), for giving a warning message and
terminate registration process.

Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
---
 kernel/kprobes.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 41fdbb7953c6..7f54a70136f3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
 		}
 	}
 
+	/*
+	 * Return error if it's being re-registered,
+	 * also give a warning message to the developer.
+	 */
+	ret = check_kprobe_rereg(&rp->kp);
+	if (WARN_ON(ret))
+		return ret;
+
 	rp->kp.pre_handler = pre_handler_kretprobe;
 	rp->kp.post_handler = NULL;
 	rp->kp.fault_handler = NULL;
-- 
2.17.1

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Steven Rostedt]

Masami,

Can you review this patch, and also, should this go to -rc and stable?

-- Steve


On Tue, 24 Nov 2020 19:57:19 +0800
Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:

> Our system encountered a re-init error when re-registering same kretprobe,
> where the kretprobe_instance in rp->free_instances is illegally accessed
> after re-init.
> 
> Implementation to avoid re-registration has been introduced for kprobe
> before, but lags for register_kretprobe(). We must check if kprobe has
> been re-registered before re-initializing kretprobe, otherwise it will
> destroy the data struct of kretprobe registered, which can lead to memory
> leak, system crash, also some unexpected behaviors.
> 
> we use check_kprobe_rereg() to check if kprobe has been re-registered
> before calling register_kretprobe(), for giving a warning message and
> terminate registration process.
> 
> Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> ---
>  kernel/kprobes.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 41fdbb7953c6..7f54a70136f3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
>  		}
>  	}
>  
> +	/*
> +	 * Return error if it's being re-registered,
> +	 * also give a warning message to the developer.
> +	 */
> +	ret = check_kprobe_rereg(&rp->kp);
> +	if (WARN_ON(ret))
> +		return ret;
> +
>  	rp->kp.pre_handler = pre_handler_kretprobe;
>  	rp->kp.post_handler = NULL;
>  	rp->kp.fault_handler = NULL;

	

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Masami Hiramatsu]

On Mon, 30 Nov 2020 16:18:50 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Masami,
> 
> Can you review this patch, and also, should this go to -rc and stable?
> 
> -- Steve

Thanks for ping me!

> On Tue, 24 Nov 2020 19:57:19 +0800
> Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:
> 
> > Our system encountered a re-init error when re-registering same kretprobe,
> > where the kretprobe_instance in rp->free_instances is illegally accessed
> > after re-init.

Ah, OK. Anyway if re-register happens on kretprobe, it must lose instances
on the list before checking re-register in register_kprobe().
So the idea looks good to me.


> > Implementation to avoid re-registration has been introduced for kprobe
> > before, but lags for register_kretprobe(). We must check if kprobe has
> > been re-registered before re-initializing kretprobe, otherwise it will
> > destroy the data struct of kretprobe registered, which can lead to memory
> > leak, system crash, also some unexpected behaviors.
> > 
> > we use check_kprobe_rereg() to check if kprobe has been re-registered
> > before calling register_kretprobe(), for giving a warning message and
> > terminate registration process.
> > 
> > Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
> > Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> > ---
> >  kernel/kprobes.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 41fdbb7953c6..7f54a70136f3 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Return error if it's being re-registered,
> > +	 * also give a warning message to the developer.
> > +	 */
> > +	ret = check_kprobe_rereg(&rp->kp);
> > +	if (WARN_ON(ret))
> > +		return ret;

If you call this here, you must make sure kprobe_addr() is called on rp->kp.
But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
this check. So it should be in between kprobe_on_func_entry() and
kretprobe_blacklist_size check, like this

	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
		return -EINVAL;

	addr = kprobe_addr(&rp->kp);
	if (IS_ERR(addr))
		return PTR_ERR(addr);
	rp->kp.addr = addr;

	ret = check_kprobe_rereg(&rp->kp);
	if (WARN_ON(ret))
		return ret;

        if (kretprobe_blacklist_size) {
		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);


Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wangshaobo (bobo)]

Hi steve, Masami,

Thanks for your works, i will check code again and modify properly 
according to steve's suggestion.

-- ShaoBo

在 2020/12/2 7:32, Masami Hiramatsu 写道:
> On Mon, 30 Nov 2020 16:18:50 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> Masami,
>>
>> Can you review this patch, and also, should this go to -rc and stable?
>>
>> -- Steve
> Thanks for ping me!
>
>> On Tue, 24 Nov 2020 19:57:19 +0800
>> Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:
>>
>>> Our system encountered a re-init error when re-registering same kretprobe,
>>> where the kretprobe_instance in rp->free_instances is illegally accessed
>>> after re-init.
> Ah, OK. Anyway if re-register happens on kretprobe, it must lose instances
> on the list before checking re-register in register_kprobe().
> So the idea looks good to me.
>
>
>>> Implementation to avoid re-registration has been introduced for kprobe
>>> before, but lags for register_kretprobe(). We must check if kprobe has
>>> been re-registered before re-initializing kretprobe, otherwise it will
>>> destroy the data struct of kretprobe registered, which can lead to memory
>>> leak, system crash, also some unexpected behaviors.
>>>
>>> we use check_kprobe_rereg() to check if kprobe has been re-registered
>>> before calling register_kretprobe(), for giving a warning message and
>>> terminate registration process.
>>>
>>> Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
>>> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
>>> ---
>>>   kernel/kprobes.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>> index 41fdbb7953c6..7f54a70136f3 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>>> @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
>>>   		}
>>>   	}
>>>   
>>> +	/*
>>> +	 * Return error if it's being re-registered,
>>> +	 * also give a warning message to the developer.
>>> +	 */
>>> +	ret = check_kprobe_rereg(&rp->kp);
>>> +	if (WARN_ON(ret))
>>> +		return ret;
> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> this check. So it should be in between kprobe_on_func_entry() and
> kretprobe_blacklist_size check, like this
>
> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> 		return -EINVAL;
>
> 	addr = kprobe_addr(&rp->kp);
> 	if (IS_ERR(addr))
> 		return PTR_ERR(addr);
> 	rp->kp.addr = addr;
>
> 	ret = check_kprobe_rereg(&rp->kp);
> 	if (WARN_ON(ret))
> 		return ret;
>
>          if (kretprobe_blacklist_size) {
> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
>
>
> Thank you,
>
>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Steven Rostedt]

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:

> Hi steve, Masami,

Hi ShaoBo,

> 
> Thanks for your works, i will check code again and modify properly 
> according to steve's suggestion.

I think you meant "to Masami's suggestion" ;-)

-- Steve

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Steven Rostedt]

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:

> Hi steve, Masami,
> 
> Thanks for your works, i will check code again and modify properly 
> according to steve's suggestion.
> 
> -- ShaoBo
> 

Anything happen with this? 

-- Steve


> 在 2020/12/2 7:32, Masami Hiramatsu 写道:
> > On Mon, 30 Nov 2020 16:18:50 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >  
> >> Masami,
> >>
> >> Can you review this patch, and also, should this go to -rc and stable?
> >>
> >> -- Steve  
> > Thanks for ping me!
> >  
> >> On Tue, 24 Nov 2020 19:57:19 +0800
> >> Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:
> >>  
> >>> Our system encountered a re-init error when re-registering same kretprobe,
> >>> where the kretprobe_instance in rp->free_instances is illegally accessed
> >>> after re-init.  
> > Ah, OK. Anyway if re-register happens on kretprobe, it must lose instances
> > on the list before checking re-register in register_kprobe().
> > So the idea looks good to me.
> >
> >  
> >>> Implementation to avoid re-registration has been introduced for kprobe
> >>> before, but lags for register_kretprobe(). We must check if kprobe has
> >>> been re-registered before re-initializing kretprobe, otherwise it will
> >>> destroy the data struct of kretprobe registered, which can lead to memory
> >>> leak, system crash, also some unexpected behaviors.
> >>>
> >>> we use check_kprobe_rereg() to check if kprobe has been re-registered
> >>> before calling register_kretprobe(), for giving a warning message and
> >>> terminate registration process.
> >>>
> >>> Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
> >>> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> >>> ---
> >>>   kernel/kprobes.c | 8 ++++++++
> >>>   1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >>> index 41fdbb7953c6..7f54a70136f3 100644
> >>> --- a/kernel/kprobes.c
> >>> +++ b/kernel/kprobes.c
> >>> @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
> >>>   		}
> >>>   	}
> >>>   
> >>> +	/*
> >>> +	 * Return error if it's being re-registered,
> >>> +	 * also give a warning message to the developer.
> >>> +	 */
> >>> +	ret = check_kprobe_rereg(&rp->kp);
> >>> +	if (WARN_ON(ret))
> >>> +		return ret;  
> > If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> > But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> > this check. So it should be in between kprobe_on_func_entry() and
> > kretprobe_blacklist_size check, like this
> >
> > 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> > 		return -EINVAL;
> >
> > 	addr = kprobe_addr(&rp->kp);
> > 	if (IS_ERR(addr))
> > 		return PTR_ERR(addr);
> > 	rp->kp.addr = addr;
> >
> > 	ret = check_kprobe_rereg(&rp->kp);
> > 	if (WARN_ON(ret))
> > 		return ret;
> >
> >          if (kretprobe_blacklist_size) {
> > 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
> >
> >
> > Thank you,
> >
> >  

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Masami Hiramatsu]

Hi ShaoBo,

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:

> Hi steve, Masami,
> 
> Thanks for your works, i will check code again and modify properly 
> according to steve's suggestion.
> 

Can you update your patch and resend it?

Thank you,

> -- ShaoBo
> 
> 在 2020/12/2 7:32, Masami Hiramatsu 写道:
> > On Mon, 30 Nov 2020 16:18:50 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >> Masami,
> >>
> >> Can you review this patch, and also, should this go to -rc and stable?
> >>
> >> -- Steve
> > Thanks for ping me!
> >
> >> On Tue, 24 Nov 2020 19:57:19 +0800
> >> Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:
> >>
> >>> Our system encountered a re-init error when re-registering same kretprobe,
> >>> where the kretprobe_instance in rp->free_instances is illegally accessed
> >>> after re-init.
> > Ah, OK. Anyway if re-register happens on kretprobe, it must lose instances
> > on the list before checking re-register in register_kprobe().
> > So the idea looks good to me.
> >
> >
> >>> Implementation to avoid re-registration has been introduced for kprobe
> >>> before, but lags for register_kretprobe(). We must check if kprobe has
> >>> been re-registered before re-initializing kretprobe, otherwise it will
> >>> destroy the data struct of kretprobe registered, which can lead to memory
> >>> leak, system crash, also some unexpected behaviors.
> >>>
> >>> we use check_kprobe_rereg() to check if kprobe has been re-registered
> >>> before calling register_kretprobe(), for giving a warning message and
> >>> terminate registration process.
> >>>
> >>> Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
> >>> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> >>> ---
> >>>   kernel/kprobes.c | 8 ++++++++
> >>>   1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >>> index 41fdbb7953c6..7f54a70136f3 100644
> >>> --- a/kernel/kprobes.c
> >>> +++ b/kernel/kprobes.c
> >>> @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
> >>>   		}
> >>>   	}
> >>>   
> >>> +	/*
> >>> +	 * Return error if it's being re-registered,
> >>> +	 * also give a warning message to the developer.
> >>> +	 */
> >>> +	ret = check_kprobe_rereg(&rp->kp);
> >>> +	if (WARN_ON(ret))
> >>> +		return ret;
> > If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> > But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> > this check. So it should be in between kprobe_on_func_entry() and
> > kretprobe_blacklist_size check, like this
> >
> > 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> > 		return -EINVAL;
> >
> > 	addr = kprobe_addr(&rp->kp);
> > 	if (IS_ERR(addr))
> > 		return PTR_ERR(addr);
> > 	rp->kp.addr = addr;
> >
> > 	ret = check_kprobe_rereg(&rp->kp);
> > 	if (WARN_ON(ret))
> > 		return ret;
> >
> >          if (kretprobe_blacklist_size) {
> > 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
> >
> >
> > Thank you,
> >
> >


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wangshaobo (bobo)]

Hi Masami,

I will update and resend it soon

Thank you

-- ShaoBo

在 2020/12/15 11:31, Masami Hiramatsu 写道:
> Hi ShaoBo,
>
> On Wed, 2 Dec 2020 09:23:35 +0800
> "Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:
>
>> Hi steve, Masami,
>>
>> Thanks for your works, i will check code again and modify properly
>> according to steve's suggestion.
>>
> Can you update your patch and resend it?
>
> Thank you,
>
>> -- ShaoBo
>>
>> 在 2020/12/2 7:32, Masami Hiramatsu 写道:
>>> On Mon, 30 Nov 2020 16:18:50 -0500
>>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>>
>>>> Masami,
>>>>
>>>> Can you review this patch, and also, should this go to -rc and stable?
>>>>
>>>> -- Steve
>>> Thanks for ping me!
>>>
>>>> On Tue, 24 Nov 2020 19:57:19 +0800
>>>> Wang ShaoBo <bobo.shaobowang@huawei.com> wrote:
>>>>
>>>>> Our system encountered a re-init error when re-registering same kretprobe,
>>>>> where the kretprobe_instance in rp->free_instances is illegally accessed
>>>>> after re-init.
>>> Ah, OK. Anyway if re-register happens on kretprobe, it must lose instances
>>> on the list before checking re-register in register_kprobe().
>>> So the idea looks good to me.
>>>
>>>
>>>>> Implementation to avoid re-registration has been introduced for kprobe
>>>>> before, but lags for register_kretprobe(). We must check if kprobe has
>>>>> been re-registered before re-initializing kretprobe, otherwise it will
>>>>> destroy the data struct of kretprobe registered, which can lead to memory
>>>>> leak, system crash, also some unexpected behaviors.
>>>>>
>>>>> we use check_kprobe_rereg() to check if kprobe has been re-registered
>>>>> before calling register_kretprobe(), for giving a warning message and
>>>>> terminate registration process.
>>>>>
>>>>> Signed-off-by: Wang ShaoBo <bobo.shaobowang@huawei.com>
>>>>> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
>>>>> ---
>>>>>    kernel/kprobes.c | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>>>> index 41fdbb7953c6..7f54a70136f3 100644
>>>>> --- a/kernel/kprobes.c
>>>>> +++ b/kernel/kprobes.c
>>>>> @@ -2117,6 +2117,14 @@ int register_kretprobe(struct kretprobe *rp)
>>>>>    		}
>>>>>    	}
>>>>>    
>>>>> +	/*
>>>>> +	 * Return error if it's being re-registered,
>>>>> +	 * also give a warning message to the developer.
>>>>> +	 */
>>>>> +	ret = check_kprobe_rereg(&rp->kp);
>>>>> +	if (WARN_ON(ret))
>>>>> +		return ret;
>>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
>>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
>>> this check. So it should be in between kprobe_on_func_entry() and
>>> kretprobe_blacklist_size check, like this
>>>
>>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>>> 		return -EINVAL;
>>>
>>> 	addr = kprobe_addr(&rp->kp);
>>> 	if (IS_ERR(addr))
>>> 		return PTR_ERR(addr);
>>> 	rp->kp.addr = addr;
>>>
>>> 	ret = check_kprobe_rereg(&rp->kp);
>>> 	if (WARN_ON(ret))
>>> 		return ret;
>>>
>>>           if (kretprobe_blacklist_size) {
>>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
>>>
>>>
>>> Thank you,
>>>
>>>
>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wangshaobo (bobo)]

Hi steven, Masami,
We have encountered a problem, when we attempted to use steven's suggestion as following,

>>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
>>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
>>> this check. So it should be in between kprobe_on_func_entry() and
>>> kretprobe_blacklist_size check, like this
>>>
>>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>>> 		return -EINVAL;
>>>
>>> 	addr = kprobe_addr(&rp->kp);
>>> 	if (IS_ERR(addr))
>>> 		return PTR_ERR(addr);
>>> 	rp->kp.addr = addr;

//there exists no-atomic operation risk, we should not modify any rp->kp's information, not all arch ensure atomic operation here.

>>>
>>> 	ret = check_kprobe_rereg(&rp->kp);
>>> 	if (WARN_ON(ret))
>>> 		return ret;
>>>
>>>           if (kretprobe_blacklist_size) {
>>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);

it returns failure from register_kprobe() end called by register_kretprobe() when
we registered a kretprobe through .symbol_name at first time(through .addr is OK),
kprobe_addr() called at the begaining of register_kprobe() will recheck and
failed at following place because at this time we symbol_name is not NULL and addr is also.

   static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
                          unsigned int offset)
    {
          if ((symbol_name && addr) || (!symbol_name && !addr))  //we failed here


So we attempted to move this sentence rp->kp.addr = addr to __get_valid_kprobe() like this to
avoid explict usage of rp->kp.addr = addr in register_kretprobe().

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index dd5821f753e6..ea014779edfe 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1502,10 +1502,15 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
  static struct kprobe *__get_valid_kprobe(struct kprobe *p)
  {
         struct kprobe *ap, *list_p;
+       void *addr;

         lockdep_assert_held(&kprobe_mutex);

-       ap = get_kprobe(p->addr);
+       addr = kprobe_addr(p);
+       if (IS_ERR(addr))
+               return NULL;
+
+       ap = get_kprobe(addr);
         if (unlikely(!ap))
                 return NULL;

But it also failed when we second time attempted to register a same kretprobe, it is also
becasue symbol_name and addr is not NULL when we used __get_valid_kprobe().

So it seems has no idea expect for modifying _kprobe_addr() like following this, the reason is that
the patch 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and kallsyms_on_each_symbol()")
has telled us we'd better use symbol name to register but not address anymore.

-static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
-                       const char *symbol_name, unsigned int offset)
+static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
+                       unsigned int offset)
  {
-       if ((symbol_name && addr) || (!symbol_name && !addr))
+       kprobe_opcode_t *addr;
+       if (!symbol_name)
                 goto invalid;

For us, this modification has not caused a big impact on other modules, only expects a little
influence on bpf from calling trace_kprobe_on_func_entry(), it can not use addr to fill in
rp.kp in struct trace_event_call anymore.

So i want to know your views, and i will resend this patch soon.

thanks,
Wang Shaobo

>>>
>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Masami Hiramatsu]

On Mon, 21 Dec 2020 21:31:42 +0800
"Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:

> Hi steven, Masami,
> We have encountered a problem, when we attempted to use steven's suggestion as following,
> 
> >>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> >>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> >>> this check. So it should be in between kprobe_on_func_entry() and
> >>> kretprobe_blacklist_size check, like this
> >>>
> >>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> >>> 		return -EINVAL;
> >>>
> >>> 	addr = kprobe_addr(&rp->kp);
> >>> 	if (IS_ERR(addr))
> >>> 		return PTR_ERR(addr);
> >>> 	rp->kp.addr = addr;
> 
> //there exists no-atomic operation risk, we should not modify any rp->kp's information, not all arch ensure atomic operation here.
> 
> >>>
> >>> 	ret = check_kprobe_rereg(&rp->kp);
> >>> 	if (WARN_ON(ret))
> >>> 		return ret;
> >>>
> >>>           if (kretprobe_blacklist_size) {
> >>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
> 
> it returns failure from register_kprobe() end called by register_kretprobe() when
> we registered a kretprobe through .symbol_name at first time(through .addr is OK),
> kprobe_addr() called at the begaining of register_kprobe() will recheck and
> failed at following place because at this time we symbol_name is not NULL and addr is also.

Good catch! Yes, it will reject if both kp->addr and kp->symbol are set.

> 
>    static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
>                           unsigned int offset)
>     {
>           if ((symbol_name && addr) || (!symbol_name && !addr))  //we failed here
> 
> 
> So we attempted to move this sentence rp->kp.addr = addr to __get_valid_kprobe() like this to
> avoid explict usage of rp->kp.addr = addr in register_kretprobe().
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index dd5821f753e6..ea014779edfe 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1502,10 +1502,15 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>   static struct kprobe *__get_valid_kprobe(struct kprobe *p)
>   {
>          struct kprobe *ap, *list_p;
> +       void *addr;
> 
>          lockdep_assert_held(&kprobe_mutex);
> 
> -       ap = get_kprobe(p->addr);
> +       addr = kprobe_addr(p);
> +       if (IS_ERR(addr))
> +               return NULL;
> +
> +       ap = get_kprobe(addr);
>          if (unlikely(!ap))
>                  return NULL;
> 
> But it also failed when we second time attempted to register a same kretprobe, it is also
> becasue symbol_name and addr is not NULL when we used __get_valid_kprobe().

What the "second time" means? If you reuse the kretprobe (and kprobe) you must
reset (cleanup) the kp->addr or kp->symbol_name. That is the initial state.
I think the API should not allow users to enter inconsistent information.

> 
> So it seems has no idea expect for modifying _kprobe_addr() like following this, the reason is that
> the patch 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and kallsyms_on_each_symbol()")
> has telled us we'd better use symbol name to register but not address anymore.
> 
> -static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
> -                       const char *symbol_name, unsigned int offset)
> +static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
> +                       unsigned int offset)
>   {
> -       if ((symbol_name && addr) || (!symbol_name && !addr))
> +       kprobe_opcode_t *addr;
> +       if (!symbol_name)
>                  goto invalid;

No, there are cases that the user will set only kp->addr, but no kp->symbol_name.

> 
> For us, this modification has not caused a big impact on other modules, only expects a little
> influence on bpf from calling trace_kprobe_on_func_entry(), it can not use addr to fill in
> rp.kp in struct trace_event_call anymore.
> 
> So i want to know your views, and i will resend this patch soon.

OK, I think it is simpler to check the rp->kp.addr && rp->kp.symbol_name
because it is not allowed (it can lead inconsistent setting).

How about this code? Is this work for you?

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 41fdbb7953c6..73500be564be 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2103,6 +2103,14 @@ int register_kretprobe(struct kretprobe *rp)
        int i;
        void *addr;
 
+       /* It is not allowed to specify addr and symbol_name at the same time */
+       if (rp->kp.addr && rp->kp.symbol_name)
+               return -EINVAL;
+
+       /* If only rp->kp.addr is specified, check reregistering kprobes */
+       if (rp->kp.addr && check_kprobe_rereg(&rp->kp))
+               return -EINVAL;
+
        if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
                return -EINVAL;
 

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Steven Rostedt]

Anything more on this?

-- Steve


On Tue, 22 Dec 2020 20:03:56 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Mon, 21 Dec 2020 21:31:42 +0800
> "Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:
> 
> > Hi steven, Masami,
> > We have encountered a problem, when we attempted to use steven's suggestion as following,
> >   
> > >>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> > >>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> > >>> this check. So it should be in between kprobe_on_func_entry() and
> > >>> kretprobe_blacklist_size check, like this
> > >>>
> > >>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> > >>> 		return -EINVAL;
> > >>>
> > >>> 	addr = kprobe_addr(&rp->kp);
> > >>> 	if (IS_ERR(addr))
> > >>> 		return PTR_ERR(addr);
> > >>> 	rp->kp.addr = addr;  
> > 
> > //there exists no-atomic operation risk, we should not modify any rp->kp's information, not all arch ensure atomic operation here.
> >   
> > >>>
> > >>> 	ret = check_kprobe_rereg(&rp->kp);
> > >>> 	if (WARN_ON(ret))
> > >>> 		return ret;
> > >>>
> > >>>           if (kretprobe_blacklist_size) {
> > >>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);  
> > 
> > it returns failure from register_kprobe() end called by register_kretprobe() when
> > we registered a kretprobe through .symbol_name at first time(through .addr is OK),
> > kprobe_addr() called at the begaining of register_kprobe() will recheck and
> > failed at following place because at this time we symbol_name is not NULL and addr is also.  
> 
> Good catch! Yes, it will reject if both kp->addr and kp->symbol are set.
> 
> > 
> >    static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
> >                           unsigned int offset)
> >     {
> >           if ((symbol_name && addr) || (!symbol_name && !addr))  //we failed here
> > 
> > 
> > So we attempted to move this sentence rp->kp.addr = addr to __get_valid_kprobe() like this to
> > avoid explict usage of rp->kp.addr = addr in register_kretprobe().
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index dd5821f753e6..ea014779edfe 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1502,10 +1502,15 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> >   static struct kprobe *__get_valid_kprobe(struct kprobe *p)
> >   {
> >          struct kprobe *ap, *list_p;
> > +       void *addr;
> > 
> >          lockdep_assert_held(&kprobe_mutex);
> > 
> > -       ap = get_kprobe(p->addr);
> > +       addr = kprobe_addr(p);
> > +       if (IS_ERR(addr))
> > +               return NULL;
> > +
> > +       ap = get_kprobe(addr);
> >          if (unlikely(!ap))
> >                  return NULL;
> > 
> > But it also failed when we second time attempted to register a same kretprobe, it is also
> > becasue symbol_name and addr is not NULL when we used __get_valid_kprobe().  
> 
> What the "second time" means? If you reuse the kretprobe (and kprobe) you must
> reset (cleanup) the kp->addr or kp->symbol_name. That is the initial state.
> I think the API should not allow users to enter inconsistent information.
> 
> > 
> > So it seems has no idea expect for modifying _kprobe_addr() like following this, the reason is that
> > the patch 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and kallsyms_on_each_symbol()")
> > has telled us we'd better use symbol name to register but not address anymore.
> > 
> > -static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
> > -                       const char *symbol_name, unsigned int offset)
> > +static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
> > +                       unsigned int offset)
> >   {
> > -       if ((symbol_name && addr) || (!symbol_name && !addr))
> > +       kprobe_opcode_t *addr;
> > +       if (!symbol_name)
> >                  goto invalid;  
> 
> No, there are cases that the user will set only kp->addr, but no kp->symbol_name.
> 
> > 
> > For us, this modification has not caused a big impact on other modules, only expects a little
> > influence on bpf from calling trace_kprobe_on_func_entry(), it can not use addr to fill in
> > rp.kp in struct trace_event_call anymore.
> > 
> > So i want to know your views, and i will resend this patch soon.  
> 
> OK, I think it is simpler to check the rp->kp.addr && rp->kp.symbol_name
> because it is not allowed (it can lead inconsistent setting).
> 
> How about this code? Is this work for you?
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 41fdbb7953c6..73500be564be 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2103,6 +2103,14 @@ int register_kretprobe(struct kretprobe *rp)
>         int i;
>         void *addr;
>  
> +       /* It is not allowed to specify addr and symbol_name at the same time */
> +       if (rp->kp.addr && rp->kp.symbol_name)
> +               return -EINVAL;
> +
> +       /* If only rp->kp.addr is specified, check reregistering kprobes */
> +       if (rp->kp.addr && check_kprobe_rereg(&rp->kp))
> +               return -EINVAL;
> +
>         if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>                 return -EINVAL;
>  
> 
> Thank you,
> 

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Masami Hiramatsu]

On Wed, 13 Jan 2021 17:48:45 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Anything more on this?

I need Wangshaobo's confirmation, because this is essentially a kind of programming bug,
not a runtime bug. kprobes user must check the kprobe(kretprobe) must be unregistered
and cleaned up before reusing it. (I recommend to re-alloc new data structure each time)

For example, if you re-register your driver/filesystem without releasing, it will
break the kernel.

Thank you,

> 
> -- Steve
> 
> 
> On Tue, 22 Dec 2020 20:03:56 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Mon, 21 Dec 2020 21:31:42 +0800
> > "Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:
> > 
> > > Hi steven, Masami,
> > > We have encountered a problem, when we attempted to use steven's suggestion as following,
> > >   
> > > >>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
> > > >>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
> > > >>> this check. So it should be in between kprobe_on_func_entry() and
> > > >>> kretprobe_blacklist_size check, like this
> > > >>>
> > > >>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> > > >>> 		return -EINVAL;
> > > >>>
> > > >>> 	addr = kprobe_addr(&rp->kp);
> > > >>> 	if (IS_ERR(addr))
> > > >>> 		return PTR_ERR(addr);
> > > >>> 	rp->kp.addr = addr;  
> > > 
> > > //there exists no-atomic operation risk, we should not modify any rp->kp's information, not all arch ensure atomic operation here.
> > >   
> > > >>>
> > > >>> 	ret = check_kprobe_rereg(&rp->kp);
> > > >>> 	if (WARN_ON(ret))
> > > >>> 		return ret;
> > > >>>
> > > >>>           if (kretprobe_blacklist_size) {
> > > >>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);  
> > > 
> > > it returns failure from register_kprobe() end called by register_kretprobe() when
> > > we registered a kretprobe through .symbol_name at first time(through .addr is OK),
> > > kprobe_addr() called at the begaining of register_kprobe() will recheck and
> > > failed at following place because at this time we symbol_name is not NULL and addr is also.  
> > 
> > Good catch! Yes, it will reject if both kp->addr and kp->symbol are set.
> > 
> > > 
> > >    static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
> > >                           unsigned int offset)
> > >     {
> > >           if ((symbol_name && addr) || (!symbol_name && !addr))  //we failed here
> > > 
> > > 
> > > So we attempted to move this sentence rp->kp.addr = addr to __get_valid_kprobe() like this to
> > > avoid explict usage of rp->kp.addr = addr in register_kretprobe().
> > > 
> > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > index dd5821f753e6..ea014779edfe 100644
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -1502,10 +1502,15 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
> > >   static struct kprobe *__get_valid_kprobe(struct kprobe *p)
> > >   {
> > >          struct kprobe *ap, *list_p;
> > > +       void *addr;
> > > 
> > >          lockdep_assert_held(&kprobe_mutex);
> > > 
> > > -       ap = get_kprobe(p->addr);
> > > +       addr = kprobe_addr(p);
> > > +       if (IS_ERR(addr))
> > > +               return NULL;
> > > +
> > > +       ap = get_kprobe(addr);
> > >          if (unlikely(!ap))
> > >                  return NULL;
> > > 
> > > But it also failed when we second time attempted to register a same kretprobe, it is also
> > > becasue symbol_name and addr is not NULL when we used __get_valid_kprobe().  
> > 
> > What the "second time" means? If you reuse the kretprobe (and kprobe) you must
> > reset (cleanup) the kp->addr or kp->symbol_name. That is the initial state.
> > I think the API should not allow users to enter inconsistent information.
> > 
> > > 
> > > So it seems has no idea expect for modifying _kprobe_addr() like following this, the reason is that
> > > the patch 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and kallsyms_on_each_symbol()")
> > > has telled us we'd better use symbol name to register but not address anymore.
> > > 
> > > -static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
> > > -                       const char *symbol_name, unsigned int offset)
> > > +static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
> > > +                       unsigned int offset)
> > >   {
> > > -       if ((symbol_name && addr) || (!symbol_name && !addr))
> > > +       kprobe_opcode_t *addr;
> > > +       if (!symbol_name)
> > >                  goto invalid;  
> > 
> > No, there are cases that the user will set only kp->addr, but no kp->symbol_name.
> > 
> > > 
> > > For us, this modification has not caused a big impact on other modules, only expects a little
> > > influence on bpf from calling trace_kprobe_on_func_entry(), it can not use addr to fill in
> > > rp.kp in struct trace_event_call anymore.
> > > 
> > > So i want to know your views, and i will resend this patch soon.  
> > 
> > OK, I think it is simpler to check the rp->kp.addr && rp->kp.symbol_name
> > because it is not allowed (it can lead inconsistent setting).
> > 
> > How about this code? Is this work for you?
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 41fdbb7953c6..73500be564be 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2103,6 +2103,14 @@ int register_kretprobe(struct kretprobe *rp)
> >         int i;
> >         void *addr;
> >  
> > +       /* It is not allowed to specify addr and symbol_name at the same time */
> > +       if (rp->kp.addr && rp->kp.symbol_name)
> > +               return -EINVAL;
> > +
> > +       /* If only rp->kp.addr is specified, check reregistering kprobes */
> > +       if (rp->kp.addr && check_kprobe_rereg(&rp->kp))
> > +               return -EINVAL;
> > +
> >         if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
> >                 return -EINVAL;
> >  
> > 
> > Thank you,
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wangshaobo (bobo)]

I have found other problems when following Masami's proposals,

I have been dealing with other things this two days and i will send 
patch as soon.


Thank you,

在 2021/1/14 8:25, Masami Hiramatsu 写道:
> On Wed, 13 Jan 2021 17:48:45 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> Anything more on this?
> I need Wangshaobo's confirmation, because this is essentially a kind of programming bug,
> not a runtime bug. kprobes user must check the kprobe(kretprobe) must be unregistered
> and cleaned up before reusing it. (I recommend to re-alloc new data structure each time)
>
> For example, if you re-register your driver/filesystem without releasing, it will
> break the kernel.
>
> Thank you,
>
>> -- Steve
>>
>>
>> On Tue, 22 Dec 2020 20:03:56 +0900
>> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>
>>> On Mon, 21 Dec 2020 21:31:42 +0800
>>> "Wangshaobo (bobo)" <bobo.shaobowang@huawei.com> wrote:
>>>
>>>> Hi steven, Masami,
>>>> We have encountered a problem, when we attempted to use steven's suggestion as following,
>>>>    
>>>>>>> If you call this here, you must make sure kprobe_addr() is called on rp->kp.
>>>>>>> But if kretprobe_blacklist_size == 0, kprobe_addr() is not called before
>>>>>>> this check. So it should be in between kprobe_on_func_entry() and
>>>>>>> kretprobe_blacklist_size check, like this
>>>>>>>
>>>>>>> 	if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>>>>>>> 		return -EINVAL;
>>>>>>>
>>>>>>> 	addr = kprobe_addr(&rp->kp);
>>>>>>> 	if (IS_ERR(addr))
>>>>>>> 		return PTR_ERR(addr);
>>>>>>> 	rp->kp.addr = addr;
>>>> //there exists no-atomic operation risk, we should not modify any rp->kp's information, not all arch ensure atomic operation here.
>>>>    
>>>>>>> 	ret = check_kprobe_rereg(&rp->kp);
>>>>>>> 	if (WARN_ON(ret))
>>>>>>> 		return ret;
>>>>>>>
>>>>>>>            if (kretprobe_blacklist_size) {
>>>>>>> 		for (i = 0; > > +	ret = check_kprobe_rereg(&rp->kp);
>>>> it returns failure from register_kprobe() end called by register_kretprobe() when
>>>> we registered a kretprobe through .symbol_name at first time(through .addr is OK),
>>>> kprobe_addr() called at the begaining of register_kprobe() will recheck and
>>>> failed at following place because at this time we symbol_name is not NULL and addr is also.
>>> Good catch! Yes, it will reject if both kp->addr and kp->symbol are set.
>>>
>>>>     static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
>>>>                            unsigned int offset)
>>>>      {
>>>>            if ((symbol_name && addr) || (!symbol_name && !addr))  //we failed here
>>>>
>>>>
>>>> So we attempted to move this sentence rp->kp.addr = addr to __get_valid_kprobe() like this to
>>>> avoid explict usage of rp->kp.addr = addr in register_kretprobe().
>>>>
>>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>>> index dd5821f753e6..ea014779edfe 100644
>>>> --- a/kernel/kprobes.c
>>>> +++ b/kernel/kprobes.c
>>>> @@ -1502,10 +1502,15 @@ static kprobe_opcode_t *kprobe_addr(struct kprobe *p)
>>>>    static struct kprobe *__get_valid_kprobe(struct kprobe *p)
>>>>    {
>>>>           struct kprobe *ap, *list_p;
>>>> +       void *addr;
>>>>
>>>>           lockdep_assert_held(&kprobe_mutex);
>>>>
>>>> -       ap = get_kprobe(p->addr);
>>>> +       addr = kprobe_addr(p);
>>>> +       if (IS_ERR(addr))
>>>> +               return NULL;
>>>> +
>>>> +       ap = get_kprobe(addr);
>>>>           if (unlikely(!ap))
>>>>                   return NULL;
>>>>
>>>> But it also failed when we second time attempted to register a same kretprobe, it is also
>>>> becasue symbol_name and addr is not NULL when we used __get_valid_kprobe().
>>> What the "second time" means? If you reuse the kretprobe (and kprobe) you must
>>> reset (cleanup) the kp->addr or kp->symbol_name. That is the initial state.
>>> I think the API should not allow users to enter inconsistent information.
>>>
>>>> So it seems has no idea expect for modifying _kprobe_addr() like following this, the reason is that
>>>> the patch 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and kallsyms_on_each_symbol()")
>>>> has telled us we'd better use symbol name to register but not address anymore.
>>>>
>>>> -static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>>>> -                       const char *symbol_name, unsigned int offset)
>>>> +static kprobe_opcode_t *_kprobe_addr(const char *symbol_name,
>>>> +                       unsigned int offset)
>>>>    {
>>>> -       if ((symbol_name && addr) || (!symbol_name && !addr))
>>>> +       kprobe_opcode_t *addr;
>>>> +       if (!symbol_name)
>>>>                   goto invalid;
>>> No, there are cases that the user will set only kp->addr, but no kp->symbol_name.
>>>
>>>> For us, this modification has not caused a big impact on other modules, only expects a little
>>>> influence on bpf from calling trace_kprobe_on_func_entry(), it can not use addr to fill in
>>>> rp.kp in struct trace_event_call anymore.
>>>>
>>>> So i want to know your views, and i will resend this patch soon.
>>> OK, I think it is simpler to check the rp->kp.addr && rp->kp.symbol_name
>>> because it is not allowed (it can lead inconsistent setting).
>>>
>>> How about this code? Is this work for you?
>>>
>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>> index 41fdbb7953c6..73500be564be 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>>> @@ -2103,6 +2103,14 @@ int register_kretprobe(struct kretprobe *rp)
>>>          int i;
>>>          void *addr;
>>>   
>>> +       /* It is not allowed to specify addr and symbol_name at the same time */
>>> +       if (rp->kp.addr && rp->kp.symbol_name)
>>> +               return -EINVAL;
>>> +
>>> +       /* If only rp->kp.addr is specified, check reregistering kprobes */
>>> +       if (rp->kp.addr && check_kprobe_rereg(&rp->kp))
>>> +               return -EINVAL;
>>> +
>>>          if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>>>                  return -EINVAL;
>>>   
>>>
>>> Thank you,
>>>
>

Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier

[Wangshaobo (bobo)]

Dear Masami and Steve,

I have sent v2 but still have confusions:

> OK, I think it is simpler to check the rp->kp.addr && rp->kp.symbol_name
> because it is not allowed (it can lead inconsistent setting).
>
> How about this code? Is this work for you?
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 41fdbb7953c6..73500be564be 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2103,6 +2103,14 @@ int register_kretprobe(struct kretprobe *rp)
>          int i;
>          void *addr;
>   
> +       /* It is not allowed to specify addr and symbol_name at the same time */
> +       if (rp->kp.addr && rp->kp.symbol_name)
> +               return -EINVAL;
> +

above sentence can be removed because of kprobe_on_func_entry() do it:

kprobe_on_func_entry()

      -=>_kprobe_addr() {if (rp->kp.addr && rp->kp.symbol_name) ...}

> +       /* If only rp->kp.addr is specified, check reregistering kprobes */
> +       if (rp->kp.addr && check_kprobe_rereg(&rp->kp))
> +               return -EINVAL;

for arch arm64，x86_64, above sentence can be moved behind following 
sentence.

kprobe_on_func_entry()

     -=>arch_kprobe_on_func_entry() {kp->offset can not be 0 ; ...}

So if offset of kprobe if not 0, do not waste time to excute above sentence.


But for Arch ppc64,  I still not figure out better one solution.


Thank you

-- Wang ShaoBo

>          if (!kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset))
>                  return -EINVAL;
>   
>
> Thank you,
>