Re: kern/41154: possible races in NFS server code ?

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Sun, Apr 05, 2009 at 09:05:00PM +0000, Manuel Bouyer wrote:
> >Description:
>       While working on the NFS server code I noticed that several
>       places use members of struct nfssvc_sock in a way that is not
>       obviously thread-safe.
>       There appears to be no locking to prevent a nfssvc_sock that has been
>       checked as SLP_VALID to become invalid while in use.
>       Some places recheck SLP_VALID where appropriate but some don't.
>       Especially I suspect there's a possible use after free of
>       ns_nam in nfssvc_nfsd(), as nothing seems to prevent nfsrv_zapsock()
>       from freeing it while nd_nam points to it.

For this specific issue, I moved the mfree of nd_nam to nfsrv_slpderef().
See attached patch

>       More generally the tail
>       of nfssvc_nfsd() doens't look safe.
> 
>       If concurent execution of nfssvc_addsock() and nfsrv_zapsock()
>       possible ? If so nfssvc_addsock() would need to take the ns_lock,
>       and revisit the way SLP_VALID is handled in these 2 functions.

After more though about this, it doens't seem to be possible.

> 
>       Last I suspect a race between sys_nfssvc() and nfsrv_zapsock()
>       could cause a corruption of the UID hash lists. More generally
>       what prevents concurent updates of the ns_uidlruhead ?

This question remains.

I found another possible race involving nfsrv_slpderef() which could lead
to a use after free of the slp. This involves one thread
calling nfsrv_slpderef(), another one grabbing slp from the
nfssvc_sockpending list and zapping it.
My fix for this is to not release the nfsd_lock between decrementing the
refcount and testing SLP_VALID.

there is also a place still using splsoftclock(), I gues it has been
forgotten. Without tacking nfsd_lock here we could dereference a NULL
pointer if we're being unlucky.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: nfs_syscalls.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_syscalls.c,v
retrieving revision 1.140
diff -u -r1.140 nfs_syscalls.c
--- nfs_syscalls.c      9 Oct 2008 14:38:21 -0000       1.140
+++ nfs_syscalls.c      8 Apr 2009 18:46:50 -0000
@@ -511,7 +511,6 @@
        u_quad_t cur_usec;
        int error = 0, cacherep, siz, sotype, writes_todo;
        struct proc *p = l->l_proc;
-       int s;
        bool doreinit;
 
 #ifndef nolint
@@ -761,14 +761,14 @@
                        getmicrotime(&tv);
                        cur_usec = (u_quad_t)tv.tv_sec * 1000000 +
                            (u_quad_t)tv.tv_usec;
-                       s = splsoftclock();
+                       mutex_enter(&nfsd_lock);
                        if (LIST_FIRST(&slp->ns_tq) &&
                            LIST_FIRST(&slp->ns_tq)->nd_time <= cur_usec) {
                                cacherep = RC_DOIT;
                                writes_todo = 1;
                        } else
                                writes_todo = 0;
-                       splx(s);
+                       mutex_exit(&nfsd_lock);
                } while (writes_todo);
                if (nfsrv_dorec(slp, nfsd, &nd, &dummy)) {
                        nfsd->nfsd_slp = NULL;
@@ -828,8 +828,6 @@
        soshutdown(so, SHUT_RDWR);
        sounlock(so);
 
-       if (slp->ns_nam)
-               m_free(slp->ns_nam);
        m_freem(slp->ns_raw);
        m = slp->ns_rec;
        while (m != NULL) {
@@ -839,8 +837,9 @@
                m_freem(m);
                m = n;
        }
+       /* XXX what about freeing ns_frag ? */
        for (nuidp = TAILQ_FIRST(&slp->ns_uidlruhead); nuidp != 0;
            nuidp = nnuidp) {
                nnuidp = TAILQ_NEXT(nuidp, nu_lru);
                LIST_REMOVE(nuidp, nu_hash);
                TAILQ_REMOVE(&slp->ns_uidlruhead, nuidp, nu_lru);
@@ -871,11 +871,9 @@
        mutex_enter(&nfsd_lock);
        KASSERT(slp->ns_sref > 0);
        ref = --slp->ns_sref;
-       mutex_exit(&nfsd_lock);
        if (ref == 0 && (slp->ns_flags & SLP_VALID) == 0) {
                file_t *fp;
 
-               mutex_enter(&nfsd_lock);
                KASSERT((slp->ns_gflags & SLP_G_DOREC) == 0);
                TAILQ_REMOVE(&nfssvc_sockhead, slp, ns_chain);
                mutex_exit(&nfsd_lock);
@@ -889,9 +887,12 @@
                        closef(fp);
                        slp->ns_so = NULL;
                }
-
+               
+               if (slp->ns_nam)
+                       m_free(slp->ns_nam);
                nfsrv_sockfree(slp);
-       }
+       } else
+               mutex_exit(&nfsd_lock);
 }
 
 /*