[bpf-next,v4,06/15] bpf: Support consuming XDP HW metadata from fext programs

Commit Message

Stanislav Fomichev Dec. 13, 2022, 2:35 a.m. UTC

From: Toke Høiland-Jørgensen <[e-mail]>

Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
programs that consume HW metadata, implement support for propagating the
offload information. The extension program doesn't need to set a flag or
ifindex, it these will just be propagated from the target by the verifier.
We need to create a separate offload object for the extension program,
though, since it can be reattached to a different program later (which
means we can't just inhering the offload information from the target).

An additional check is added on attach that the new target is compatible
with the offload information in the extension prog.

Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
Signed-off-by: Stanislav Fomichev <[e-mail]>
---
 include/linux/bpf.h   |  7 ++++++
 kernel/bpf/offload.c  | 52 ++++++++++++++++++++++++++++---------------
 kernel/bpf/syscall.c  |  8 +++++++
 kernel/bpf/verifier.c | 19 +++++++++++-----
 4 files changed, 62 insertions(+), 24 deletions(-)

Comments

Martin KaFai Lau Dec. 14, 2022, 1:45 a.m. UTC | #1

On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> From: Toke Høiland-Jørgensen <[e-mail]>
> 
> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
> programs that consume HW metadata, implement support for propagating the
> offload information. The extension program doesn't need to set a flag or
> ifindex, it these will just be propagated from the target by the verifier.

s/it/because/ ... these will just be propagated....

> We need to create a separate offload object for the extension program,
> though, since it can be reattached to a different program later (which
> means we can't just inhering the offload information from the target).

hmm.... inheriting?

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 11c558be4992..8686475f0dbe 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>   			goto out_put_prog;
>   		}
>   
> +		if (bpf_prog_is_dev_bound(prog->aux) &&


> +		    (bpf_prog_is_offloaded(tgt_prog->aux) ||
> +		     !bpf_prog_is_dev_bound(tgt_prog->aux) ||
> +		     !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {

hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock. 
offload could be NULL, no?

It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.

> +			err = -EINVAL;
> +			goto out_put_prog;
> +		}
> +
>   		key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id);
>   	}
>   
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e61fe0472b9b..5c6d6d61e57a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16524,11 +16524,6 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>   	if (tgt_prog) {
>   		struct bpf_prog_aux *aux = tgt_prog->aux;
>   
> -		if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> -			bpf_log(log, "Replacing device-bound programs not supported\n");
> -			return -EINVAL;
> -		}
> -
>   		for (i = 0; i < aux->func_info_cnt; i++)
>   			if (aux->func_info[i].type_id == btf_id) {
>   				subprog = i;
> @@ -16789,10 +16784,22 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>   	if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
>   		/* to make freplace equivalent to their targets, they need to
>   		 * inherit env->ops and expected_attach_type for the rest of the
> -		 * verification
> +		 * verification; we also need to propagate the prog offload data
> +		 * for resolving kfuncs.
>   		 */
>   		env->ops = bpf_verifier_ops[tgt_prog->type];
>   		prog->expected_attach_type = tgt_prog->expected_attach_type;
> +
> +		if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> +			if (bpf_prog_is_offloaded(tgt_prog->aux))
> +				return -EINVAL;
> +
> +			prog->aux->dev_bound = true;
> +			ret = __bpf_prog_dev_bound_init(prog,
> +							tgt_prog->aux->offload->netdev);

Same here for tgt_prog->aux->offload.  bpf_prog_dev_bound_init() will need to 
take an extra dst_prog arg, like bpf_prog_dev_bound_init(prog, attr, dst_prog). 
It should be called earlier in syscall.c.

> +			if (ret)
> +				return ret;
> +		}
>   	}
>   
>   	/* store info about the attachment target that will be used later */

Toke Høiland-Jørgensen Dec. 14, 2022, 10:41 a.m. UTC | #2

Martin KaFai Lau <[e-mail]> writes:

> On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
>> From: Toke Høiland-Jørgensen <[e-mail]>
>> 
>> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
>> programs that consume HW metadata, implement support for propagating the
>> offload information. The extension program doesn't need to set a flag or
>> ifindex, it these will just be propagated from the target by the verifier.
>
> s/it/because/ ... these will just be propagated....

Yeah, or just drop 'it' :)

>> We need to create a separate offload object for the extension program,
>> though, since it can be reattached to a different program later (which
>> means we can't just inhering the offload information from the target).
>
> hmm.... inheriting?

Think I meant to write "we can't just inherit"

>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 11c558be4992..8686475f0dbe 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>>   			goto out_put_prog;
>>   		}
>>   
>> +		if (bpf_prog_is_dev_bound(prog->aux) &&
>
>
>> +		    (bpf_prog_is_offloaded(tgt_prog->aux) ||
>> +		     !bpf_prog_is_dev_bound(tgt_prog->aux) ||
>> +		     !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
>
> hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock. 
> offload could be NULL, no?
>
> It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.

Hmm, right, I was kinda expecting that this would not go away while
tgt_prog was alive, but I see now that's not the case due to the
unregister hook. So yeah, needs locking (same below) :)

-Toke

Stanislav Fomichev Dec. 14, 2022, 6:43 p.m. UTC | #3

On Wed, Dec 14, 2022 at 2:41 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Martin KaFai Lau <[e-mail]> writes:
>
> > On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> >> From: Toke Høiland-Jørgensen <[e-mail]>
> >>
> >> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
> >> programs that consume HW metadata, implement support for propagating the
> >> offload information. The extension program doesn't need to set a flag or
> >> ifindex, it these will just be propagated from the target by the verifier.
> >
> > s/it/because/ ... these will just be propagated....
>
> Yeah, or just drop 'it' :)
>
> >> We need to create a separate offload object for the extension program,
> >> though, since it can be reattached to a different program later (which
> >> means we can't just inhering the offload information from the target).
> >
> > hmm.... inheriting?
>
> Think I meant to write "we can't just inherit"
>
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index 11c558be4992..8686475f0dbe 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> >>                      goto out_put_prog;
> >>              }
> >>
> >> +            if (bpf_prog_is_dev_bound(prog->aux) &&
> >
> >
> >> +                (bpf_prog_is_offloaded(tgt_prog->aux) ||
> >> +                 !bpf_prog_is_dev_bound(tgt_prog->aux) ||
> >> +                 !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
> >
> > hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock.
> > offload could be NULL, no?
> >
> > It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.
>
> Hmm, right, I was kinda expecting that this would not go away while
> tgt_prog was alive, but I see now that's not the case due to the
> unregister hook. So yeah, needs locking (same below) :)

Agreed, thanks! These seem easy enough to address on my side, so I'll
take care of them (and will keep your attribution).

> -Toke
>

Toke Høiland-Jørgensen Dec. 14, 2022, 10:19 p.m. UTC | #4

Stanislav Fomichev <[e-mail]> writes:

> On Wed, Dec 14, 2022 at 2:41 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>>
>> Martin KaFai Lau <[e-mail]> writes:
>>
>> > On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
>> >> From: Toke Høiland-Jørgensen <[e-mail]>
>> >>
>> >> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
>> >> programs that consume HW metadata, implement support for propagating the
>> >> offload information. The extension program doesn't need to set a flag or
>> >> ifindex, it these will just be propagated from the target by the verifier.
>> >
>> > s/it/because/ ... these will just be propagated....
>>
>> Yeah, or just drop 'it' :)
>>
>> >> We need to create a separate offload object for the extension program,
>> >> though, since it can be reattached to a different program later (which
>> >> means we can't just inhering the offload information from the target).
>> >
>> > hmm.... inheriting?
>>
>> Think I meant to write "we can't just inherit"
>>
>> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> >> index 11c558be4992..8686475f0dbe 100644
>> >> --- a/kernel/bpf/syscall.c
>> >> +++ b/kernel/bpf/syscall.c
>> >> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>> >>                      goto out_put_prog;
>> >>              }
>> >>
>> >> +            if (bpf_prog_is_dev_bound(prog->aux) &&
>> >
>> >
>> >> +                (bpf_prog_is_offloaded(tgt_prog->aux) ||
>> >> +                 !bpf_prog_is_dev_bound(tgt_prog->aux) ||
>> >> +                 !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
>> >
>> > hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock.
>> > offload could be NULL, no?
>> >
>> > It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.
>>
>> Hmm, right, I was kinda expecting that this would not go away while
>> tgt_prog was alive, but I see now that's not the case due to the
>> unregister hook. So yeah, needs locking (same below) :)
>
> Agreed, thanks! These seem easy enough to address on my side, so I'll
> take care of them (and will keep your attribution).

Awesome, thanks!

-Toke

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de6279725f41..ed288d18bf8d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2482,6 +2482,7 @@  void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id);
 void unpriv_ebpf_notify(int new_state);
 
 #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL)
+int __bpf_prog_dev_bound_init(struct bpf_prog *prog, struct net_device *netdev);
 int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr);
 void bpf_dev_bound_netdev_unregister(struct net_device *dev);
 
@@ -2516,6 +2517,12 @@  void sock_map_unhash(struct sock *sk);
 void sock_map_destroy(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
 #else
+static inline int __bpf_prog_dev_bound_init(struct bpf_prog *prog,
+					    struct net_device *netdev)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int bpf_prog_dev_bound_init(struct bpf_prog *prog,
 					union bpf_attr *attr)
 {
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 3b6c9023f24d..0646dea1b0e1 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -188,17 +188,13 @@  static void __bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
 	kfree(ondev);
 }
 
-int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
+int __bpf_prog_dev_bound_init(struct bpf_prog *prog, struct net_device *netdev)
 {
 	struct bpf_offload_netdev *ondev;
 	struct bpf_prog_offload *offload;
 	int err;
 
-	if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
-	    attr->prog_type != BPF_PROG_TYPE_XDP)
-		return -EINVAL;
-
-	if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
+	if (!netdev)
 		return -EINVAL;
 
 	offload = kzalloc(sizeof(*offload), GFP_USER);
@@ -206,14 +202,7 @@  int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
 		return -ENOMEM;
 
 	offload->prog = prog;
-
-	offload->netdev = dev_get_by_index(current->nsproxy->net_ns,
-					   attr->prog_ifindex);
-	err = bpf_dev_offload_check(offload->netdev);
-	if (err)
-		goto err_maybe_put;
-
-	prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
+	offload->netdev = netdev;
 
 	down_write(&bpf_devs_lock);
 	ondev = bpf_offload_find_netdev(offload->netdev);
@@ -236,19 +225,46 @@  int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
 	offload->offdev = ondev->offdev;
 	prog->aux->offload = offload;
 	list_add_tail(&offload->offloads, &ondev->progs);
-	dev_put(offload->netdev);
 	up_write(&bpf_devs_lock);
 
 	return 0;
 err_unlock:
 	up_write(&bpf_devs_lock);
-err_maybe_put:
-	if (offload->netdev)
-		dev_put(offload->netdev);
 	kfree(offload);
 	return err;
 }
 
+int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
+{
+	struct net_device *netdev;
+	int err;
+
+	if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
+	    attr->prog_type != BPF_PROG_TYPE_XDP)
+		return -EINVAL;
+
+	if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
+		return -EINVAL;
+
+	netdev = dev_get_by_index(current->nsproxy->net_ns, attr->prog_ifindex);
+	if (!netdev)
+		return -EINVAL;
+
+	err = bpf_dev_offload_check(netdev);
+	if (err)
+		goto out;
+
+	prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
+
+	err = __bpf_prog_dev_bound_init(prog, netdev);
+	if (err)
+		goto out;
+
+out:
+	dev_put(netdev);
+	return err;
+}
+
 int bpf_prog_offload_verifier_prep(struct bpf_prog *prog)
 {
 	struct bpf_prog_offload *offload;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 11c558be4992..8686475f0dbe 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3021,6 +3021,14 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 			goto out_put_prog;
 		}
 
+		if (bpf_prog_is_dev_bound(prog->aux) &&
+		    (bpf_prog_is_offloaded(tgt_prog->aux) ||
+		     !bpf_prog_is_dev_bound(tgt_prog->aux) ||
+		     !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
+
 		key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id);
 	}
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e61fe0472b9b..5c6d6d61e57a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16524,11 +16524,6 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 	if (tgt_prog) {
 		struct bpf_prog_aux *aux = tgt_prog->aux;
 
-		if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
-			bpf_log(log, "Replacing device-bound programs not supported\n");
-			return -EINVAL;
-		}
-
 		for (i = 0; i < aux->func_info_cnt; i++)
 			if (aux->func_info[i].type_id == btf_id) {
 				subprog = i;
@@ -16789,10 +16784,22 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 	if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
 		/* to make freplace equivalent to their targets, they need to
 		 * inherit env->ops and expected_attach_type for the rest of the
-		 * verification
+		 * verification; we also need to propagate the prog offload data
+		 * for resolving kfuncs.
 		 */
 		env->ops = bpf_verifier_ops[tgt_prog->type];
 		prog->expected_attach_type = tgt_prog->expected_attach_type;
+
+		if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
+			if (bpf_prog_is_offloaded(tgt_prog->aux))
+				return -EINVAL;
+
+			prog->aux->dev_bound = true;
+			ret = __bpf_prog_dev_bound_init(prog,
+							tgt_prog->aux->offload->netdev);
+			if (ret)
+				return ret;
+		}
 	}
 
 	/* store info about the attachment target that will be used later */