* [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier @ 2020-11-24 11:57 Wang ShaoBo 2020-11-30 21:18 ` Steven Rostedt 0 siblings, 1 reply; 14+ messages in thread From: Wang ShaoBo @ 2020-11-24 11:57 UTC (permalink / raw) To: naveen.n.rao Cc: anil.s.keshavamurthy, davem, rostedt, linux-kernel, huawei.libin, cj.chengjian 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-11-24 11:57 [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier Wang ShaoBo @ 2020-11-30 21:18 ` Steven Rostedt 2020-12-01 23:32 ` Masami Hiramatsu 0 siblings, 1 reply; 14+ messages in thread From: Steven Rostedt @ 2020-11-30 21:18 UTC (permalink / raw) To: Masami Hiramatsu Cc: Wang ShaoBo, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-11-30 21:18 ` Steven Rostedt @ 2020-12-01 23:32 ` Masami Hiramatsu 2020-12-02 1:23 ` Wangshaobo (bobo) 0 siblings, 1 reply; 14+ messages in thread From: Masami Hiramatsu @ 2020-12-01 23:32 UTC (permalink / raw) To: Steven Rostedt Cc: Wang ShaoBo, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-01 23:32 ` Masami Hiramatsu @ 2020-12-02 1:23 ` Wangshaobo (bobo) 2020-12-02 1:27 ` Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Wangshaobo (bobo) @ 2020-12-02 1:23 UTC (permalink / raw) To: Masami Hiramatsu, Steven Rostedt Cc: naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-02 1:23 ` Wangshaobo (bobo) @ 2020-12-02 1:27 ` Steven Rostedt 2020-12-14 16:36 ` Steven Rostedt 2020-12-15 3:31 ` Masami Hiramatsu 2 siblings, 0 replies; 14+ messages in thread From: Steven Rostedt @ 2020-12-02 1:27 UTC (permalink / raw) To: Wangshaobo (bobo) Cc: Masami Hiramatsu, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-02 1:23 ` Wangshaobo (bobo) 2020-12-02 1:27 ` Steven Rostedt @ 2020-12-14 16:36 ` Steven Rostedt 2020-12-15 3:31 ` Masami Hiramatsu 2 siblings, 0 replies; 14+ messages in thread From: Steven Rostedt @ 2020-12-14 16:36 UTC (permalink / raw) To: Wangshaobo (bobo) Cc: Masami Hiramatsu, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-02 1:23 ` Wangshaobo (bobo) 2020-12-02 1:27 ` Steven Rostedt 2020-12-14 16:36 ` Steven Rostedt @ 2020-12-15 3:31 ` Masami Hiramatsu 2020-12-15 8:39 ` Wangshaobo (bobo) 2020-12-21 13:31 ` Wangshaobo (bobo) 2 siblings, 2 replies; 14+ messages in thread From: Masami Hiramatsu @ 2020-12-15 3:31 UTC (permalink / raw) To: Wangshaobo (bobo) Cc: Steven Rostedt, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-15 3:31 ` Masami Hiramatsu @ 2020-12-15 8:39 ` Wangshaobo (bobo) 2020-12-21 13:31 ` Wangshaobo (bobo) 1 sibling, 0 replies; 14+ messages in thread From: Wangshaobo (bobo) @ 2020-12-15 8:39 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, >>> >>> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-15 3:31 ` Masami Hiramatsu 2020-12-15 8:39 ` Wangshaobo (bobo) @ 2020-12-21 13:31 ` Wangshaobo (bobo) 2020-12-22 11:03 ` Masami Hiramatsu 1 sibling, 1 reply; 14+ messages in thread From: Wangshaobo (bobo) @ 2020-12-21 13:31 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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 >>> > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-21 13:31 ` Wangshaobo (bobo) @ 2020-12-22 11:03 ` Masami Hiramatsu 2021-01-13 22:48 ` Steven Rostedt 2021-01-29 3:37 ` Wangshaobo (bobo) 0 siblings, 2 replies; 14+ messages in thread From: Masami Hiramatsu @ 2020-12-22 11:03 UTC (permalink / raw) To: Wangshaobo (bobo) Cc: Steven Rostedt, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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> ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-22 11:03 ` Masami Hiramatsu @ 2021-01-13 22:48 ` Steven Rostedt 2021-01-14 0:25 ` Masami Hiramatsu 2021-01-29 3:37 ` Wangshaobo (bobo) 1 sibling, 1 reply; 14+ messages in thread From: Steven Rostedt @ 2021-01-13 22:48 UTC (permalink / raw) To: Masami Hiramatsu Cc: Wangshaobo (bobo), naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2021-01-13 22:48 ` Steven Rostedt @ 2021-01-14 0:25 ` Masami Hiramatsu 2021-01-14 1:06 ` Wangshaobo (bobo) 0 siblings, 1 reply; 14+ messages in thread From: Masami Hiramatsu @ 2021-01-14 0:25 UTC (permalink / raw) To: Steven Rostedt Cc: Wangshaobo (bobo), naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2021-01-14 0:25 ` Masami Hiramatsu @ 2021-01-14 1:06 ` Wangshaobo (bobo) 0 siblings, 0 replies; 14+ messages in thread From: Wangshaobo (bobo) @ 2021-01-14 1:06 UTC (permalink / raw) To: Masami Hiramatsu, Steven Rostedt Cc: naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, >>> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier 2020-12-22 11:03 ` Masami Hiramatsu 2021-01-13 22:48 ` Steven Rostedt @ 2021-01-29 3:37 ` Wangshaobo (bobo) 1 sibling, 0 replies; 14+ messages in thread From: Wangshaobo (bobo) @ 2021-01-29 3:37 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, naveen.n.rao, anil.s.keshavamurthy, davem, linux-kernel, huawei.libin, cj.chengjian 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, > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-29 3:38 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-24 11:57 [PATCH] kretprobe: avoid re-registration of the same kretprobe earlier Wang ShaoBo 2020-11-30 21:18 ` Steven Rostedt 2020-12-01 23:32 ` Masami Hiramatsu 2020-12-02 1:23 ` Wangshaobo (bobo) 2020-12-02 1:27 ` Steven Rostedt 2020-12-14 16:36 ` Steven Rostedt 2020-12-15 3:31 ` Masami Hiramatsu 2020-12-15 8:39 ` Wangshaobo (bobo) 2020-12-21 13:31 ` Wangshaobo (bobo) 2020-12-22 11:03 ` Masami Hiramatsu 2021-01-13 22:48 ` Steven Rostedt 2021-01-14 0:25 ` Masami Hiramatsu 2021-01-14 1:06 ` Wangshaobo (bobo) 2021-01-29 3:37 ` Wangshaobo (bobo)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).