On Tue, 2018-12-18 at 09:45 +0300, Vasily Averin wrote: > On 12/18/18 12:50 AM, J. Bruce Fields wrote: > > On Mon, Dec 17, 2018 at 07:23:54PM +0300, Vasily Averin wrote: > > > if node have NFSv41+ mounts inside several net namespaces > > > it can lead to use-after-free in svc_process_common() > > > > > > svc_process_common() > > > /* Setup reply header */ > > > rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp); <<< > > > HERE > > > > > > svc_process_common() can use already freed rqstp->rq_xprt, > > > it was assigned in bc_svc_process() where it was taken from serv- > > > >sv_bc_xprt. > > > > > > serv is global structure but sv_bc_xprt is assigned per- > > > netnamespace, > > > so if nfsv41+ shares are mounted in several containers together > > > bc_svc_process() can use wrong backchannel or even access freed > > > memory. > > > > > > To find correct svc_xprt of client-related backchannel > > > bc_svc_process() now calls new .bc_get_xprt callback > > > that executes svc_find_xprt() with proper xprt name. > > > > This stuff is confusing and I need to stare at it some more before > > I > > understand, but it's weird that we'd need to search for the right > > xprt. > > All NFS clients in all net namespaces used the same minorversion > shares common nfs_callback_data taken from global nfs_callback_info > array. > > Moreover these clients can use either rdma or nfs transport, > however only one of them can be used in one net namespace. > > Each net namespace must have own backchannel, > it cannot depend on other net namespaces, > because at least they can use different transports. > > So one svc_serv should be able to handle several (per-netns) > backchannels. > > Frankly speaking If you prefer I can easily convert global > nfs_callback_info to per net-namespace. > I've checked, it works too. However current solution looks better for > me. > > > We know which connection the backchannel request came over, and > > there > > should only be one backchannel using that connection, why can't we > > find > > it by just chasing pointers the right way? > > it is allocated by using follwing calltrace: > nfs_callback_up > nfs_callback_up_net > xprt->ops->bc_up(serv, net) -> xs_tcp_bc_up > svc_create_xprt(serv, "tcp-bc") > __svc_xpo_create > svc_bc_tcp_create > svc_bc_create_socket > > Here backchannel's svc_sock/svc/xprt is created. > It is per-netns and therefore it cannot be saved as pointer on global > svc_serv. > > It could be saved on some xprt related to forechannel, > I've expected it was done already -- but it was not done. > I've tried to find any way to do it -- but without success, > according structures seems are not accessible in svc_bc_tcp_create. > > Finally I've found that backchannel's xprt is added into serv- > >sv_permsocks > and svc_find_xprt can find it by name. > > It would be great if you can advise some more simple way. > > > OK, I do need to look at it more. > > It is quite important for containers so I think this patch (or any > alternative solution) > should be pushed in stable@. > The whole "let's set up rqstp->rq_xprt for the back channel" is nothing but a giant hack in order to work around the fact that svc_process_common() uses it to find the xpt_ops, and perform a couple of (meaningless for the back channel) tests of xpt_flags. What say we just pass in the xpt_ops as a parameter to svc_process_common(), and make those xpt_flags tests check for whether or not rqstp->rq_xprt is actually non-NULL? It probably also requires us to store a pointer to struct net in the struct svc_rqst so that nfs4_callback_compound() and svcauth_gss_accept() can find it, but that should be OK since the transport already has that referenced. Cheers, Trond -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
