Date: Tue, 16 Jan 2024 19:05:16 GMT From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: c0303fa84eed - stable/14 - tcp_hpts: let tcp_hpts_init() set a random CPU only once Message-ID: <202401161905.40GJ5GXY010212@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/14 has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=c0303fa84eed989ec7ba9aafcb7028bb11c9d6a2 commit c0303fa84eed989ec7ba9aafcb7028bb11c9d6a2 Author: Gleb Smirnoff <glebius@FreeBSD.org> AuthorDate: 2023-12-07 22:41:43 +0000 Commit: Gleb Smirnoff <glebius@FreeBSD.org> CommitDate: 2024-01-16 18:47:49 +0000 tcp_hpts: let tcp_hpts_init() set a random CPU only once After d2ef52ef3dee the tcp_hpts_init() function can be called multiple times on a tcpcb if it is switched there and back between two TCP stacks. First, this makes existing assertion in tcp_hpts_init() incorrect. Second, it creates possibility to change a randomly set t_hpts_cpu to a different random value, while a tcpcb is already in the HPTS wheel, triggering other assertions later in tcp_hptsi(). The best approach here would be to work on the stacks to really clear a tcpcb out of HPTS wheel in tfb_tcp_fb_fini, draining the IHPTS_MOVING state. But that's pretty intrusive change, so let's just get back to the old logic (pre d2ef52ef3dee) where t_hpts_cpu was set to a random value only once in a CPU lifetime and a newly switched stack inherits t_hpts_cpu from the previous stack. Reviewed by: rrs, tuexen Differential Revision: https://reviews.freebsd.org/D42946 Reported-by: syzbot+fab29fe1ab089c52998d@syzkaller.appspotmail.com Reported-by: syzbot+ca5f2aa0fda15dcfe6d7@syzkaller.appspotmail.com Fixes: 2b3a77467dd3d74a7170f279fb25f9736b46ef8a (cherry picked from commit 3f46be6acadd5d660acde67d9d4c80137f424b70) --- sys/netinet/tcp_hpts.c | 16 ++++++++++++---- sys/netinet/tcp_subr.c | 3 +++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/sys/netinet/tcp_hpts.c b/sys/netinet/tcp_hpts.c index 92b2f69c9e58..e39c2b69ea7e 100644 --- a/sys/netinet/tcp_hpts.c +++ b/sys/netinet/tcp_hpts.c @@ -542,15 +542,23 @@ tcp_hpts_release(struct tcpcb *tp) } /* - * Initialize newborn tcpcb to get ready for use with HPTS. + * Initialize tcpcb to get ready for use with HPTS. We will know which CPU + * is preferred on the first incoming packet. Before that avoid crowding + * a single CPU with newborn connections and use a random one. + * This initialization is normally called on a newborn tcpcb, but potentially + * can be called once again if stack is switched. In that case we inherit CPU + * that the previous stack has set, be it random or not. In extreme cases, + * e.g. syzkaller fuzzing, a tcpcb can already be in HPTS in IHPTS_MOVING state + * and has never received a first packet. */ void tcp_hpts_init(struct tcpcb *tp) { - tp->t_hpts_cpu = hpts_random_cpu(); - tp->t_lro_cpu = HPTS_CPU_NONE; - MPASS(!(tp->t_flags2 & TF2_HPTS_CPU_SET)); + if (__predict_true(tp->t_hpts_cpu == HPTS_CPU_NONE)) { + tp->t_hpts_cpu = hpts_random_cpu(); + MPASS(!(tp->t_flags2 & TF2_HPTS_CPU_SET)); + } } /* diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c index 4e28f7917bdf..34150707072e 100644 --- a/sys/netinet/tcp_subr.c +++ b/sys/netinet/tcp_subr.c @@ -2276,6 +2276,9 @@ tcp_newtcpcb(struct inpcb *inp) /* All mbuf queue/ack compress flags should be off */ tcp_lro_features_off(tp); + tp->t_hpts_cpu = HPTS_CPU_NONE; + tp->t_lro_cpu = HPTS_CPU_NONE; + callout_init_rw(&tp->t_callout, &inp->inp_lock, CALLOUT_RETURNUNLOCKED); for (int i = 0; i < TT_N; i++) tp->t_timers[i] = SBT_MAX;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202401161905.40GJ5GXY010212>