Re: [PATCH v2 bpf-next 4/5] bpf: tcp: Stop bpf_setsockopt(TCP_CONGESTION) in init ops to recur itself

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/28/22 7:04 PM, Eric Dumazet wrote:
On Fri, Sep 23, 2022 at 3:48 PM Martin KaFai Lau <kafai@xxxxxx> wrote:

From: Martin KaFai Lau <martin.lau@xxxxxxxxxx>

When a bad bpf prog '.init' calls
bpf_setsockopt(TCP_CONGESTION, "itself"), it will trigger this loop:

.init => bpf_setsockopt(tcp_cc) => .init => bpf_setsockopt(tcp_cc) ...
... => .init => bpf_setsockopt(tcp_cc).

It was prevented by the prog->active counter before but the prog->active
detection cannot be used in struct_ops as explained in the earlier
patch of the set.

In this patch, the second bpf_setsockopt(tcp_cc) is not allowed
in order to break the loop.  This is done by using a bit of
an existing 1 byte hole in tcp_sock to check if there is
on-going bpf_setsockopt(TCP_CONGESTION) in this tcp_sock.

Note that this essentially limits only the first '.init' can
call bpf_setsockopt(TCP_CONGESTION) to pick a fallback cc (eg. peer
does not support ECN) and the second '.init' cannot fallback to
another cc.  This applies even the second
bpf_setsockopt(TCP_CONGESTION) will not cause a loop.

Signed-off-by: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
---
  include/linux/tcp.h |  6 ++++++
  net/core/filter.c   | 28 +++++++++++++++++++++++++++-
  2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a9fbe22732c3..3bdf687e2fb3 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -388,6 +388,12 @@ struct tcp_sock {
         u8      bpf_sock_ops_cb_flags;  /* Control calling BPF programs
                                          * values defined in uapi/linux/tcp.h
                                          */
+       u8      bpf_chg_cc_inprogress:1; /* In the middle of
+                                         * bpf_setsockopt(TCP_CONGESTION),
+                                         * it is to avoid the bpf_tcp_cc->init()
+                                         * to recur itself by calling
+                                         * bpf_setsockopt(TCP_CONGESTION, "itself").
+                                         */
  #define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) (TP->bpf_sock_ops_cb_flags & ARG)
  #else
  #define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) 0
diff --git a/net/core/filter.c b/net/core/filter.c
index 96f2f7a65e65..ac4c45c02da5 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5105,6 +5105,9 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname,
  static int sol_tcp_sockopt_congestion(struct sock *sk, char *optval,
                                       int *optlen, bool getopt)
  {
+       struct tcp_sock *tp;
+       int ret;
+
         if (*optlen < 2)
                 return -EINVAL;

@@ -5125,8 +5128,31 @@ static int sol_tcp_sockopt_congestion(struct sock *sk, char *optval,
         if (*optlen >= sizeof("cdg") - 1 && !strncmp("cdg", optval, *optlen))
                 return -ENOTSUPP;

-       return do_tcp_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
+       /* It stops this looping
+        *
+        * .init => bpf_setsockopt(tcp_cc) => .init =>
+        * bpf_setsockopt(tcp_cc)" => .init => ....
+        *
+        * The second bpf_setsockopt(tcp_cc) is not allowed
+        * in order to break the loop when both .init
+        * are the same bpf prog.
+        *
+        * This applies even the second bpf_setsockopt(tcp_cc)
+        * does not cause a loop.  This limits only the first
+        * '.init' can call bpf_setsockopt(TCP_CONGESTION) to
+        * pick a fallback cc (eg. peer does not support ECN)
+        * and the second '.init' cannot fallback to
+        * another.
+        */
+       tp = tcp_sk(sk);
+       if (tp->bpf_chg_cc_inprogress)
+               return -EBUSY;
+

Is the socket locked (and owned by current thread) at this point ?
If not, changing bpf_chg_cc_inprogress would be racy.

Yes, the socket is locked and owned. There is a sock_owned_by_me check earlier in _bpf_setsockopt().



+       tp->bpf_chg_cc_inprogress = 1;
+       ret = do_tcp_setsockopt(sk, SOL_TCP, TCP_CONGESTION,
                                 KERNEL_SOCKPTR(optval), *optlen);
+       tp->bpf_chg_cc_inprogress = 0;
+       return ret;
  }

  static int sol_tcp_sockopt(struct sock *sk, int optname,
--
2.30.2





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux