Message-ID: <1550853168.9958.1.camel@redhat.com>
Subject: Re: [PATCH 1/1] SUNRPC: fix handling of half-closed connection
From:   Dave Wysochanski <dwysocha@redhat.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc:     trond.myklebust@hammerspace.com,
        Anna Schumaker <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Date:   Fri, 22 Feb 2019 11:32:48 -0500
In-Reply-To: <CAN-5tyH1e_cakjmU5VkQraw=ht71y8vxvaZbe2W6zAdfEnWLPA@mail.gmail.com>
References: <20190220145650.21566-1-olga.kornievskaia@gmail.com>
         <1550837576.6456.3.camel@redhat.com>
         <CAN-5tyH1e_cakjmU5VkQraw=ht71y8vxvaZbe2W6zAdfEnWLPA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, 2019-02-22 at 09:44 -0500, Olga Kornievskaia wrote:
> Hi Dave,
> 
> A re-producer is a server that sends an ACK to the client's FIN/ACK
> request and does nothing afterwards (I can reproduce it 100% with a
> hacked up server. It was discovered with a "broken" server that
> doesn't fully closes a connection). It leave this client unable to
> connect to this server again indefinitely/forever/reboot required
> kind
> of state. Once it was considered that doing something like that to
> the
> client is a form of an attack (denial-of-server) and thus the kernel
> has a tcp_fin_timeout option after which the kernel will abort the
> connection. However this only applies to the sockets that have been
> closed by the client. This is NOT the case. NFS does not close the
> connection and it ignores kernel's notification of FIN_WAIT2 state.
> 
Interesting.  I had the same reproducer but eventually an RST would get
sent from the NFS client due to the TCP keepalives.  It sounds like
that is not the case anymore.  When I did my testing was over a year
and a half ago though.


> One can argue that this is a broken server and we shouldn't bother.
> But this patch is an attempt to argue that the client still should
> care and deal with this condition. However, if the community feels
> that a broken server is a broken server and this form of an attack is
> not interested, this patch can live will be an archive for later or
> never.
> 

This isn't IPoIB is it?

Actually, fwiw, looking back I had speculated on changes in this area. 
I'm adding you to the CC list of this bug which had some of my musings
on it:
https://bugzilla.redhat.com/show_bug.cgi?id=1466802#c43
That bug I ended up closing when we could no longer prove there was any
state where the NFS client could get stuck in FIN_WAIT2 after the
keepalive patch.

It can happen that the server only sends the ACK back to the clients
FIN,ACK so in general the testcase is valid.  But then the question is
how long should one wait for the final data and FIN from the server, or
are there ever instances where you shouldn't wait forever?  Is there a
way for us to know for sure there is no data left to receive so it's
safe to timeout?  No RPCs outstanding?

I don't claim to know many of the subtleties here as far as would the
server wait forever in LAST_ACK or do implementations eventually
timeout after some time?  Seems like if these last packets get lost
there is likely a bug somewhere (either firewall or TCP stack, etc).
https://tools.ietf.org/html/rfc793#page-22

It looks like at least some people are putting timeouts into their
stacks though I'm not sure that's a good idea or not.