[PATCH] bpf: hide do_bpf_send_signal when unused

[PATCH] bpf: hide do_bpf_send_signal when unused

[Arnd Bergmann]

When CONFIG_MODULES is disabled, this function is never called:

kernel/trace/bpf_trace.c:581:13: error: 'do_bpf_send_signal' defined but not used [-Werror=unused-function]

Add another #ifdef around it.

Fixes: 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/trace/bpf_trace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c102c240bb0b..b1a814e2d451 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -602,6 +602,7 @@ struct send_signal_irq_work {
 
 static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
 
+#ifdef CONFIG_MODULES
 static void do_bpf_send_signal(struct irq_work *entry)
 {
 	struct send_signal_irq_work *work;
@@ -609,6 +610,7 @@ static void do_bpf_send_signal(struct irq_work *entry)
 	work = container_of(entry, struct send_signal_irq_work, irq_work);
 	group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, PIDTYPE_TGID);
 }
+#endif
 
 BPF_CALL_1(bpf_send_signal, u32, sig)
 {
-- 
2.20.0

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Alexei Starovoitov]

On Mon, Jun 17, 2019 at 5:59 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> When CONFIG_MODULES is disabled, this function is never called:
>
> kernel/trace/bpf_trace.c:581:13: error: 'do_bpf_send_signal' defined but not used [-Werror=unused-function]

hmm. it should work just fine without modules.
the bug is somewhere else.

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Steven Rostedt]

On Mon, 17 Jun 2019 08:26:29 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Jun 17, 2019 at 5:59 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > When CONFIG_MODULES is disabled, this function is never called:
> >
> > kernel/trace/bpf_trace.c:581:13: error: 'do_bpf_send_signal' defined but not used [-Werror=unused-function]  
> 
> hmm. it should work just fine without modules.
> the bug is somewhere else.

From what I see, the only use of do_bpf_send_signal is within a
#ifdef CONFIG_MODULES, which means that you will get a warning about a
static unused when CONFIG_MODULES is not defined.

In kernel/trace/bpf_trace.c we have:

static void do_bpf_send_signal(struct irq_work *entry)

[..]

#ifdef CONFIG_MODULES

[..]

        for_each_possible_cpu(cpu) {
                work = per_cpu_ptr(&send_signal_work, cpu);
                init_irq_work(&work->irq_work, do_bpf_send_signal);  <-- on use of do_bpf_send_signal
        }
[..]
#endif /* CONFIG_MODULES */

The bug (really just a warning) reported is exactly here.

-- Steve

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Matt Mullins]

On Mon, 2019-06-17 at 19:09 -0400, Steven Rostedt wrote:
> On Mon, 17 Jun 2019 08:26:29 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > On Mon, Jun 17, 2019 at 5:59 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > 
> > > When CONFIG_MODULES is disabled, this function is never called:
> > > 
> > > kernel/trace/bpf_trace.c:581:13: error: 'do_bpf_send_signal' defined but not used [-Werror=unused-function]  
> > 
> > hmm. it should work just fine without modules.
> > the bug is somewhere else.
> 
> From what I see, the only use of do_bpf_send_signal is within a
> #ifdef CONFIG_MODULES, which means that you will get a warning about a
> static unused when CONFIG_MODULES is not defined.
> 
> In kernel/trace/bpf_trace.c we have:
> 
> static void do_bpf_send_signal(struct irq_work *entry)
> 
> [..]
> 
> #ifdef CONFIG_MODULES
> 
> [..]
> 
>         for_each_possible_cpu(cpu) {
>                 work = per_cpu_ptr(&send_signal_work, cpu);
>                 init_irq_work(&work->irq_work, do_bpf_send_signal);  <-- on use of do_bpf_send_signal
>         }
> [..]
> #endif /* CONFIG_MODULES */
> 
> The bug (really just a warning) reported is exactly here.

I don't think bpf_send_signal is tied to modules at all;
send_signal_irq_work_init and the corresponding initcall should be
moved outside that #ifdef.

> 
> -- Steve

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Alexei Starovoitov]

On Mon, Jun 17, 2019 at 4:13 PM Matt Mullins <mmullins@fb.com> wrote:
> >
> > The bug (really just a warning) reported is exactly here.
>
> I don't think bpf_send_signal is tied to modules at all;
> send_signal_irq_work_init and the corresponding initcall should be
> moved outside that #ifdef.

right. I guess send_signal_irq_work_init was accidentally placed
after bpf_event_init and happened to be within that ifdef.
Should definitely be outside.

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Steven Rostedt]

On Mon, 17 Jun 2019 16:27:33 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Jun 17, 2019 at 4:13 PM Matt Mullins <mmullins@fb.com> wrote:
> > >
> > > The bug (really just a warning) reported is exactly here.  
> >
> > I don't think bpf_send_signal is tied to modules at all;
> > send_signal_irq_work_init and the corresponding initcall should be
> > moved outside that #ifdef.  
> 
> right. I guess send_signal_irq_work_init was accidentally placed
> after bpf_event_init and happened to be within that ifdef.
> Should definitely be outside.

So Arnd did find a bug. Just the wrong solution ;-)

-- Steve

Re: [PATCH] bpf: hide do_bpf_send_signal when unused

[Yonghong Song]

On 6/17/19 5:18 PM, Steven Rostedt wrote:
> On Mon, 17 Jun 2019 16:27:33 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
>> On Mon, Jun 17, 2019 at 4:13 PM Matt Mullins <mmullins@fb.com> wrote:
>>>>
>>>> The bug (really just a warning) reported is exactly here.
>>>
>>> I don't think bpf_send_signal is tied to modules at all;
>>> send_signal_irq_work_init and the corresponding initcall should be
>>> moved outside that #ifdef.
>>
>> right. I guess send_signal_irq_work_init was accidentally placed
>> after bpf_event_init and happened to be within that ifdef.
>> Should definitely be outside.
> 
> So Arnd did find a bug. Just the wrong solution ;-)
> 
> -- Steve

Hi, Arnd,

The following change can fix the issue.

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c102c240bb0b..ca1255d14576 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1431,6 +1431,20 @@ int bpf_get_perf_event_info(const struct 
perf_event *event, u32 *prog_id,
         return err;
  }

+static int __init send_signal_irq_work_init(void)
+{
+       int cpu;
+       struct send_signal_irq_work *work;
+
+       for_each_possible_cpu(cpu) {
+               work = per_cpu_ptr(&send_signal_work, cpu);
+               init_irq_work(&work->irq_work, do_bpf_send_signal);
+       }
+       return 0;
+}
+
+subsys_initcall(send_signal_irq_work_init);
+
  #ifdef CONFIG_MODULES
  static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
                             void *module)
@@ -1478,18 +1492,5 @@ static int __init bpf_event_init(void)
         return 0;
  }

-static int __init send_signal_irq_work_init(void)
-{
-       int cpu;
-       struct send_signal_irq_work *work;
-
-       for_each_possible_cpu(cpu) {
-               work = per_cpu_ptr(&send_signal_work, cpu);
-               init_irq_work(&work->irq_work, do_bpf_send_signal);
-       }
-       return 0;
-}
-
  fs_initcall(bpf_event_init);
-subsys_initcall(send_signal_irq_work_init);
  #endif /* CONFIG_MODULES */

Could you submit a new revision? Thanks!

Yonghong