I think I can answer a few of these.  See answers inlined below…

Timmons

On 9/19/16, 8:53 AM, "Nadav Har'El" <n...@scylladb.com> wrote:

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

Well, KASSERT only actually fails in debug builds, so you’d never see a
problem with a release build
(since NDEBUG is defined in conf/release.mk).

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


Yes, the primary problem occurs in the closed state, but tcp_do_segment
asserts that the tcp state
is > TCP_LISTEN, so it seemed like a reasonable check for the net channel,
too.

Well, the only reason you end up in this state is that the net channel
should have been torn down
already, but hasn’t (though it could be in the process of doing so).
Additionally, since the network driver calls directly into this function,
you
could potentially have a burst of packets destined for this no longer
valid net channel.  So, it
made sense to just close the channel here instead of allowing more packets
to take this path
into the stack.

Handing the packet off to netisr_dispatch allows the stack to generate a
reset.

>
>
>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
><mailto:osv-dev%2bunsubscr...@googlegroups.com>.
>For more options, visit
>https://groups.google.com/d/optout <https://groups.google.com/d/optout>.
>
>
>
>
>
>





Spirent Communications e-mail confidentiality.
------------------------------------------------------------------------
This e-mail contains confidential and / or privileged information belonging to 
Spirent Communications plc, its affiliates and / or subsidiaries. If you are 
not the intended recipient, you are hereby notified that any disclosure, 
copying, distribution and / or the taking of any action based upon reliance on 
the contents of this transmission is strictly forbidden. If you have received 
this message in error please notify the sender by return e-mail and delete it 
from your system.

Spirent Communications plc
Northwood Park, Gatwick Road, Crawley, West Sussex, RH10 9XN, United Kingdom.
Tel No. +44 (0) 1293 767676
Fax No. +44 (0) 1293 767677

Registered in England Number 470893
Registered at Northwood Park, Gatwick Road, Crawley, West Sussex, RH10 9XN, 
United Kingdom.

Or if within the US,

Spirent Communications,
27349 Agoura Road, Calabasas, CA, 91301, USA.
Tel No. 1-818-676- 2300

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

Reply via email to