diff mbox series

[net-next,v2,05/15] xdp: allow attaching programs loaded for specific device

Message ID 20171103205630.1083-6-jakub.kicinski@netronome.com
State Accepted, archived
Delegated to: David Miller
Headers show
Series bpf: add offload as a first class citizen | expand

Commit Message

Jakub Kicinski Nov. 3, 2017, 8:56 p.m. UTC 
Pass the netdev pointer to bpf_prog_get_type().  This way
BPF code can decide whether the device matches what the
code was loaded/translated for.

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  | 10 ++++++++++
 kernel/bpf/syscall.c | 33 +++++++++++++++++++++++++++++----
 net/core/dev.c       |  6 +++++-
 3 files changed, 44 insertions(+), 5 deletions(-)

Comments

Jiri Pirko Nov. 12, 2017, 9 a.m. UTC | #1 
Fri, Nov 03, 2017 at 09:56:20PM CET, jakub.kicinski@netronome.com wrote:
>Pass the netdev pointer to bpf_prog_get_type().  This way
>BPF code can decide whether the device matches what the
>code was loaded/translated for.
>

[...]


>diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>index 3217c20ea91b..68f9123acd39 100644
>--- a/kernel/bpf/syscall.c
>+++ b/kernel/bpf/syscall.c
>@@ -1057,7 +1057,22 @@ 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 *attach_type)
>+static bool bpf_prog_can_attach(struct bpf_prog *prog,
>+				enum bpf_prog_type *attach_type,
>+				struct net_device *netdev)
>+{
>+	struct bpf_dev_offload *offload = prog->aux->offload;
>+
>+	if (prog->type != *attach_type)
>+		return false;
>+	if (offload && offload->netdev != netdev)

This means you return false in case netdev function arg is NULL. Is that
intentional?

Seems wrong to me because for example in cls_bpf_prog_from_efd, you
would get an error in case TCA_CLS_FLAGS_SKIP_SW is not set.




>+		return false;
>+
>+	return true;
>+}
Daniel Borkmann Nov. 12, 2017, 7:33 p.m. UTC | #2 
On 11/12/2017 10:00 AM, Jiri Pirko wrote:
> Fri, Nov 03, 2017 at 09:56:20PM CET, jakub.kicinski@netronome.com wrote:
>> Pass the netdev pointer to bpf_prog_get_type().  This way
>> BPF code can decide whether the device matches what the
>> code was loaded/translated for.
> 
> [...]
> 
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 3217c20ea91b..68f9123acd39 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1057,7 +1057,22 @@ 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 *attach_type)
>> +static bool bpf_prog_can_attach(struct bpf_prog *prog,
>> +				enum bpf_prog_type *attach_type,
>> +				struct net_device *netdev)
>> +{
>> +	struct bpf_dev_offload *offload = prog->aux->offload;
>> +
>> +	if (prog->type != *attach_type)
>> +		return false;
>> +	if (offload && offload->netdev != netdev)
> 
> This means you return false in case netdev function arg is NULL. Is that
> intentional?
> 
> Seems wrong to me because for example in cls_bpf_prog_from_efd, you
> would get an error in case TCA_CLS_FLAGS_SKIP_SW is not set.

I think it's fine, the prog was originally loaded in the verifier pass
to be JITed via nfp JIT, so you'll have a valid prog->aux->offload, and
you're specifying TCA_CLS_FLAGS_SKIP_SW for fetching the qdisc dev where
we match devs above. So if that dev is not set (NULL) e.g. due to intention
of running the specified prog in sw, then you cannot use that bpf_prog
which was loaded as offloaded one instead. Looks correct to me.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 98bacd0fa5cc..c397934f91dd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -335,6 +335,8 @@  extern const struct bpf_verifier_ops xdp_analyzer_ops;
 
 struct bpf_prog *bpf_prog_get(u32 ufd);
 struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type);
+struct bpf_prog *bpf_prog_get_type_dev(u32 ufd, enum bpf_prog_type type,
+				       struct net_device *netdev);
 struct bpf_prog * __must_check bpf_prog_add(struct bpf_prog *prog, int i);
 void bpf_prog_sub(struct bpf_prog *prog, int i);
 struct bpf_prog * __must_check bpf_prog_inc(struct bpf_prog *prog);
@@ -428,6 +430,14 @@  static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
+
+static inline struct bpf_prog *bpf_prog_get_type_dev(u32 ufd,
+						     enum bpf_prog_type type,
+						     struct net_device *netdev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 static inline struct bpf_prog * __must_check bpf_prog_add(struct bpf_prog *prog,
 							  int i)
 {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 3217c20ea91b..68f9123acd39 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1057,7 +1057,22 @@  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 *attach_type)
+static bool bpf_prog_can_attach(struct bpf_prog *prog,
+				enum bpf_prog_type *attach_type,
+				struct net_device *netdev)
+{
+	struct bpf_dev_offload *offload = prog->aux->offload;
+
+	if (prog->type != *attach_type)
+		return false;
+	if (offload && offload->netdev != netdev)
+		return false;
+
+	return true;
+}
+
+static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type,
+				       struct net_device *netdev)
 {
 	struct fd f = fdget(ufd);
 	struct bpf_prog *prog;
@@ -1065,7 +1080,7 @@  static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type)
 	prog = ____bpf_prog_get(f);
 	if (IS_ERR(prog))
 		return prog;
-	if (attach_type && (prog->type != *attach_type || prog->aux->offload)) {
+	if (attach_type && !bpf_prog_can_attach(prog, attach_type, netdev)) {
 		prog = ERR_PTR(-EINVAL);
 		goto out;
 	}
@@ -1078,12 +1093,12 @@  static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *attach_type)
 
 struct bpf_prog *bpf_prog_get(u32 ufd)
 {
-	return __bpf_prog_get(ufd, NULL);
+	return __bpf_prog_get(ufd, NULL, NULL);
 }
 
 struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)
 {
-	struct bpf_prog *prog = __bpf_prog_get(ufd, &type);
+	struct bpf_prog *prog = __bpf_prog_get(ufd, &type, NULL);
 
 	if (!IS_ERR(prog))
 		trace_bpf_prog_get_type(prog);
@@ -1091,6 +1106,16 @@  struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_get_type);
 
+struct bpf_prog *bpf_prog_get_type_dev(u32 ufd, enum bpf_prog_type type,
+				       struct net_device *netdev)
+{
+	struct bpf_prog *prog = __bpf_prog_get(ufd, &type, netdev);
+
+	if (!IS_ERR(prog))
+		trace_bpf_prog_get_type(prog);
+	return prog;
+}
+
 /* last field in 'union bpf_attr' used by this command */
 #define	BPF_PROG_LOAD_LAST_FIELD prog_target_ifindex
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 10cde58d3275..30b5fe32c525 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7157,7 +7157,11 @@  int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		    __dev_xdp_attached(dev, bpf_op, NULL))
 			return -EBUSY;
 
-		prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
+		if (bpf_op == ops->ndo_bpf)
+			prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
+						     dev);
+		else
+			prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
 		if (IS_ERR(prog))
 			return PTR_ERR(prog);
 	}