On Mon, Aug 15, 2022 at 02:29:02PM +0300, Vitaliy Makkoveev wrote:
> Nothing special, just get assign `inp' and `tp' from passed socket. I
> want to do this before (*pru_usrreq)() split to avoid huge code
> duplication.
>
> We have "tp might get 0 when using socket splicing" commentary. I
> checked where we set `inp_ppcb' to NULL and found, tcp_close() is the
> only place, and the following in_pcbdetach() sets `so_pcb' to NULL. So
> this commentary looks wrong, because we can't have TCP sockets without
> TCP pcb attached.
I added the comment and check after a panic in production.
----------------------------
revision 1.109
date: 2012/01/03 21:50:12; author: bluhm; state: Exp; lines: +6 -2;
When used with socket splicing, tcp_usrreq() might get called with
a socket that has an inp but tp is NULL. The call stack for that
is tcp_input() tcp_close() soisdisconnected() sorwakeup() somove()
tcp_usrreq(PRU_RCVD). To avoid a NULL dereference, just return in
that case.
ok henning@
----------------------------
tcp_input() calls tcp_close() after a reset packet.
> This diff works fine with sys/kern/sosplice test.
I am not sure that regress covers the tp == NULL case.
These reset conditions are hard to trigger.
Maybe it does not happen anymore as I have decoupled somove() with
a soplice thread 4 years later.
----------------------------
revision 1.155
date: 2016/08/25 14:13:19; author: bluhm; state: Exp; lines: +48 -9;
commitid: PnBATF8qpqSyZflF;
Spliced TCP sockets become faster when the output part is running
as its own task thread. This is inspired by userland copy where a
process also has to go through the scheduler. This gives the socket
buffer a chance to be filled up and tcp_output() is called less
often and with bigger chunks.
When two kernel tasks share all the workload, the current scheduler
implementation will hang userland processes on single cpu machines.
As a workaround put a yield() into the splicing thread after each
task execution. This reduces the number of calls of tcp_output()
even more.
OK tedu@ mpi@
----------------------------
/*
* TCP has a sendbuffer that can handle multiple packets
* at once. So queue the stream a bit to accumulate data.
* The sosplice thread will call somove() later and send
* the packets calling tcp_output() only once.
* In the UDP case, send out the packets immediately.
* Using a thread would make things slower.
*/
if (so->so_proto->pr_flags & PR_WANTRCVD)
task_add(sosplice_taskq, &so->so_splicetask);
My original plan was to keep sosplice thread optional and have the
code work without it. On the other hand we use it in TCP for 6
years now.
Should we move if (tp == NULL) to PRU_RCVD? That is the relevant
case. I think if ((so->so_state & (SS_ISCONNECTED|SS_ISCONNECTING))
!= 0) prevents us from calling tcp_output(tp) with NULL.
So the only place to crash by dereferencing tp is
if (so->so_options & SO_DEBUG) {
otp = tp;
ostate = tp->t_state;
}
When I added the check in rev 1.109, tp was always dereferenced.
if (inp) {
tp = intotcpcb(inp);
/* tp might get 0 when using socket splicing */
if (tp == NULL) {
splx(s);
return (0);
}
#ifdef KPROF
tcp_acounts[tp->t_state][req]++;
#endif
ostate = tp->t_state;
} else
ostate = 0;
We seem to be safe unless someone removes the sosplice thread and
runs with SO_DEBUG. Is this enough history to justify to remove a
check while just moving code around?
If we use if (tp != NULL && (so->so_options & SO_DEBUG)) everything
should be fine.
> + KASSERT(*rtp != NULL);
Remember deraadt@ yelling about too many KASSERT().
bluhm
