Message-ID: <20210602014608.wxzfsgzuq7rut4ra@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 1 Jun 2021 18:46:08 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     davem@...emloft.net, daniel@...earbox.net, andrii@...nel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf-next 1/3] bpf: Introduce bpf_timer

On Thu, May 27, 2021 at 06:57:17PM +0200, Toke Høiland-Jørgensen wrote:
> >     if (val) {
> >         bpf_timer_init(&val->timer, timer_cb2, 0);
> >         bpf_timer_start(&val->timer, 1000 /* call timer_cb2 in 1 msec */);
> 
> nit: there are 1M nanoseconds in a millisecond :)

oops :)

> >     }
> > }
> >
> > This patch adds helper implementations that rely on hrtimers
> > to call bpf functions as timers expire.
> > The following patch adds necessary safety checks.
> >
> > Only programs with CAP_BPF are allowed to use bpf_timer.
> >
> > The amount of timers used by the program is constrained by
> > the memcg recorded at map creation time.
> >
> > The bpf_timer_init() helper is receiving hidden 'map' and 'prog' arguments
> > supplied by the verifier. The prog pointer is needed to do refcnting of bpf
> > program to make sure that program doesn't get freed while timer is armed.
> >
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> 
> Overall this LGTM, and I believe it will be usable for my intended use
> case. One question:
> 
> With this, it will basically be possible to create a BPF daemon, won't
> it? I.e., if a program includes a timer and the callback keeps re-arming
> itself this will continue indefinitely even if userspace closes all refs
> to maps and programs? Not saying this is a problem, just wanted to check
> my understanding (i.e., that there's not some hidden requirement on
> userspace keeping a ref open that I'm missing)...

That is correct.
Another option would be to auto-cancel the timer when the last reference
to the prog drops. That may feel safer, since forever
running bpf daemon is a certainly new concept.
The main benefits of doing prog_refcnt++ from bpf_timer_start are ease
of use and no surprises.
Disappearing timer callback when prog unloads is outside of bpf prog control.
For example the tracing bpf prog might collect some data and periodically
flush it to user space. The prog would arm the timer and when callback
is invoked it would send the data via ring buffer and start another
data collection cycle.
When the user space part of the service exits it doesn't have
an ability to enforce the flush of the last chunk of data.
It could do prog_run cmd that will call the timer callback,
but it's racy.
The solution to this problem could be __init and __fini
sections that will be invoked when the prog is loaded
and when the last refcnt drops.
It's a complementary feature though.
The prog_refcnt++ from bpf_timer_start combined with a prog
explicitly doing bpf_timer_cancel from __fini
would be the most flexible combination.
This way the prog can choose to be a daemon or it can choose
to cancel its timers and do data flushing when the last prog
reference drops.
The prog refcnt would be split (similar to uref). The __fini callback
will be invoked when refcnt reaches zero, but all increments
done by bpf_timer_start will be counted separately.
The user space wouldn't need to do the prog_run command.
It would detach the prog and close(prog_fd).
That will trigger __fini callback that will cancel the timers
and the prog will be fully unloaded.
That would make bpf progs resemble kernel modules even more.

> > +BPF_CALL_5(bpf_timer_init, struct bpf_timer_kern *, timer, void *, cb, int, flags,
> > +	   struct bpf_map *, map, struct bpf_prog *, prog)
> > +{
> > +	struct bpf_hrtimer *t;
> > +
> > +	if (flags)
> > +		return -EINVAL;
> > +	if (READ_ONCE(timer->timer))
> > +		return -EBUSY;
> > +	/* allocate hrtimer via map_kmalloc to use memcg accounting */
> > +	t = bpf_map_kmalloc_node(map, sizeof(*t), GFP_ATOMIC, NUMA_NO_NODE);
> > +	if (!t)
> > +		return -ENOMEM;
> > +	t->callback_fn = cb;
> > +	t->value = (void *)timer /* - offset of bpf_timer inside elem */;
> > +	t->key = t->value - round_up(map->key_size, 8);
> 
> For array-maps won't this just point to somewhere inside the previous value?

Excellent catch. Thank you. Will fix.

> > +	t->map = map;
> > +	t->prog = prog;
> > +	spin_lock_init(&t->lock);
> > +	hrtimer_init(&t->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> > +	t->timer.function = timer_cb;
> > +	if (cmpxchg(&timer->timer, NULL, t)) {
> > +		/* Parallel bpf_timer_init() calls raced. */
> > +		kfree(t);
> > +		return -EBUSY;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_timer_init_proto = {
> > +	.func		= bpf_timer_init,
> > +	.gpl_only	= false,
> 
> hrtimer_init() is EXPORT_SYMBOL_GPL, should this be as well? Same with
> the others below.

Excellent catch as well! Will fix.

Powered by blists - more mailing lists