[net-next,v2,03/15] bpf: report offload info to user space

Message ID 20171103205630.1083-4-jakub.kicinski@netronome.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • bpf: add offload as a first class citizen
Related show

Commit Message

Jakub Kicinski Nov. 3, 2017, 8:56 p.m.
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 <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 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(+)

Comments

Alexei Starovoitov Nov. 4, 2017, 9:45 a.m. | #1
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 <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
>  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 ?
Jakub Kicinski Nov. 4, 2017, 10:32 a.m. | #2
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 <jakub.kicinski@netronome.com>
> > Reviewed-by: Simon Horman <simon.horman@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> > ---
> >  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?

Patch

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, info_len) ||
 	    put_user(info_len, &uattr->info.info_len))