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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz wrote:

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

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:

quoted

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 <redacted>
---
 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?

quoted

        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

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:

quoted

On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz wrote:

quoted

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 <redacted>
---
 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.

quoted

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.

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 <redacted>
---
 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

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 <redacted>
---
 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

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

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 <redacted>

All warnings (new ones prefixed by >>):

quoted

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

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 <redacted>

All warnings (new ones prefixed by >>):

   net/ipv4/bpf_tcp_ca.c: In function 'bpf_tcp_ca_init_member':

quoted

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

On Thu, Jun 09, 2022 at 10:47:02PM +0200, Jörn-Thorben Hinz wrote:

!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.

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:

quoted

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 <redacted>
---
 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-

quoted

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.

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:

quoted

Thanks for the feedback, Martin.

On Wed, 2022-06-08 at 11:33 -0700, Martin KaFai Lau wrote:

quoted

On Wed, Jun 08, 2022 at 07:48:43PM +0200, Jörn-Thorben Hinz
wrote:

quoted

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 <redacted>
---
 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.

quoted

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.

quoted

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

quoted

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.