vput(9) for NFS

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

vput(9) for NFS

Martin Pieuchot
Diff below implements most of the missing locking goo for NFS without
enabling it.  It does the following:

- Add a missing PDIRUNLOCK in nfs_lookup()

- Change vrele(9) into vput(9) where necessary.  nfs_nget() will in
  future return a locked NFSnode.  Just like ffs_vget() returns a
  locked vnode.  So every time it is called we need a corresponding
  vput(9).

- Make sure VN_KNOTE() is always called with a valid reference, before
  vrele(9) or vput(9).

- Substitute an XXX by an assert in nfs_removeit().

However as long as nfs_lock/unlock are noops, this diff should not
introduce any change in behavior.  But please do test this diff :)

Ok?

Index: nfs/nfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/nfs/nfs_vnops.c,v
retrieving revision 1.171
diff -u -p -r1.171 nfs_vnops.c
--- nfs/nfs_vnops.c 22 Feb 2017 11:42:46 -0000 1.171
+++ nfs/nfs_vnops.c 9 Apr 2018 14:00:09 -0000
@@ -769,6 +769,7 @@ nfs_lookup(void *v)
  flags = cnp->cn_flags;
 
  *vpp = NULLVP;
+ newvp = NULLVP;
  if ((flags & ISLASTCN) && (dvp->v_mount->mnt_flag & MNT_RDONLY) &&
     (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME))
  return (EROFS);
@@ -836,8 +837,10 @@ nfs_lookup(void *v)
  if (cnp->cn_nameiop != LOOKUP && (flags & ISLASTCN))
  cnp->cn_flags |= SAVENAME;
  if ((!lockparent || !(flags & ISLASTCN)) &&
-     newvp != dvp)
+     newvp != dvp) {
  VOP_UNLOCK(dvp, p);
+ cnp->cn_flags |= PDIRUNLOCK;
+ }
  return (0);
  }
  cache_purge(newvp);
@@ -1318,10 +1321,6 @@ nfs_mknodrpc(struct vnode *dvp, struct v
  if (!error) {
  nfsm_mtofh(dvp, newvp, info.nmi_v3, gotvp);
  if (!gotvp) {
- if (newvp) {
- vrele(newvp);
- newvp = NULL;
- }
  error = nfs_lookitup(dvp, cnp->cn_nameptr,
     cnp->cn_namelen, cnp->cn_cred, cnp->cn_proc, &np);
  if (!error)
@@ -1335,7 +1334,7 @@ nfs_mknodrpc(struct vnode *dvp, struct v
 nfsmout:
  if (error) {
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else {
  if (cnp->cn_flags & MAKEENTRY)
  nfs_cache_enter(dvp, newvp, cnp);
@@ -1345,7 +1344,7 @@ nfsmout:
  VTONFS(dvp)->n_flag |= NMODIFIED;
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1362,7 +1361,7 @@ nfs_mknod(void *v)
 
  error = nfs_mknodrpc(ap->a_dvp, &newvp, ap->a_cnp, ap->a_vap);
  if (!error)
- vrele(newvp);
+ vput(newvp);
 
  VN_KNOTE(ap->a_dvp, NOTE_WRITE);
 
@@ -1430,10 +1429,6 @@ again:
  if (!error) {
  nfsm_mtofh(dvp, newvp, info.nmi_v3, gotvp);
  if (!gotvp) {
- if (newvp) {
- vrele(newvp);
- newvp = NULL;
- }
  error = nfs_lookitup(dvp, cnp->cn_nameptr,
     cnp->cn_namelen, cnp->cn_cred, cnp->cn_proc, &np);
  if (!error)
@@ -1451,7 +1446,7 @@ nfsmout:
  goto again;
  }
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else if (info.nmi_v3 && (fmode & O_EXCL))
  error = nfs_setattrrpc(newvp, vap, cnp->cn_cred, cnp->cn_proc);
  if (!error) {
@@ -1464,7 +1459,7 @@ nfsmout:
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
  VN_KNOTE(ap->a_dvp, NOTE_WRITE);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1530,12 +1525,13 @@ nfs_remove(void *v)
  error = nfs_sillyrename(dvp, vp, cnp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  NFS_INVALIDATE_ATTRCACHE(np);
- vrele(dvp);
- vrele(vp);
-
  VN_KNOTE(vp, NOTE_DELETE);
  VN_KNOTE(dvp, NOTE_WRITE);
-
+ if (dvp == vp)
+ vrele(vp);
+ else
+ vput(vp);
+ vput(dvp);
  return (error);
 }
 
@@ -1545,9 +1541,9 @@ nfs_remove(void *v)
 int
 nfs_removeit(struct sillyrename *sp)
 {
+ KASSERT(VOP_ISLOCKED(sp->s_dvp));
  /*
  * Make sure that the directory vnode is still valid.
- * XXX we should lock sp->s_dvp here.
  *
  * NFS can potentially try to nuke a silly *after* the directory
  * has already been pushed out on a forced unmount. Since the silly
@@ -1628,7 +1624,7 @@ nfs_rename(void *v)
  if (tvp && tvp->v_usecount > 1 && !VTONFS(tvp)->n_sillyrename &&
     tvp->v_type != VDIR && !nfs_sillyrename(tdvp, tvp, tcnp)) {
  VN_KNOTE(tvp, NOTE_DELETE);
- vrele(tvp);
+ vput(tvp);
  tvp = NULL;
  }
 
@@ -1827,13 +1823,13 @@ nfs_symlink(void *v)
 
 nfsmout:
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  VTONFS(dvp)->n_flag |= NMODIFIED;
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
  VN_KNOTE(dvp, NOTE_WRITE);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1904,7 +1900,7 @@ nfsmout:
  }
  if (error) {
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else {
  VN_KNOTE(dvp, NOTE_WRITE|NOTE_LINK);
  if (cnp->cn_flags & MAKEENTRY)
@@ -1912,7 +1908,7 @@ nfsmout:
  *ap->a_vpp = newvp;
  }
  pool_put(&namei_pool, cnp->cn_pnbuf);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1936,7 +1932,7 @@ nfs_rmdir(void *v)
 
  if (dvp == vp) {
  vrele(dvp);
- vrele(dvp);
+ vput(dvp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  return (EINVAL);
  }
@@ -1964,8 +1960,8 @@ nfsmout:
  VN_KNOTE(vp, NOTE_DELETE);
 
  cache_purge(vp);
- vrele(vp);
- vrele(dvp);
+ vput(vp);
+ vput(dvp);
  /*
  * Kludge: Map ENOENT => 0 assuming that you have a reply to a retry.
  */
@@ -2492,7 +2488,10 @@ nfs_readdirplusrpc(struct vnode *vp, str
  nfsm_adv(nfsm_rndup(i));
  }
  if (newvp != NULLVP) {
- vrele(newvp);
+ if (newvp == vp)
+ vrele(newvp);
+ else
+ vput(newvp);
  newvp = NULLVP;
  }
  nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED);
@@ -2533,8 +2532,12 @@ nfs_readdirplusrpc(struct vnode *vp, str
  }
 
 nfsmout:
- if (newvp != NULLVP)
- vrele(newvp);
+ if (newvp != NULLVP) {
+ if (newvp == vp)
+ vrele(newvp);
+ else
+ vput(newvp);
+ }
  return (error);
 }
 
@@ -2659,7 +2662,7 @@ nfs_lookitup(struct vnode *dvp, char *na
  nfsm_postop_attr(newvp, attrflag);
  if (!attrflag && *npp == NULL) {
  m_freem(info.nmi_mrep);
- vrele(newvp);
+ vput(newvp);
  return (ENOENT);
  }
  } else
@@ -2670,7 +2673,7 @@ nfsmout:
  if (npp && *npp == NULL) {
  if (error) {
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else
  *npp = np;
  }

Reply | Threaded
Open this post in threaded view
|

Re: vput(9) for NFS

Martin Pieuchot
On 09/04/18(Mon) 16:10, Martin Pieuchot wrote:

> Diff below implements most of the missing locking goo for NFS without
> enabling it.  It does the following:
>
> - Add a missing PDIRUNLOCK in nfs_lookup()
>
> - Change vrele(9) into vput(9) where necessary.  nfs_nget() will in
>   future return a locked NFSnode.  Just like ffs_vget() returns a
>   locked vnode.  So every time it is called we need a corresponding
>   vput(9).
>
> - Make sure VN_KNOTE() is always called with a valid reference, before
>   vrele(9) or vput(9).
>
> - Substitute an XXX by an assert in nfs_removeit().
>
> However as long as nfs_lock/unlock are noops, this diff should not
> introduce any change in behavior.  But please do test this diff :)

New version of the diff after a review from visa@.  It includes:

- Stop calling vput(9) inside nfs_mknodrpc() to be able to call
  VN_KNOTE() with a proper reference in nfs_mknod().

- Do not unconditionally unlock a node in an error case in
  nfs_lookitup().

- Fix a dangling reference in the case of 'goto again' in nfs_create().

Index: nfs/nfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/nfs/nfs_vnops.c,v
retrieving revision 1.171
diff -u -p -r1.171 nfs_vnops.c
--- nfs/nfs_vnops.c 22 Feb 2017 11:42:46 -0000 1.171
+++ nfs/nfs_vnops.c 11 Apr 2018 10:21:11 -0000
@@ -769,6 +769,7 @@ nfs_lookup(void *v)
  flags = cnp->cn_flags;
 
  *vpp = NULLVP;
+ newvp = NULLVP;
  if ((flags & ISLASTCN) && (dvp->v_mount->mnt_flag & MNT_RDONLY) &&
     (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME))
  return (EROFS);
@@ -836,8 +837,10 @@ nfs_lookup(void *v)
  if (cnp->cn_nameiop != LOOKUP && (flags & ISLASTCN))
  cnp->cn_flags |= SAVENAME;
  if ((!lockparent || !(flags & ISLASTCN)) &&
-     newvp != dvp)
+     newvp != dvp) {
  VOP_UNLOCK(dvp, p);
+ cnp->cn_flags |= PDIRUNLOCK;
+ }
  return (0);
  }
  cache_purge(newvp);
@@ -1282,7 +1285,6 @@ nfs_mknodrpc(struct vnode *dvp, struct v
  rdev = nfs_xdrneg1;
  else {
  VOP_ABORTOP(dvp, cnp);
- vput(dvp);
  return (EOPNOTSUPP);
  }
  nfsstats.rpccnt[NFSPROC_MKNOD]++;
@@ -1318,10 +1320,6 @@ nfs_mknodrpc(struct vnode *dvp, struct v
  if (!error) {
  nfsm_mtofh(dvp, newvp, info.nmi_v3, gotvp);
  if (!gotvp) {
- if (newvp) {
- vrele(newvp);
- newvp = NULL;
- }
  error = nfs_lookitup(dvp, cnp->cn_nameptr,
     cnp->cn_namelen, cnp->cn_cred, cnp->cn_proc, &np);
  if (!error)
@@ -1335,7 +1333,7 @@ nfs_mknodrpc(struct vnode *dvp, struct v
 nfsmout:
  if (error) {
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else {
  if (cnp->cn_flags & MAKEENTRY)
  nfs_cache_enter(dvp, newvp, cnp);
@@ -1345,7 +1343,6 @@ nfsmout:
  VTONFS(dvp)->n_flag |= NMODIFIED;
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
- vrele(dvp);
  return (error);
 }
 
@@ -1362,9 +1359,10 @@ nfs_mknod(void *v)
 
  error = nfs_mknodrpc(ap->a_dvp, &newvp, ap->a_cnp, ap->a_vap);
  if (!error)
- vrele(newvp);
+ vput(newvp);
 
  VN_KNOTE(ap->a_dvp, NOTE_WRITE);
+ vput(ap->a_dvp);
 
  return (error);
 }
@@ -1390,8 +1388,11 @@ nfs_create(void *v)
  /*
  * Oops, not for me..
  */
- if (vap->va_type == VSOCK)
- return (nfs_mknodrpc(dvp, ap->a_vpp, cnp, vap));
+ if (vap->va_type == VSOCK) {
+ error = nfs_mknodrpc(dvp, ap->a_vpp, cnp, vap);
+ vput(dvp);
+ return (error);
+ }
 
  if (vap->va_vaflags & VA_EXCLUSIVE)
  fmode |= O_EXCL;
@@ -1430,10 +1431,6 @@ again:
  if (!error) {
  nfsm_mtofh(dvp, newvp, info.nmi_v3, gotvp);
  if (!gotvp) {
- if (newvp) {
- vrele(newvp);
- newvp = NULL;
- }
  error = nfs_lookitup(dvp, cnp->cn_nameptr,
     cnp->cn_namelen, cnp->cn_cred, cnp->cn_proc, &np);
  if (!error)
@@ -1446,12 +1443,14 @@ again:
 
 nfsmout:
  if (error) {
+ if (newvp) {
+ vput(newvp);
+ newvp = NULL;
+ }
  if (info.nmi_v3 && (fmode & O_EXCL) && error == NFSERR_NOTSUPP) {
  fmode &= ~O_EXCL;
  goto again;
  }
- if (newvp)
- vrele(newvp);
  } else if (info.nmi_v3 && (fmode & O_EXCL))
  error = nfs_setattrrpc(newvp, vap, cnp->cn_cred, cnp->cn_proc);
  if (!error) {
@@ -1464,7 +1463,7 @@ nfsmout:
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
  VN_KNOTE(ap->a_dvp, NOTE_WRITE);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1530,12 +1529,13 @@ nfs_remove(void *v)
  error = nfs_sillyrename(dvp, vp, cnp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  NFS_INVALIDATE_ATTRCACHE(np);
- vrele(dvp);
- vrele(vp);
-
  VN_KNOTE(vp, NOTE_DELETE);
  VN_KNOTE(dvp, NOTE_WRITE);
-
+ if (vp == dvp)
+ vrele(vp);
+ else
+ vput(vp);
+ vput(dvp);
  return (error);
 }
 
@@ -1545,9 +1545,9 @@ nfs_remove(void *v)
 int
 nfs_removeit(struct sillyrename *sp)
 {
+ KASSERT(VOP_ISLOCKED(sp->s_dvp));
  /*
  * Make sure that the directory vnode is still valid.
- * XXX we should lock sp->s_dvp here.
  *
  * NFS can potentially try to nuke a silly *after* the directory
  * has already been pushed out on a forced unmount. Since the silly
@@ -1628,7 +1628,7 @@ nfs_rename(void *v)
  if (tvp && tvp->v_usecount > 1 && !VTONFS(tvp)->n_sillyrename &&
     tvp->v_type != VDIR && !nfs_sillyrename(tdvp, tvp, tcnp)) {
  VN_KNOTE(tvp, NOTE_DELETE);
- vrele(tvp);
+ vput(tvp);
  tvp = NULL;
  }
 
@@ -1827,13 +1827,13 @@ nfs_symlink(void *v)
 
 nfsmout:
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  VTONFS(dvp)->n_flag |= NMODIFIED;
  if (!wccflag)
  NFS_INVALIDATE_ATTRCACHE(VTONFS(dvp));
  VN_KNOTE(dvp, NOTE_WRITE);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1904,7 +1904,7 @@ nfsmout:
  }
  if (error) {
  if (newvp)
- vrele(newvp);
+ vput(newvp);
  } else {
  VN_KNOTE(dvp, NOTE_WRITE|NOTE_LINK);
  if (cnp->cn_flags & MAKEENTRY)
@@ -1912,7 +1912,7 @@ nfsmout:
  *ap->a_vpp = newvp;
  }
  pool_put(&namei_pool, cnp->cn_pnbuf);
- vrele(dvp);
+ vput(dvp);
  return (error);
 }
 
@@ -1936,7 +1936,7 @@ nfs_rmdir(void *v)
 
  if (dvp == vp) {
  vrele(dvp);
- vrele(dvp);
+ vput(dvp);
  pool_put(&namei_pool, cnp->cn_pnbuf);
  return (EINVAL);
  }
@@ -1964,8 +1964,8 @@ nfsmout:
  VN_KNOTE(vp, NOTE_DELETE);
 
  cache_purge(vp);
- vrele(vp);
- vrele(dvp);
+ vput(vp);
+ vput(dvp);
  /*
  * Kludge: Map ENOENT => 0 assuming that you have a reply to a retry.
  */
@@ -2492,7 +2492,10 @@ nfs_readdirplusrpc(struct vnode *vp, str
  nfsm_adv(nfsm_rndup(i));
  }
  if (newvp != NULLVP) {
- vrele(newvp);
+ if (newvp == vp)
+ vrele(newvp);
+ else
+ vput(newvp);
  newvp = NULLVP;
  }
  nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED);
@@ -2533,8 +2536,12 @@ nfs_readdirplusrpc(struct vnode *vp, str
  }
 
 nfsmout:
- if (newvp != NULLVP)
- vrele(newvp);
+ if (newvp != NULLVP) {
+ if (newvp == vp)
+ vrele(newvp);
+ else
+ vput(newvp);
+ }
  return (error);
 }
 
@@ -2659,7 +2666,10 @@ nfs_lookitup(struct vnode *dvp, char *na
  nfsm_postop_attr(newvp, attrflag);
  if (!attrflag && *npp == NULL) {
  m_freem(info.nmi_mrep);
- vrele(newvp);
+ if (newvp == dvp)
+ vrele(newvp);
+ else
+ vput(newvp);
  return (ENOENT);
  }
  } else
@@ -2669,8 +2679,10 @@ nfs_lookitup(struct vnode *dvp, char *na
 nfsmout:
  if (npp && *npp == NULL) {
  if (error) {
- if (newvp)
+ if (newvp == dvp)
  vrele(newvp);
+ else
+ vput(newvp);
  } else
  *npp = np;
  }

Reply | Threaded
Open this post in threaded view
|

Re: vput(9) for NFS

Visa Hankala-2
On Wed, Apr 11, 2018 at 12:34:03PM +0200, Martin Pieuchot wrote:

> On 09/04/18(Mon) 16:10, Martin Pieuchot wrote:
> > Diff below implements most of the missing locking goo for NFS without
> > enabling it.  It does the following:
> >
> > - Add a missing PDIRUNLOCK in nfs_lookup()
> >
> > - Change vrele(9) into vput(9) where necessary.  nfs_nget() will in
> >   future return a locked NFSnode.  Just like ffs_vget() returns a
> >   locked vnode.  So every time it is called we need a corresponding
> >   vput(9).
> >
> > - Make sure VN_KNOTE() is always called with a valid reference, before
> >   vrele(9) or vput(9).
> >
> > - Substitute an XXX by an assert in nfs_removeit().
> >
> > However as long as nfs_lock/unlock are noops, this diff should not
> > introduce any change in behavior.  But please do test this diff :)
>
> New version of the diff after a review from visa@.  It includes:
>
> - Stop calling vput(9) inside nfs_mknodrpc() to be able to call
>   VN_KNOTE() with a proper reference in nfs_mknod().
>
> - Do not unconditionally unlock a node in an error case in
>   nfs_lookitup().
>
> - Fix a dangling reference in the case of 'goto again' in nfs_create().

OK visa@, with one more tweak:

The KASSERT(VOP_ISLOCKED(sp->s_dvp)) in nfs_removeit() should be
commented out until the locking is enabled. Currently, VOP_ISLOCKED()
returns zero with NFS vnodes.

Reply | Threaded
Open this post in threaded view
|

Re: vput(9) for NFS

Alexander Bluhm
In reply to this post by Martin Pieuchot
On Wed, Apr 11, 2018 at 12:34:03PM +0200, Martin Pieuchot wrote:
> @@ -769,6 +769,7 @@ nfs_lookup(void *v)
>   flags = cnp->cn_flags;
>  
>   *vpp = NULLVP;
> + newvp = NULLVP;
>   if ((flags & ISLASTCN) && (dvp->v_mount->mnt_flag & MNT_RDONLY) &&
>      (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME))
>   return (EROFS);

I dont see why this is necessary, but it does not hurt either.

> @@ -1545,9 +1545,9 @@ nfs_remove(void *v)
>  int
>  nfs_removeit(struct sillyrename *sp)
>  {
> + KASSERT(VOP_ISLOCKED(sp->s_dvp));
>   /*
>   * Make sure that the directory vnode is still valid.
> - * XXX we should lock sp->s_dvp here.

With the assert removed it passes regress, otherwise it panics.

bluhm