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

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Cheng Jian <[email protected]>
> ---
> 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;

2020-12-01 23:37:22

[permalink] [raw]

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

On Mon, 30 Nov 2020 16:18:50 -0500
Steven Rostedt <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Cheng Jian <[email protected]>
> > ---
> > 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 <[email protected]>

2020-12-02 01:26:45

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Cheng Jian <[email protected]>
>>> ---
>>> 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,
>
>

2020-12-02 01:29:59

[permalink] [raw]

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

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <[email protected]> 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

2020-12-14 18:14:40

[permalink] [raw]

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

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> Signed-off-by: Cheng Jian <[email protected]>
> >>> ---
> >>> 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,
> >
> >

2020-12-15 03:34:51

[permalink] [raw]

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

Hi ShaoBo,

On Wed, 2 Dec 2020 09:23:35 +0800
"Wangshaobo (bobo)" <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> Signed-off-by: Cheng Jian <[email protected]>
> >>> ---
> >>> 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 <[email protected]>

2020-12-15 08:44:25

[permalink] [raw]

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

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)" <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Cheng Jian <[email protected]>
>>>>> ---
>>>>> 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,
>>>
>>>
>

2020-12-21 13:33:34

[permalink] [raw]

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

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

>>>
>

2020-12-22 11:05:38

[permalink] [raw]

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

On Mon, 21 Dec 2020 21:31:42 +0800
"Wangshaobo (bobo)" <[email protected]> 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 <[email protected]>

2021-01-14 01:47:02

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]> wrote:
>>
>>> On Mon, 21 Dec 2020 21:31:42 +0800
>>> "Wangshaobo (bobo)" <[email protected]> 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,
>>>
>

2021-01-14 01:52:41

[permalink] [raw]

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

On Wed, 13 Jan 2021 17:48:45 -0500
Steven Rostedt <[email protected]> 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 <[email protected]> wrote:
>
> > On Mon, 21 Dec 2020 21:31:42 +0800
> > "Wangshaobo (bobo)" <[email protected]> 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 <[email protected]>

2021-01-14 01:57:56

[permalink] [raw]

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

Anything more on this?

-- Steve

On Tue, 22 Dec 2020 20:03:56 +0900
Masami Hiramatsu <[email protected]> wrote:

> On Mon, 21 Dec 2020 21:31:42 +0800
> "Wangshaobo (bobo)" <[email protected]> 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,
>

2021-01-29 03:41:10