I'm having a hard time understanding this patch. Maybe Avi (CCed) remembers
some of the code involved (which he wrote) and might have better comments.
Some questions inline below:
--
Nadav Har'El
n...@scylladb.com
On Fri, Sep 9, 2016 at 9:08 PM, Timmons C. Player <
timmons.pla...@spirent.com> wrote:
> Check the TCP state of net_channel directed packets to ensure that
> tcp_do_segment can process them. If not, hand the packet off to
> netisr_dispatch and teardown the net channel. This prevents triggering
> assertion failures in tcp_do_segment.
>
> Also update the lock acquisition check at the beginning of tcp_do_segment
> to match the assertion check below it.
>
> Signed-off-by: Timmons C. Player <timmons.pla...@spirent.com>
> ---
> bsd/sys/netinet/tcp_input.cc | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/bsd/sys/netinet/tcp_input.cc b/bsd/sys/netinet/tcp_input.cc
> index 642cfeb..c780d46 100644
> --- a/bsd/sys/netinet/tcp_input.cc
> +++ b/bsd/sys/netinet/tcp_input.cc
> @@ -1048,7 +1048,9 @@ tcp_do_segment(struct mbuf *m, struct tcphdr *th,
> struct socket *so,
> * if we fail, drop the packet. FIXME: invert the lock order so
> we don't
> * have to drop packets.
> */
> - if (tp->get_state() != TCPS_ESTABLISHED && ti_locked ==
> TI_UNLOCKED) {
> + if ((tp->get_state() != TCPS_ESTABLISHED
> + || (thflags & (TH_SYN | TH_FIN | TH_RST) != 0))
> + && ti_locked == TI_UNLOCKED) {
>
This change makes sense to me, as even for an established connection, a FIN
or RST might require that global lock (I don't see how we can have a SYN
for an established connection).
However I don't understand, if this is what this patch does, how this the
connection closing ever work before? Wouldn't it fail when it noticed the
lock was not taken?
(note to self: the code modified here came from commit
128af65368a421e28bea16c303aee68b8b34de24)
> if (INP_INFO_TRY_WLOCK(&V_tcbinfo)) {
> ti_locked = TI_WLOCKED;
> } else {
> @@ -3188,6 +3190,15 @@ tcp_newreno_partial_ack(struct tcpcb *tp, struct
> tcphdr *th)
> static void
> tcp_net_channel_packet(tcpcb* tp, mbuf* m)
> {
> + if (tp->get_state() <= TCPS_LISTEN) {
> + // We can't hand this packet off to tcp_do_segment due to
> the
> + // current connection state. Drop the channel and
> handle the
> + // packet via the slow path.
> + tcp_teardown_net_channel(tp);
> + netisr_dispatch(NETISR_ETHER, m);
> + return;
> + }
>
I am guessing your problem wasn't with the listening state but rather the
"closed" state?
If the socket is closed, why do we call this netisr_dispatch() function,
what would it do?
Also why do we need to tcp_teardown_net_channel() here? Doesn't it get
called elsewhere already?
Maybe this code is correct, but I simply am not familiar enough with it to
tell... Avi?
> +
> log_packet_handling(m, NETISR_ETHER);
> caddr_t start = m->m_hdr.mh_data;
> auto h = start;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
