Re: [PATCH v14 14/25] nfs_common: add NFS LOCALIO auxiliary protocol enablement

[Mike Snitzer <snitzer@kernel.org>]

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