Re: [PATCH net-next v2 02/15] bpf: offload: add infrastructure for loading programs for a specific netdev

[Jakub Kicinski <jakub.kicinski@netronome.com>]

Hi!

Sorry for the delay!

On Mon, 06 Nov 2017 18:32:45 +0100, Daniel Borkmann wrote:
> On 11/03/2017 09:56 PM, Jakub Kicinski wrote:
> > @@ -1549,6 +1555,8 @@ static void bpf_prog_free_deferred(struct work_struct *work)
> >   	struct bpf_prog_aux *aux;
> >
> >   	aux = container_of(work, struct bpf_prog_aux, work);
> > +	if (bpf_prog_is_dev_bound(aux))
> > +		bpf_prog_offload_destroy(aux->prog);
> >   	bpf_jit_free(aux->prog);
> >   }  
> [...]
> > +static int bpf_offload_notification(struct notifier_block *notifier,
> > +				    ulong event, void *ptr)
> > +{
> > +	struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> > +	struct bpf_dev_offload *offload, *tmp;
> > +
> > +	ASSERT_RTNL();
> > +
> > +	switch (event) {
> > +	case NETDEV_UNREGISTER:
> > +		list_for_each_entry_safe(offload, tmp, &bpf_prog_offload_devs,
> > +					 offloads) {
> > +			if (offload->netdev == netdev)
> > +				__bpf_prog_offload_destroy(offload->prog);  
> 
> We would be calling this twice, right? Once here and then on prog
> destruction again. __bpf_prog_offload_destroy() looks it will handle
> this just fine, but we should probably add a comment to
> __bpf_prog_offload_destroy() such that when changes are made to it
> it's obvious that we need to be extra careful.

Good point, I will add the comment.

> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 323be2473c4b..1574b9f0f24e 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -824,7 +824,10 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog)
> >   	if (type >= ARRAY_SIZE(bpf_prog_types) || !bpf_prog_types[type])
> >   		return -EINVAL;
> >
> > -	prog->aux->ops = bpf_prog_types[type];
> > +	if (!bpf_prog_is_dev_bound(prog->aux))
> > +		prog->aux->ops = bpf_prog_types[type];
> > +	else
> > +		prog->aux->ops = &bpf_offload_prog_ops;
> >   	prog->type = type;
> >   	return 0;
> >   }
> > @@ -1054,7 +1057,7 @@ struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
> >   }
> >   EXPORT_SYMBOL_GPL(bpf_prog_inc_not_zero);
> >
> > -static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)
> > +static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type)
> >   {
> >   	struct fd f = fdget(ufd);
> >   	struct bpf_prog *prog;
> > @@ -1062,7 +1065,7 @@ static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)
> >   	prog = ____bpf_prog_get(f);
> >   	if (IS_ERR(prog))
> >   		return prog;
> > -	if (type && prog->type != *type) {
> > +	if (attach_type && (prog->type != *attach_type || prog->aux->offload)) {
> >   		prog = ERR_PTR(-EINVAL);
> >   		goto out;
> >   	}
> > @@ -1089,7 +1092,7 @@ struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)
> >   EXPORT_SYMBOL_GPL(bpf_prog_get_type);
> >
> >   /* last field in 'union bpf_attr' used by this command */
> > -#define	BPF_PROG_LOAD_LAST_FIELD prog_name
> > +#define	BPF_PROG_LOAD_LAST_FIELD prog_target_ifindex  
> 
> For program types that are neither XDP nor cls_bpf, we should reject
> the request if something calls bpf(2) with non-0 prog_target_ifindex.
> 
> That way, i) we don't burn the whole field and could perhaps reuse/union
> it for other prog types like tracing in future. Probably makes sense to
> do anyway since ii) for types like tracing, we would want to reject this
> upfront here and not when later attach happens.
> 
> I probably missed something when reading the code, but if I spotted
> that correctly, we might otherwise even go and nfp-jit simple progs
> for non-networking types (we would bail out later though on in
> __bpf_prog_get() ... but we shouldn't let syscall return in first
> place)?

Agreed, I will fix this.