Re: [PATCH 1/4] nfs: use-after-free in svc_process_common()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2018-12-24 at 11:59 +0300, Vasily Averin wrote:
> On 12/24/18 11:21 AM, Trond Myklebust wrote:
> > On Mon, 2018-12-24 at 09:05 +0300, Vasily Averin wrote:
> > > On 12/24/18 8:51 AM, Vasily Averin wrote:
> > > > On 12/24/18 2:56 AM, Trond Myklebust wrote:
> > > > > On Sat, 2018-12-22 at 20:46 +0300, Vasily Averin wrote:
> > > > > > On 12/21/18 4:00 AM, bfields@fieldses.org wrote:
> > > > > > > On Tue, Dec 18, 2018 at 02:55:15PM +0000, Trond Myklebust
> > > > > > > wrote:
> > > > > > > > No. We don't care about xpt_flags for the back channel
> > > > > > > > because
> > > > > > > > there is
> > > > > > > > no "server transport". The actual transport is stored
> > > > > > > > in
> > > > > > > > the
> > > > > > > > 'struct
> > > > > > > > rpc_rqst', and is the struct rpc_xprt corresponding to
> > > > > > > > the
> > > > > > > > client
> > > > > > > > socket or RDMA channel.
> > > > > > > > 
> > > > > > > > IOW: All we really need in svc_process_common() is to
> > > > > > > > be
> > > > > > > > able to
> > > > > > > > run
> > > > > > > > rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(), and that
> > > > > > > > can
> > > > > > > > be
> > > > > > > > passed
> > > > > > > > either as a pointer to the struct svc_xprt_ops itself.
> > > > > > > 
> > > > > > > For what it's worth, I'd rather get rid of that op--it's
> > > > > > > an
> > > > > > > awfully
> > > > > > > roundabout way just to do "svc_putnl(resv, 0);" in the
> > > > > > > tcp
> > > > > > > case.
> > > > > > 
> > > > > > Do you mean that svc_create_xprt(serv, "tcp-bc", ...) was
> > > > > > used
> > > > > > ONLY
> > > > > > to call 
> > > > > > svc_tcp_prep_reply_hdr() in svc_process_common() ?
> > > > > > And according call for rdma-bc does nothing useful at all? 
> > > > > > 
> > > > > > I've just tried to remove svc_create_xprt() from
> > > > > > xs_tcp_bc_up()
> > > > > > and
> > > > > > just 
> > > > > > provide pointer to svc_tcp_prep_reply_hdr()
> > > > > > in  svc_process_common() 
> > > > > > via per-netns sunrpc_net -- and seems it was enough, my
> > > > > > testcase
> > > > > > worked correctly.
> > > > > 
> > > > > I don't see how that function is related to net namespaces.
> > > > > As
> > > > > far as I
> > > > > can tell, it only signals whether or not the type of
> > > > > transport
> > > > > uses the
> > > > > TCP record marking scheme.
> > > > 
> > > > We need to know which kind of transport is used in specified
> > > > net
> > > > namespace,
> > > > for example init_ns can use RDMA transport and netns "second"
> > > > can
> > > > use 
> > > > TCP transport at the same time.
> > > > If you do not like an idea to use function pointer as a mark --
> > > > ok
> > > > I can save only some boolean flag on sunrpc_net, check it in
> > > > svc_process_common() 
> > > > and if it is set -- call svc_tcp_prep_reply_hdr() directly.
> > 
> > I'm not against the idea of using a function pointer, but I'm
> > saying
> > that the transport is not unique per-netns. Instead, the transport
> > is
> > usually per NFS mount, but you can always retrieve a pointer to it
> > directly in bc_svc_process() from req->rq_xprt. 
> 
> You're right, I was wrong because I was focused on creation of fake
> transport svc_xprt.
> Yes, we cannot use per-netns pointer here.
> 
> > > moreover, I can do not change sunrpc_net at all,
> > > I can check in bc_svc_common() which transport uses incoming
> > > svc_req
> > > and provide such flag as new parameter to svc_process_common().
> > 
> > The function or flag used by bc_svc_common() could be added to req-
> > > rq_xprt->ops as another 'bc_' field and then passed to
> > svc_process_common() as the parameter.
> 
> Can I just check rqstp->rq_prot ? It is inherited from incoming
> svc_req,
> and it seems it enough to check its propo, it isn't? 
> 
> svc_process_common()
> ...
>         /* Setup reply header */
>         if (rqstp->rq_prot == IPPROTO_TCP)
>                 svc_tcp_prep_reply_hdr(rqstp);

Yes. In these days with retpoline slowing down all indirect function
calls, then the above is probably the better solution.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com