Re: [PATCH net-next v2 03/15] bpf: report offload info to user space
On Sat, 4 Nov 2017 18:45:31 +0900, Alexei Starovoitov wrote: > On Fri, Nov 03, 2017 at 01:56:18PM -0700, Jakub Kicinski wrote: > > Extend struct bpf_prog_info to contain information about program > > being bound to a device. Since the netdev may get destroyed while > > program still exists we need a flag to indicate the program is > > loaded for a device, even if the device is gone. > > > > Signed-off-by: Jakub Kicinski> > Reviewed-by: Simon Horman > > Reviewed-by: Quentin Monnet > > --- > > include/linux/bpf.h | 1 + > > include/uapi/linux/bpf.h | 6 ++ > > kernel/bpf/offload.c | 12 > > kernel/bpf/syscall.c | 5 + > > 4 files changed, 24 insertions(+) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index e45d43f9ec92..98bacd0fa5cc 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -506,6 +506,7 @@ static inline int cpu_map_enqueue(struct > > bpf_cpu_map_entry *rcpu, > > > > int bpf_prog_offload_compile(struct bpf_prog *prog); > > void bpf_prog_offload_destroy(struct bpf_prog *prog); > > +u32 bpf_prog_offload_ifindex(struct bpf_prog *prog); > > > > #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL) > > int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr); > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 727a3dba13e6..e92f62cf933a 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -894,6 +894,10 @@ enum sk_action { > > > > #define BPF_TAG_SIZE 8 > > > > +enum bpf_prog_status { > > + BPF_PROG_STATUS_DEV_BOUND = (1 << 0), > > +}; > > + > > struct bpf_prog_info { > > __u32 type; > > __u32 id; > > @@ -907,6 +911,8 @@ struct bpf_prog_info { > > __u32 nr_map_ids; > > __aligned_u64 map_ids; > > char name[BPF_OBJ_NAME_LEN]; > > + __u32 ifindex; > > + __u32 status; > > why status is needed? > ifindex cannot be zero, so if it's set > 0 would mean > that the program is bound. Devices may come and go, independently from the lifetime of the program, therefore there is a notion of a program which has been loaded for a particular device but the device is gone (and therefore its ifindex is meaningless). I tried to explain this in the commit message. > Also would be good to have consistent name with prog_load. > imo prog_target_ifindex is too long. > May be call it 'ifindex' both in bpf_attr and in bpf_prog_info ? Perhaps I'm missing something, but bpf_attr is a huge union of (mostly) unnamed anonymous structs. I foresee that we will have to add an ifindex member for a map command as well, therefore the prog_* prefix seems prudent. Should I go back to prog_ifindex in bpf_attr? Or perhaps should I duplicate the struct for BPF_PROG_LOAD but this time give it a member name so we can extend it without worrying about member name conflicts?
Re: [PATCH net-next v2 03/15] bpf: report offload info to user space
On Fri, Nov 03, 2017 at 01:56:18PM -0700, Jakub Kicinski wrote: > Extend struct bpf_prog_info to contain information about program > being bound to a device. Since the netdev may get destroyed while > program still exists we need a flag to indicate the program is > loaded for a device, even if the device is gone. > > Signed-off-by: Jakub Kicinski> Reviewed-by: Simon Horman > Reviewed-by: Quentin Monnet > --- > include/linux/bpf.h | 1 + > include/uapi/linux/bpf.h | 6 ++ > kernel/bpf/offload.c | 12 > kernel/bpf/syscall.c | 5 + > 4 files changed, 24 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index e45d43f9ec92..98bacd0fa5cc 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -506,6 +506,7 @@ static inline int cpu_map_enqueue(struct > bpf_cpu_map_entry *rcpu, > > int bpf_prog_offload_compile(struct bpf_prog *prog); > void bpf_prog_offload_destroy(struct bpf_prog *prog); > +u32 bpf_prog_offload_ifindex(struct bpf_prog *prog); > > #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL) > int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr); > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 727a3dba13e6..e92f62cf933a 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -894,6 +894,10 @@ enum sk_action { > > #define BPF_TAG_SIZE 8 > > +enum bpf_prog_status { > + BPF_PROG_STATUS_DEV_BOUND = (1 << 0), > +}; > + > struct bpf_prog_info { > __u32 type; > __u32 id; > @@ -907,6 +911,8 @@ struct bpf_prog_info { > __u32 nr_map_ids; > __aligned_u64 map_ids; > char name[BPF_OBJ_NAME_LEN]; > + __u32 ifindex; > + __u32 status; why status is needed? ifindex cannot be zero, so if it's set > 0 would mean that the program is bound. Also would be good to have consistent name with prog_load. imo prog_target_ifindex is too long. May be call it 'ifindex' both in bpf_attr and in bpf_prog_info ?
[PATCH net-next v2 03/15] bpf: report offload info to user space
Extend struct bpf_prog_info to contain information about program being bound to a device. Since the netdev may get destroyed while program still exists we need a flag to indicate the program is loaded for a device, even if the device is gone. Signed-off-by: Jakub KicinskiReviewed-by: Simon Horman Reviewed-by: Quentin Monnet --- include/linux/bpf.h | 1 + include/uapi/linux/bpf.h | 6 ++ kernel/bpf/offload.c | 12 kernel/bpf/syscall.c | 5 + 4 files changed, 24 insertions(+) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index e45d43f9ec92..98bacd0fa5cc 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -506,6 +506,7 @@ static inline int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, int bpf_prog_offload_compile(struct bpf_prog *prog); void bpf_prog_offload_destroy(struct bpf_prog *prog); +u32 bpf_prog_offload_ifindex(struct bpf_prog *prog); #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL) int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 727a3dba13e6..e92f62cf933a 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -894,6 +894,10 @@ enum sk_action { #define BPF_TAG_SIZE 8 +enum bpf_prog_status { + BPF_PROG_STATUS_DEV_BOUND = (1 << 0), +}; + struct bpf_prog_info { __u32 type; __u32 id; @@ -907,6 +911,8 @@ struct bpf_prog_info { __u32 nr_map_ids; __aligned_u64 map_ids; char name[BPF_OBJ_NAME_LEN]; + __u32 ifindex; + __u32 status; } __attribute__((aligned(8))); struct bpf_map_info { diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c index 5553e0e2f8b1..2816feb38be1 100644 --- a/kernel/bpf/offload.c +++ b/kernel/bpf/offload.c @@ -144,6 +144,18 @@ int bpf_prog_offload_compile(struct bpf_prog *prog) return bpf_prog_offload_translate(prog); } +u32 bpf_prog_offload_ifindex(struct bpf_prog *prog) +{ + struct bpf_dev_offload *offload = prog->aux->offload; + u32 ifindex; + + rtnl_lock(); + ifindex = offload->netdev ? offload->netdev->ifindex : 0; + rtnl_unlock(); + + return ifindex; +} + const struct bpf_prog_ops bpf_offload_prog_ops = { }; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 1574b9f0f24e..3217c20ea91b 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1592,6 +1592,11 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, return -EFAULT; } + if (bpf_prog_is_dev_bound(prog->aux)) { + info.status |= BPF_PROG_STATUS_DEV_BOUND; + info.ifindex = bpf_prog_offload_ifindex(prog); + } + done: if (copy_to_user(uinfo, , info_len) || put_user(info_len, >info.info_len)) -- 2.14.1