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

[Trond Myklebust <trondmy@hammerspace.com>]

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. 


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

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