On Fri, Aug 30, 2024 at 09:39:10AM +1000, NeilBrown wrote:
> On Thu, 29 Aug 2024, Mike Snitzer wrote:
>
> > +
> > +bool nfs_uuid_is_local(const uuid_t *uuid, struct net *net, struct auth_domain *dom)
> > +{
> > + bool is_local = false;
> > + nfs_uuid_t *nfs_uuid;
> > +
> > + rcu_read_lock();
> > + nfs_uuid = nfs_uuid_lookup(uuid);
> > + if (nfs_uuid) {
> > + nfs_uuid->net = maybe_get_net(net);
>
> I know I said it looked wrong to be getting a ref for the domain but not
> the net - and it did. But that doesn't mean the fix was to get a ref
> for the net and to hold it indefinitely.
>
> This ref is now held until the client happens to notice that localio
> doesn't work any more (because nfsd_serv_try_get() fails). So the
> shutdown of a net namespace will be delayed indefinitely if the NFS
> filesystem isn't being actively used.
>
> I would prefer that there were a way for the net namespace to reach back
> into the client and disconnect itself. Probably this would be a
> linked-list in struct nfsd_net which linked list_heads in struct
> nfs_client. This list would need to be protected by a spinlock -
> probably global in nfs_common so client could remove itself and server
> could remove all clients after clearing their net pointers.
>
> It is probably best if I explain all of what I am thinking as a patch.
>
> Stay tuned.
OK, a mechanism to have the net namespace disconnect itself sounds neat.
Or alternatively we could do what I was doing:
/* Not running in nfsd context, must safely get reference on nfsd_serv */
cl_nfssvc_net = maybe_get_net(cl_nfssvc_net);
if (!cl_nfssvc_net)
return -ENXIO;
nn = net_generic(cl_nfssvc_net, nfsd_net_id);
/* The server may already be shutting down, disallow new localio */
if (unlikely(!nfsd_serv_try_get(nn))) {
But only if maybe_get_net() will always fail safely...
I feel like we talked about the relative safety of maybe_get_net()
before (but I'm coming up short searching my email):
static inline struct net *maybe_get_net(struct net *net)
{
/* Used when we know struct net exists but we
* aren't guaranteed a previous reference count
* exists. If the reference count is zero this
* function fails and returns NULL.
*/
if (!refcount_inc_not_zero(&net->ns.count))
net = NULL;
return net;
}
So you have doubts the struct net will always still exist because I
didn't take a reference? (from fs/nfsd/localio.c):
static __be32 localio_proc_uuid_is_local(struct svc_rqst *rqstp)
{
struct localio_uuidarg *argp = rqstp->rq_argp;
(void) nfs_uuid_is_local(&argp->uuid, SVC_NET(rqstp),
rqstp->rq_client);
return rpc_success;
}
I think that's a fair concern (despite it working fine in practice
with destructive container testing, I cannot say there won't ever be a
use-after-free bug).
So all said: consider me staying tuned ;)
Thanks
