[PATCH bpf-next 0/2] Align BPF TCP CCs implementing cong_control() with non-BPF CCs

[PATCH bpf-next 0/2] Align BPF TCP CCs implementing cong_control() with non-BPF CCs

[Jörn-Thorben Hinz]

Hello there,

hope, I’m in the right place with bpf-next. Previous changes to
bpf_tcp_ca.c seemed to have gone here, besides the MAINTAINERS file and
get_maintainer.pl hinting more towards the neighboring net-next.

This small series corrects some inconveniences for a BPF TCP CC that
implements and uses tcp_congestion_ops.cong_control(). Until now, such a
CC did not have all necessary write access to struct sock and
unnecessarily needed to implement cong_avoid().

Jörn-Thorben Hinz (2):
  bpf: Allow a TCP CC to write sk_pacing_rate and sk_pacing_status
  bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

 net/ipv4/bpf_tcp_ca.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.30.2

[PATCH bpf-next 1/2] bpf: Allow a TCP CC to write sk_pacing_rate and sk_pacing_status

[Jörn-Thorben Hinz]

A CC that implements tcp_congestion_ops.cong_control() should be able to
control sk_pacing_rate and set sk_pacing_status, since
tcp_update_pacing_rate() is never called in this case. A built-in CC or
one from a kernel module is already able to write to both struct sock
members. For a BPF program, write access has not been allowed, yet.

Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
---
 net/ipv4/bpf_tcp_ca.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index f79ab942f03b..1f5c53ede4e5 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -111,6 +111,12 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 	}
 
 	switch (off) {
+	case offsetof(struct sock, sk_pacing_rate):
+		end = offsetofend(struct sock, sk_pacing_rate);
+		break;
+	case offsetof(struct sock, sk_pacing_status):
+		end = offsetofend(struct sock, sk_pacing_status);
+		break;
 	case bpf_ctx_range(struct inet_connection_sock, icsk_ca_priv):
 		end = offsetofend(struct inet_connection_sock, icsk_ca_priv);
 		break;
-- 
2.30.2

[PATCH bpf-next 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Jörn-Thorben Hinz]

When a CC implements tcp_congestion_ops.cong_control(), the alternate
cong_avoid() is not in use in the TCP stack. Do not force a BPF CC to
implement cong_avoid() as a no-op by always requiring it.

An incomplete BPF CC implementing neither cong_avoid() nor
cong_control() will still get rejected by
tcp_register_congestion_control().

Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
---
 net/ipv4/bpf_tcp_ca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 1f5c53ede4e5..37290d0bf134 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -17,6 +17,7 @@ extern struct bpf_struct_ops bpf_tcp_congestion_ops;
 static u32 optional_ops[] = {
 	offsetof(struct tcp_congestion_ops, init),
 	offsetof(struct tcp_congestion_ops, release),
+	offsetof(struct tcp_congestion_ops, cong_avoid),
 	offsetof(struct tcp_congestion_ops, set_state),
 	offsetof(struct tcp_congestion_ops, cwnd_event),
 	offsetof(struct tcp_congestion_ops, in_ack_event),
-- 
2.30.2

Re: [PATCH bpf-next 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Martin KaFai Lau]

On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz wrote:
> When a CC implements tcp_congestion_ops.cong_control(), the alternate
> cong_avoid() is not in use in the TCP stack. Do not force a BPF CC to
> implement cong_avoid() as a no-op by always requiring it.
> 
> An incomplete BPF CC implementing neither cong_avoid() nor
> cong_control() will still get rejected by
> tcp_register_congestion_control().
> 
> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> ---
>  net/ipv4/bpf_tcp_ca.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 1f5c53ede4e5..37290d0bf134 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -17,6 +17,7 @@ extern struct bpf_struct_ops bpf_tcp_congestion_ops;
>  static u32 optional_ops[] = {
>  	offsetof(struct tcp_congestion_ops, init),
>  	offsetof(struct tcp_congestion_ops, release),
> +	offsetof(struct tcp_congestion_ops, cong_avoid),
At least one of the cong_avoid() or cong_control() is needed.
It is better to remove is_optional(moff) check and its optional_ops[]
here.  Only depends on the tcp_register_congestion_control() which
does a similar check at the beginning.

Patch 1 looks good.  tcp_bbr.c also needs the sk_pacing fields.

A selftest is needed.  Can you share your bpf tcp-cc and
use it as a selftest to exercise the change in this patch
set ?


>  	offsetof(struct tcp_congestion_ops, set_state),
>  	offsetof(struct tcp_congestion_ops, cwnd_event),
>  	offsetof(struct tcp_congestion_ops, in_ack_event),
> -- 
> 2.30.2
> 

Re: [PATCH bpf-next 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Jörn-Thorben Hinz]

Thanks for the feedback, Martin.

On Wed, 2022-06-08 at 11:33 -0700, Martin KaFai Lau wrote:
> On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz wrote:
> > When a CC implements tcp_congestion_ops.cong_control(), the
> > alternate
> > cong_avoid() is not in use in the TCP stack. Do not force a BPF CC
> > to
> > implement cong_avoid() as a no-op by always requiring it.
> > 
> > An incomplete BPF CC implementing neither cong_avoid() nor
> > cong_control() will still get rejected by
> > tcp_register_congestion_control().
> > 
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > ---
> >  net/ipv4/bpf_tcp_ca.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > index 1f5c53ede4e5..37290d0bf134 100644
> > --- a/net/ipv4/bpf_tcp_ca.c
> > +++ b/net/ipv4/bpf_tcp_ca.c
> > @@ -17,6 +17,7 @@ extern struct bpf_struct_ops
> > bpf_tcp_congestion_ops;
> >  static u32 optional_ops[] = {
> >         offsetof(struct tcp_congestion_ops, init),
> >         offsetof(struct tcp_congestion_ops, release),
> > +       offsetof(struct tcp_congestion_ops, cong_avoid),
> At least one of the cong_avoid() or cong_control() is needed.
> It is better to remove is_optional(moff) check and its optional_ops[]
> here.  Only depends on the tcp_register_congestion_control() which
> does a similar check at the beginning.
You mean completely remove this part of the validation from
bpf_tcp_ca.c and just rely on tcp_register_congestion_control()? True,
that would be even easier to maintain at this point, make
tcp_register_congestion_control() the one-and-only place that has to
know about required and optional functions.

Will rework the second patch.

> 
> Patch 1 looks good.  tcp_bbr.c also needs the sk_pacing fields.
> 
> A selftest is needed.  Can you share your bpf tcp-cc and
> use it as a selftest to exercise the change in this patch
> set ?
I cannot do that just now, unfortunately. It’s still earlier work in
progress. Also, it will have an additional, external dependency which
might make it unfit to be included here/as a selftest. I will keep it
in mind for later this year, though.

In the meantime, I could look into adding a more naive/trivial test,
that implements cong_control() without cong_avoid() and relies on
sk_pacing_* being writable, if you would prefer that? Would that be
fine as a follow-up patch (might take me a moment) or better be
included in this series?

> 
> 
> >         offsetof(struct tcp_congestion_ops, set_state),
> >         offsetof(struct tcp_congestion_ops, cwnd_event),
> >         offsetof(struct tcp_congestion_ops, in_ack_event),
> > -- 
> > 2.30.2
> > 

Re: [PATCH bpf-next 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Martin KaFai Lau]

On Thu, Jun 09, 2022 at 10:55:25AM +0200, Jörn-Thorben Hinz wrote:
> Thanks for the feedback, Martin.
> 
> On Wed, 2022-06-08 at 11:33 -0700, Martin KaFai Lau wrote:
> > On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz wrote:
> > > When a CC implements tcp_congestion_ops.cong_control(), the
> > > alternate
> > > cong_avoid() is not in use in the TCP stack. Do not force a BPF CC
> > > to
> > > implement cong_avoid() as a no-op by always requiring it.
> > > 
> > > An incomplete BPF CC implementing neither cong_avoid() nor
> > > cong_control() will still get rejected by
> > > tcp_register_congestion_control().
> > > 
> > > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > > ---
> > >  net/ipv4/bpf_tcp_ca.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > > index 1f5c53ede4e5..37290d0bf134 100644
> > > --- a/net/ipv4/bpf_tcp_ca.c
> > > +++ b/net/ipv4/bpf_tcp_ca.c
> > > @@ -17,6 +17,7 @@ extern struct bpf_struct_ops
> > > bpf_tcp_congestion_ops;
> > >  static u32 optional_ops[] = {
> > >         offsetof(struct tcp_congestion_ops, init),
> > >         offsetof(struct tcp_congestion_ops, release),
> > > +       offsetof(struct tcp_congestion_ops, cong_avoid),
> > At least one of the cong_avoid() or cong_control() is needed.
> > It is better to remove is_optional(moff) check and its optional_ops[]
> > here.  Only depends on the tcp_register_congestion_control() which
> > does a similar check at the beginning.
> You mean completely remove this part of the validation from
> bpf_tcp_ca.c and just rely on tcp_register_congestion_control()? True,
Yes.

> that would be even easier to maintain at this point, make
> tcp_register_congestion_control() the one-and-only place that has to
> know about required and optional functions.
> 
> Will rework the second patch.
> 
> > 
> > Patch 1 looks good.  tcp_bbr.c also needs the sk_pacing fields.
> > 
> > A selftest is needed.  Can you share your bpf tcp-cc and
> > use it as a selftest to exercise the change in this patch
> > set ?
> I cannot do that just now, unfortunately. It’s still earlier work in
> progress. Also, it will have an additional, external dependency which
> might make it unfit to be included here/as a selftest. I will keep it
> in mind for later this year, though.
What is the external dependency ?  Could you share some high level
of the CC you are developing ?
The reason for this question is to see if there is something
missing from the kernel side to write the tcp-cc in bpf that you
are developing.

> In the meantime, I could look into adding a more naive/trivial test,
> that implements cong_control() without cong_avoid() and relies on
> sk_pacing_* being writable, if you would prefer that? Would that be
> fine as a follow-up patch (might take me a moment) or better be
> included in this series?
Yeah, it will do and the test should be submitted together in
this series.

Re: [PATCH bpf-next 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Jörn-Thorben Hinz]

On Thu, 2022-06-09 at 11:55 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 09, 2022 at 10:55:25AM +0200, Jörn-Thorben Hinz wrote:
> > Thanks for the feedback, Martin.
> > 
> > On Wed, 2022-06-08 at 11:33 -0700, Martin KaFai Lau wrote:
> > > On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz
> > > wrote:
> > > > When a CC implements tcp_congestion_ops.cong_control(), the
> > > > alternate
> > > > cong_avoid() is not in use in the TCP stack. Do not force a BPF
> > > > CC
> > > > to
> > > > implement cong_avoid() as a no-op by always requiring it.
> > > > 
> > > > An incomplete BPF CC implementing neither cong_avoid() nor
> > > > cong_control() will still get rejected by
> > > > tcp_register_congestion_control().
> > > > 
> > > > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > > > ---
> > > >  net/ipv4/bpf_tcp_ca.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > > > index 1f5c53ede4e5..37290d0bf134 100644
> > > > --- a/net/ipv4/bpf_tcp_ca.c
> > > > +++ b/net/ipv4/bpf_tcp_ca.c
> > > > @@ -17,6 +17,7 @@ extern struct bpf_struct_ops
> > > > bpf_tcp_congestion_ops;
> > > >  static u32 optional_ops[] = {
> > > >         offsetof(struct tcp_congestion_ops, init),
> > > >         offsetof(struct tcp_congestion_ops, release),
> > > > +       offsetof(struct tcp_congestion_ops, cong_avoid),
> > > At least one of the cong_avoid() or cong_control() is needed.
> > > It is better to remove is_optional(moff) check and its
> > > optional_ops[]
> > > here.  Only depends on the tcp_register_congestion_control()
> > > which
> > > does a similar check at the beginning.
> > You mean completely remove this part of the validation from
> > bpf_tcp_ca.c and just rely on tcp_register_congestion_control()?
> > True,
> Yes.
> 
> > that would be even easier to maintain at this point, make
> > tcp_register_congestion_control() the one-and-only place that has
> > to
> > know about required and optional functions.
> > 
> > Will rework the second patch.
> > 
> > > 
> > > Patch 1 looks good.  tcp_bbr.c also needs the sk_pacing fields.
> > > 
> > > A selftest is needed.  Can you share your bpf tcp-cc and
> > > use it as a selftest to exercise the change in this patch
> > > set ?
> > I cannot do that just now, unfortunately. It’s still earlier work
> > in
> > progress. Also, it will have an additional, external dependency
> > which
> > might make it unfit to be included here/as a selftest. I will keep
> > it
> > in mind for later this year, though.
> What is the external dependency ?  Could you share some high level
> of the CC you are developing ?
> The reason for this question is to see if there is something
> missing from the kernel side to write the tcp-cc in bpf that you
> are developing.
The algorithm is PowerTCP[1], I’m currently implementing it with eBPF.
It requires telemetry from the network.

The mentioned dependency (not developed by us, not yet released) will
provide such telemetry. Its end-host part is implemented with eBPF
itself and does not require any additional kernel patches.

At the moment I’m not aware of any other bits missing from the kernel
side. Will propose another patch if that changes or at least report it.

[1] https://www.usenix.org/system/files/nsdi22-paper-addanki_3.pdf

> 
> > In the meantime, I could look into adding a more naive/trivial
> > test,
> > that implements cong_control() without cong_avoid() and relies on
> > sk_pacing_* being writable, if you would prefer that? Would that be
> > fine as a follow-up patch (might take me a moment) or better be
> > included in this series?
> Yeah, it will do and the test should be submitted together in
> this series.
Please see v3 of the series.

[PATCH bpf-next v2 0/2] Align BPF TCP CCs implementing cong_control() with non-BPF CCs

[Jörn-Thorben Hinz]

This small series corrects some inconveniences for a BPF TCP CC that
implements and uses tcp_congestion_ops.cong_control(). Until now, such a
CC did not have all necessary write access to struct sock and
unnecessarily needed to implement cong_avoid().

---
v2:
 - Drop redundant check for required functions and just rely on
   tcp_register_congestion_control() (Martin KaFai Lau)

Jörn-Thorben Hinz (2):
  bpf: Allow a TCP CC to write sk_pacing_rate and sk_pacing_status
  bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

 net/ipv4/bpf_tcp_ca.c | 38 ++++++--------------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

-- 
2.30.2

[PATCH bpf-next v2 1/2] bpf: Allow a TCP CC to write sk_pacing_rate and sk_pacing_status

[Jörn-Thorben Hinz]

A CC that implements tcp_congestion_ops.cong_control() should be able to
control sk_pacing_rate and set sk_pacing_status, since
tcp_update_pacing_rate() is never called in this case. A built-in CC or
one from a kernel module is already able to write to both struct sock
members. For a BPF program, write access has not been allowed, yet.

Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
---
 net/ipv4/bpf_tcp_ca.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index f79ab942f03b..1f5c53ede4e5 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -111,6 +111,12 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 	}
 
 	switch (off) {
+	case offsetof(struct sock, sk_pacing_rate):
+		end = offsetofend(struct sock, sk_pacing_rate);
+		break;
+	case offsetof(struct sock, sk_pacing_status):
+		end = offsetofend(struct sock, sk_pacing_status);
+		break;
 	case bpf_ctx_range(struct inet_connection_sock, icsk_ca_priv):
 		end = offsetofend(struct inet_connection_sock, icsk_ca_priv);
 		break;
-- 
2.30.2

[PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Jörn-Thorben Hinz]

Remove the check for required and optional functions in a struct
tcp_congestion_ops from bpf_tcp_ca.c. Rely on
tcp_register_congestion_control() to reject a BPF CC that does not
implement all required functions, as it will do for a non-BPF CC.

When a CC implements tcp_congestion_ops.cong_control(), the alternate
cong_avoid() is not in use in the TCP stack. Previously, a BPF CC was
still forced to implement cong_avoid() as a no-op since it was
non-optional in bpf_tcp_ca.c.

Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
---
 net/ipv4/bpf_tcp_ca.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 1f5c53ede4e5..39b64f317499 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -14,18 +14,6 @@
 /* "extern" is to avoid sparse warning.  It is only used in bpf_struct_ops.c. */
 extern struct bpf_struct_ops bpf_tcp_congestion_ops;
 
-static u32 optional_ops[] = {
-	offsetof(struct tcp_congestion_ops, init),
-	offsetof(struct tcp_congestion_ops, release),
-	offsetof(struct tcp_congestion_ops, set_state),
-	offsetof(struct tcp_congestion_ops, cwnd_event),
-	offsetof(struct tcp_congestion_ops, in_ack_event),
-	offsetof(struct tcp_congestion_ops, pkts_acked),
-	offsetof(struct tcp_congestion_ops, min_tso_segs),
-	offsetof(struct tcp_congestion_ops, sndbuf_expand),
-	offsetof(struct tcp_congestion_ops, cong_control),
-};
-
 static u32 unsupported_ops[] = {
 	offsetof(struct tcp_congestion_ops, get_info),
 };
@@ -51,18 +39,6 @@ static int bpf_tcp_ca_init(struct btf *btf)
 	return 0;
 }
 
-static bool is_optional(u32 member_offset)
-{
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(optional_ops); i++) {
-		if (member_offset == optional_ops[i])
-			return true;
-	}
-
-	return false;
-}
-
 static bool is_unsupported(u32 member_offset)
 {
 	unsigned int i;
@@ -268,14 +244,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
 		return 1;
 	}
 
-	if (!btf_type_resolve_func_ptr(btf_vmlinux, member->type, NULL))
-		return 0;
-
-	/* Ensure bpf_prog is provided for compulsory func ptr */
-	prog_fd = (int)(*(unsigned long *)(udata + moff));
-	if (!prog_fd && !is_optional(moff) && !is_unsupported(moff))
-		return -EINVAL;
-
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[kernel test robot]

Hi "Jörn-Thorben,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/J-rn-Thorben-Hinz/bpf-Allow-a-TCP-CC-to-write-sk_pacing_rate-and-sk_pacing_status/20220610-054718
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-a015 (https://download.01.org/0day-ci/archive/20220610/202206100820.VN7PQCkp-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 70d35fe1257e429266b83025997b400e9f79110e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ac2a3c95ce28ad79c2ef7e6c52c4fd25af9f3c6d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review J-rn-Thorben-Hinz/bpf-Allow-a-TCP-CC-to-write-sk_pacing_rate-and-sk_pacing_status/20220610-054718
        git checkout ac2a3c95ce28ad79c2ef7e6c52c4fd25af9f3c6d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ipv4/bpf_tcp_ca.c:225:6: warning: unused variable 'prog_fd' [-Wunused-variable]
           int prog_fd;
               ^
   1 warning generated.


vim +/prog_fd +225 net/ipv4/bpf_tcp_ca.c

0baf26b0fcd74b Martin KaFai Lau   2020-01-08  218  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  219  static int bpf_tcp_ca_init_member(const struct btf_type *t,
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  220  				  const struct btf_member *member,
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  221  				  void *kdata, const void *udata)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  222  {
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  223  	const struct tcp_congestion_ops *utcp_ca;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  224  	struct tcp_congestion_ops *tcp_ca;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08 @225  	int prog_fd;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  226  	u32 moff;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  227  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  228  	utcp_ca = (const struct tcp_congestion_ops *)udata;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  229  	tcp_ca = (struct tcp_congestion_ops *)kdata;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  230  
8293eb995f349a Alexei Starovoitov 2021-12-01  231  	moff = __btf_member_bit_offset(t, member) / 8;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  232  	switch (moff) {
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  233  	case offsetof(struct tcp_congestion_ops, flags):
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  234  		if (utcp_ca->flags & ~TCP_CONG_MASK)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  235  			return -EINVAL;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  236  		tcp_ca->flags = utcp_ca->flags;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  237  		return 1;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  238  	case offsetof(struct tcp_congestion_ops, name):
8e7ae2518f5265 Martin KaFai Lau   2020-03-13  239  		if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name,
8e7ae2518f5265 Martin KaFai Lau   2020-03-13  240  				     sizeof(tcp_ca->name)) <= 0)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  241  			return -EINVAL;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  242  		if (tcp_ca_find(utcp_ca->name))
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  243  			return -EEXIST;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  244  		return 1;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  245  	}
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  246  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  247  	return 0;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  248  }
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  249  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[kernel test robot]

Hi "Jörn-Thorben,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/J-rn-Thorben-Hinz/bpf-Allow-a-TCP-CC-to-write-sk_pacing_rate-and-sk_pacing_status/20220610-054718
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220610/202206102106.exquHIbr-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/ac2a3c95ce28ad79c2ef7e6c52c4fd25af9f3c6d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review J-rn-Thorben-Hinz/bpf-Allow-a-TCP-CC-to-write-sk_pacing_rate-and-sk_pacing_status/20220610-054718
        git checkout ac2a3c95ce28ad79c2ef7e6c52c4fd25af9f3c6d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/ipv4/bpf_tcp_ca.c: In function 'bpf_tcp_ca_init_member':
>> net/ipv4/bpf_tcp_ca.c:225:13: warning: unused variable 'prog_fd' [-Wunused-variable]
     225 |         int prog_fd;
         |             ^~~~~~~


vim +/prog_fd +225 net/ipv4/bpf_tcp_ca.c

0baf26b0fcd74b Martin KaFai Lau   2020-01-08  218  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  219  static int bpf_tcp_ca_init_member(const struct btf_type *t,
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  220  				  const struct btf_member *member,
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  221  				  void *kdata, const void *udata)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  222  {
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  223  	const struct tcp_congestion_ops *utcp_ca;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  224  	struct tcp_congestion_ops *tcp_ca;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08 @225  	int prog_fd;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  226  	u32 moff;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  227  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  228  	utcp_ca = (const struct tcp_congestion_ops *)udata;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  229  	tcp_ca = (struct tcp_congestion_ops *)kdata;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  230  
8293eb995f349a Alexei Starovoitov 2021-12-01  231  	moff = __btf_member_bit_offset(t, member) / 8;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  232  	switch (moff) {
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  233  	case offsetof(struct tcp_congestion_ops, flags):
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  234  		if (utcp_ca->flags & ~TCP_CONG_MASK)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  235  			return -EINVAL;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  236  		tcp_ca->flags = utcp_ca->flags;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  237  		return 1;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  238  	case offsetof(struct tcp_congestion_ops, name):
8e7ae2518f5265 Martin KaFai Lau   2020-03-13  239  		if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name,
8e7ae2518f5265 Martin KaFai Lau   2020-03-13  240  				     sizeof(tcp_ca->name)) <= 0)
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  241  			return -EINVAL;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  242  		if (tcp_ca_find(utcp_ca->name))
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  243  			return -EEXIST;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  244  		return 1;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  245  	}
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  246  
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  247  	return 0;
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  248  }
0baf26b0fcd74b Martin KaFai Lau   2020-01-08  249  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Martin KaFai Lau]

On Thu, Jun 09, 2022 at 10:47:02PM +0200, Jörn-Thorben Hinz wrote:
> Remove the check for required and optional functions in a struct
> tcp_congestion_ops from bpf_tcp_ca.c. Rely on
> tcp_register_congestion_control() to reject a BPF CC that does not
> implement all required functions, as it will do for a non-BPF CC.
> 
> When a CC implements tcp_congestion_ops.cong_control(), the alternate
> cong_avoid() is not in use in the TCP stack. Previously, a BPF CC was
> still forced to implement cong_avoid() as a no-op since it was
> non-optional in bpf_tcp_ca.c.
> 
> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> ---
>  net/ipv4/bpf_tcp_ca.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 1f5c53ede4e5..39b64f317499 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -14,18 +14,6 @@
>  /* "extern" is to avoid sparse warning.  It is only used in bpf_struct_ops.c. */
>  extern struct bpf_struct_ops bpf_tcp_congestion_ops;
>  
> -static u32 optional_ops[] = {
> -	offsetof(struct tcp_congestion_ops, init),
> -	offsetof(struct tcp_congestion_ops, release),
> -	offsetof(struct tcp_congestion_ops, set_state),
> -	offsetof(struct tcp_congestion_ops, cwnd_event),
> -	offsetof(struct tcp_congestion_ops, in_ack_event),
> -	offsetof(struct tcp_congestion_ops, pkts_acked),
> -	offsetof(struct tcp_congestion_ops, min_tso_segs),
> -	offsetof(struct tcp_congestion_ops, sndbuf_expand),
> -	offsetof(struct tcp_congestion_ops, cong_control),
> -};
> -
>  static u32 unsupported_ops[] = {
>  	offsetof(struct tcp_congestion_ops, get_info),
>  };
> @@ -51,18 +39,6 @@ static int bpf_tcp_ca_init(struct btf *btf)
>  	return 0;
>  }
>  
> -static bool is_optional(u32 member_offset)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(optional_ops); i++) {
> -		if (member_offset == optional_ops[i])
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
>  static bool is_unsupported(u32 member_offset)
>  {
>  	unsigned int i;
> @@ -268,14 +244,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
>  		return 1;
>  	}
>  
> -	if (!btf_type_resolve_func_ptr(btf_vmlinux, member->type, NULL))
> -		return 0;
> -
> -	/* Ensure bpf_prog is provided for compulsory func ptr */
> -	prog_fd = (int)(*(unsigned long *)(udata + moff));
> -	if (!prog_fd && !is_optional(moff) && !is_unsupported(moff))
!is_unsupported() is still needed.

and remove 'int prog_fd' as reported by the test bot.

Test is still needed.  You can copy the simpler "bpf_dctcp"
to another tcp_congestion_ops.  Write+read the sk_packing
and also use .cong_control instead of .cong_avoid.  I think rs->acked_sacked
is the 'delivered' and the 'ack' is not used.

Re: [PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

[Jörn-Thorben Hinz]

On Mon, 2022-06-13 at 10:17 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 09, 2022 at 10:47:02PM +0200, Jörn-Thorben Hinz wrote:
> > Remove the check for required and optional functions in a struct
> > tcp_congestion_ops from bpf_tcp_ca.c. Rely on
> > tcp_register_congestion_control() to reject a BPF CC that does not
> > implement all required functions, as it will do for a non-BPF CC.
> > 
> > When a CC implements tcp_congestion_ops.cong_control(), the
> > alternate
> > cong_avoid() is not in use in the TCP stack. Previously, a BPF CC
> > was
> > still forced to implement cong_avoid() as a no-op since it was
> > non-optional in bpf_tcp_ca.c.
> > 
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > ---
> >  net/ipv4/bpf_tcp_ca.c | 32 --------------------------------
> >  1 file changed, 32 deletions(-)
> > 
> > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > index 1f5c53ede4e5..39b64f317499 100644
> > --- a/net/ipv4/bpf_tcp_ca.c
> > +++ b/net/ipv4/bpf_tcp_ca.c
> > @@ -14,18 +14,6 @@
> >  /* "extern" is to avoid sparse warning.  It is only used in
> > bpf_struct_ops.c. */
> >  extern struct bpf_struct_ops bpf_tcp_congestion_ops;
> >  
> > -static u32 optional_ops[] = {
> > -       offsetof(struct tcp_congestion_ops, init),
> > -       offsetof(struct tcp_congestion_ops, release),
> > -       offsetof(struct tcp_congestion_ops, set_state),
> > -       offsetof(struct tcp_congestion_ops, cwnd_event),
> > -       offsetof(struct tcp_congestion_ops, in_ack_event),
> > -       offsetof(struct tcp_congestion_ops, pkts_acked),
> > -       offsetof(struct tcp_congestion_ops, min_tso_segs),
> > -       offsetof(struct tcp_congestion_ops, sndbuf_expand),
> > -       offsetof(struct tcp_congestion_ops, cong_control),
> > -};
> > -
> >  static u32 unsupported_ops[] = {
> >         offsetof(struct tcp_congestion_ops, get_info),
> >  };
> > @@ -51,18 +39,6 @@ static int bpf_tcp_ca_init(struct btf *btf)
> >         return 0;
> >  }
> >  
> > -static bool is_optional(u32 member_offset)
> > -{
> > -       unsigned int i;
> > -
> > -       for (i = 0; i < ARRAY_SIZE(optional_ops); i++) {
> > -               if (member_offset == optional_ops[i])
> > -                       return true;
> > -       }
> > -
> > -       return false;
> > -}
> > -
> >  static bool is_unsupported(u32 member_offset)
> >  {
> >         unsigned int i;
> > @@ -268,14 +244,6 @@ static int bpf_tcp_ca_init_member(const struct
> > btf_type *t,
> >                 return 1;
> >         }
> >  
> > -       if (!btf_type_resolve_func_ptr(btf_vmlinux, member->type,
> > NULL))
> > -               return 0;
> > -
> > -       /* Ensure bpf_prog is provided for compulsory func ptr */
> > -       prog_fd = (int)(*(unsigned long *)(udata + moff));
> > -       if (!prog_fd && !is_optional(moff) &&
> > !is_unsupported(moff))
> !is_unsupported() is still needed.
I don’t think it is necessary here.

There is also bpf_tcp_ca_check_member(), which also and only tests for
is_unsupported(). And check_member() will be called even earlier than
init_member() when loading a struct_ops. This deleted is_unsupported()
call should have been there only to test for missing but required &&
supported functions.

I added a test in v3 to make sure the check for the unsupported
get_info() (still) works.

Please correct me, if I misread/misfollowed the code paths!

> 
> and remove 'int prog_fd' as reported by the test bot.
Of course, sorry about that … Oversight on my side.

> 
> Test is still needed.  You can copy the simpler "bpf_dctcp"
> to another tcp_congestion_ops.  Write+read the sk_packing
> and also use .cong_control instead of .cong_avoid.  I think rs-
> >acked_sacked
> is the 'delivered' and the 'ack' is not used.
Added tests in v3 of the patch series. I’m open for further feedback.