Re: [PATCH RFC 08/15] SUNRPC: Add RPC_TASK_CORK flag

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2022-04-19 at 18:16 +0000, Chuck Lever III wrote:
> 
> 
> > On Apr 18, 2022, at 10:57 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Mon, 2022-04-18 at 12:52 -0400, Chuck Lever wrote:
> > > Introduce a mechanism to cause xprt_transmit() to break out of
> > > its
> > > sending loop at a specific rpc_rqst, rather than draining the
> > > whole
> > > transmit queue.
> > > 
> > > This enables the client to send just an RPC TLS probe and then
> > > wait
> > > for the response before proceeding with the rest of the queue.
> > > 
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > >  include/linux/sunrpc/sched.h  |    2 ++
> > >  include/trace/events/sunrpc.h |    1 +
> > >  net/sunrpc/xprt.c             |    2 ++
> > >  3 files changed, 5 insertions(+)
> > > 
> > > diff --git a/include/linux/sunrpc/sched.h
> > > b/include/linux/sunrpc/sched.h
> > > index 599133fb3c63..f8c09638fa69 100644
> > > --- a/include/linux/sunrpc/sched.h
> > > +++ b/include/linux/sunrpc/sched.h
> > > @@ -125,6 +125,7 @@ struct rpc_task_setup {
> > >  #define RPC_TASK_TLSCRED               0x00000008      /* Use
> > > AUTH_TLS credential */
> > >  #define RPC_TASK_NULLCREDS             0x00000010      /* Use
> > > AUTH_NULL credential */
> > >  #define RPC_CALL_MAJORSEEN             0x00000020      /* major
> > > timeout seen */
> > > +#define RPC_TASK_CORK                  0x00000040      /* cork
> > > the
> > > xmit queue */
> > >  #define RPC_TASK_DYNAMIC               0x00000080      /* task
> > > was
> > > kmalloc'ed */
> > >  #define        RPC_TASK_NO_ROUND_ROBIN         0x00000100     
> > > /*
> > > send requests on "main" xprt */
> > >  #define RPC_TASK_SOFT                  0x00000200      /* Use
> > > soft
> > > timeouts */
> > > @@ -137,6 +138,7 @@ struct rpc_task_setup {
> > >  
> > >  #define RPC_IS_ASYNC(t)                ((t)->tk_flags &
> > > RPC_TASK_ASYNC)
> > >  #define RPC_IS_SWAPPER(t)      ((t)->tk_flags &
> > > RPC_TASK_SWAPPER)
> > > +#define RPC_IS_CORK(t)         ((t)->tk_flags & RPC_TASK_CORK)
> > >  #define RPC_IS_SOFT(t)         ((t)->tk_flags &
> > > (RPC_TASK_SOFT|RPC_TASK_TIMEOUT))
> > >  #define RPC_IS_SOFTCONN(t)     ((t)->tk_flags &
> > > RPC_TASK_SOFTCONN)
> > >  #define RPC_WAS_SENT(t)                ((t)->tk_flags &
> > > RPC_TASK_SENT)
> > > diff --git a/include/trace/events/sunrpc.h
> > > b/include/trace/events/sunrpc.h
> > > index 811187c47ebb..e8d6adff1a50 100644
> > > --- a/include/trace/events/sunrpc.h
> > > +++ b/include/trace/events/sunrpc.h
> > > @@ -312,6 +312,7 @@ TRACE_EVENT(rpc_request,
> > >                 { RPC_TASK_TLSCRED, "TLSCRED"
> > > },                        \
> > >                 { RPC_TASK_NULLCREDS, "NULLCREDS"
> > > },                    \
> > >                 { RPC_CALL_MAJORSEEN, "MAJORSEEN"
> > > },                    \
> > > +               { RPC_TASK_CORK, "CORK"
> > > },                              \
> > >                 { RPC_TASK_DYNAMIC, "DYNAMIC"
> > > },                        \
> > >                 { RPC_TASK_NO_ROUND_ROBIN, "NO_ROUND_ROBIN"
> > > },          \
> > >                 { RPC_TASK_SOFT, "SOFT"
> > > },                              \
> > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > index 86d62cffba0d..4b303b945b51 100644
> > > --- a/net/sunrpc/xprt.c
> > > +++ b/net/sunrpc/xprt.c
> > > @@ -1622,6 +1622,8 @@ xprt_transmit(struct rpc_task *task)
> > >                 if (xprt_request_data_received(task) &&
> > >                     !test_bit(RPC_TASK_NEED_XMIT, &task-
> > > > tk_runstate))
> > >                         break;
> > > +               if (RPC_IS_CORK(task))
> > > +                       break;
> > >                 cond_resched_lock(&xprt->queue_lock);
> > >         }
> > >         spin_unlock(&xprt->queue_lock);
> > > 
> > > 
> > 
> > This is entirely the wrong place for this kind of control
> > mechanism.
> 
> I'm not sure I entirely understand your concern, so bear with
> me while I try to clarify.
> 
> 
> > TLS vs not-TLS needs to be decided up front when we initialise the
> > transport (i.e. at mount time or whenever the pNFS channels are set
> > up). Otherwise, we're vulnerable to downgrade attacks.
> 
> Downgrade attacks are prevented by using "xprtsec=tls" because
> in that case, transport creation fails if either the AUTH_TLS
> fails or the handshake fails.
> 
> The TCP connection has to be established first, though. Then the
> client can send the RPC_AUTH_TLS probe, which is the same as the
> NULL ping that it already sends. That mechanism is independent
> of the lower layer transport (TCP in this case).
> 
> Therefore, RPC traffic must be stoppered while the client:
> 
> 1. waits for the AUTH_TLS probe's reply, and
> 
> 2. waits for the handshake to complete
> 
> Because an RPC message is involved in this interaction, I didn't
> see a way to implement it completely within xprtsock's TCP
> connection logic. IMO, driving the handshake has to be done by
> the generic RPC client.
> 
> So, do you mean that I need to replace RPC_TASK_CORK with a
> special return code from xs_tcp_send_request() ?


I mean the right mechanism for controlling whether or not the transport
is ready to serve RPC requests is through the XPRT_CONNECTED flag. All
the existing generic RPC error handling, congestion handling, etc
depends on that flag being set correctly.

Until the TLS socket has completed its handshake protocol and is ready
to transmit data, it should not be declared connected. The distinction
between the two states 'TCP is unconnected' and 'TLS handshake is
incomplete' is a socket/transport setup detail as far as the RPC xprt
layer is concerned: just another set of intermediate states between
SYN_SENT and ESTABLISHED.

> > Once we've decided that TLS is the right thing to do, then we
> > shouldn't
> > declare to the RPC layer that the TLS-enabled transport is
> > connected
> > until the underlying transport connection is established, and the
> > TLS
> > handshake is done.
> 
> That logic is handled in patch 10/15.
> 
> Reconnecting and re-establishing a TLS session is handled in
> patches 11/15 and 12/15. Again, if the transport's policy setting
> is "must use TLS" then the client ensures that a TLS session is in
> use before allowing more RPC traffic on the new connection.
> 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
​
www.hammer.space