Page MenuHomeFreeBSD
Differential D15424 Diff 42614 sys/fs/nfsserver/nfs_nfsdport.c

Changeset View

Standalone View

View Options

sys/fs/nfsserver/nfs_nfsdport.c

Show First 20 Lines • Show All 2,163 Lines • ▼ Show 20 Lines if (dp->d_fileno != 0 && dp->d_type != DT_WHT &&
r = VFS_ROOT(new_mp, r = VFS_ROOT(new_mp,
LK_SHARED, &nvp); LK_SHARED, &nvp);
needs_unbusy = 1; needs_unbusy = 1;
if (r == 0) if (r == 0)
at_root = 1; at_root = 1;
} }
} }
} }
if (!r) {
/*
* If we failed to look up the entry, then it
* has become invalid, most likely removed.
julianUnsubmitted
Done
Inline Actions

become, not became.. it's in this case would be short for "it has".
Possibly Best to actually spell it that way.

julian: become, not became.. it's in this case would be short for "it has". Possibly Best to…
*/
if (r != 0) {
if (needs_unbusy)
vfs_unbusy(new_mp);
goto invalid;
}
KASSERT(refp != NULL || nvp != NULL,
rmacklemUnsubmitted
Done
Inline Actions

Maybe you could stick a
KASSERT(nvp != NULL. ...);
here as a asafety belt against future changes that might result in the code
getting down here without a valid nvp?
(I agree that it should be non-NULL when you get here after your changes.)

rmacklem: Maybe you could stick a KASSERT(nvp != NULL. ...); here as a asafety belt against future…
avgAuthorUnsubmitted
Done
Inline Actions

The only reason I didn't add such an assertion is that NULL pointer dereference is a very obvious kind of bug. But I agree that the assertion can be helpful to readers and maintainers of the code, so I am adding it now.
Also, I overlooked the case where we get a non-NULL refp for NFSv4, so we do not get nvp at all.

avg: The only reason I didn't add such an assertion is that NULL pointer dereference is a very…
("%s: undetected lookup error", __func__));
if (refp == NULL && if (refp == NULL &&
((nd->nd_flag & ND_NFSV3) || ((nd->nd_flag & ND_NFSV3) ||
NFSNONZERO_ATTRBIT(&attrbits))) { NFSNONZERO_ATTRBIT(&attrbits))) {
r = nfsvno_getfh(nvp, &nfh, p); r = nfsvno_getfh(nvp, &nfh, p);
if (!r) if (r == 0) {
r = nfsvno_getattr(nvp, nvap, r = nfsvno_getattr(nvp, nvap,
nd->nd_cred, p, 1); nd->nd_cred, p, 1);
if (r == 0 && is_zfs == 1 && }
nfsrv_enable_crossmntpt != 0 &&
(nd->nd_flag & ND_NFSV4) != 0 &&
nvp->v_type == VDIR &&
vp->v_mount != nvp->v_mount) {
/* /*
* For a ZFS snapshot, there is a * For a ZFS snapshot, there is a
* pseudo mount that does not set * pseudo mount that does not set
* v_mountedhere, so it needs to * v_mountedhere, so it needs to
* be detected via a different * be detected via a different
* mount structure. * mount structure.
*/ */
if (r == 0 && is_zfs == 1 &&
nfsrv_enable_crossmntpt != 0 &&
(nd->nd_flag & ND_NFSV4) != 0 &&
nvp->v_type == VDIR &&
vp->v_mount != nvp->v_mount) {
at_root = 1; at_root = 1;
if (new_mp == mp) if (new_mp == mp)
new_mp = nvp->v_mount; new_mp = nvp->v_mount;
} }
} }
} else {
/*
* If we failed to get attributes of the entry,
* then just skip it for NFSv3 (the traditional
* behavior in the old NFS server).
* For NFSv4 the behavior is controlled by
* RDATTRERROR: we either ignore the error or
* fail the request.
*/
if (r != 0) {
rmacklemUnsubmitted
Done
Inline Actions

Shouldn't there be a
{

vput(nvp);
if (needs_unbusy != 0)
      vfs_unbusy(new_mp);
goto invalid;

}
in this "if" as well as the code that follows, since "invalid:" is after code that does this,
instead of just a "goto invalid;"?

rmacklem: Shouldn't there be a { vput(nvp); if (needs_unbusy != 0) vfs_unbusy…
avgAuthorUnsubmitted
Done
Inline Actions

Good catch! I think that needs_unbusy is for NFSv4 only, but nvp must be released.

avg: Good catch! I think that `needs_unbusy` is for NFSv4 only, but //nvp// must be released.
if ((nd->nd_flag & ND_NFSV3) != 0) {
vput(nvp);
nvp = NULL; nvp = NULL;
goto invalid;
} }
if (r) {
if (!NFSISSET_ATTRBIT(&attrbits, if (!NFSISSET_ATTRBIT(&attrbits,
NFSATTRBIT_RDATTRERROR)) { NFSATTRBIT_RDATTRERROR)) {
if (nvp != NULL)
vput(nvp); vput(nvp);
nvp = NULL;
if (needs_unbusy != 0) if (needs_unbusy != 0)
vfs_unbusy(new_mp); vfs_unbusy(new_mp);
nd->nd_repstat = r; nd->nd_repstat = r;
break; break;
} }
} }
} }
/* /*
* Build the directory record xdr * Build the directory record xdr
*/ */
if (nd->nd_flag & ND_NFSV3) { if (nd->nd_flag & ND_NFSV3) {
NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED); NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED);
*tl++ = newnfs_true; *tl++ = newnfs_true;
*tl++ = 0; *tl++ = 0;
*tl = txdr_unsigned(dp->d_fileno); *tl = txdr_unsigned(dp->d_fileno);
dirlen += nfsm_strtom(nd, dp->d_name, nlen); dirlen += nfsm_strtom(nd, dp->d_name, nlen);
NFSM_BUILD(tl, u_int32_t *, 2 * NFSX_UNSIGNED); NFSM_BUILD(tl, u_int32_t *, 2 * NFSX_UNSIGNED);
*tl++ = 0; *tl++ = 0;
*tl = txdr_unsigned(*cookiep); *tl = txdr_unsigned(*cookiep);
nfsrv_postopattr(nd, 0, nvap); nfsrv_postopattr(nd, 0, nvap);
dirlen += nfsm_fhtom(nd,(u_int8_t *)&nfh,0,1); dirlen += nfsm_fhtom(nd,(u_int8_t *)&nfh,0,1);
dirlen += (5*NFSX_UNSIGNED+NFSX_V3POSTOPATTR); dirlen += (5*NFSX_UNSIGNED+NFSX_V3POSTOPATTR);
if (nvp != NULL) if (nvp != NULL)
vput(nvp); vput(nvp);
julianUnsubmitted
Done
Inline Actions

You are really sure about nvp never being NULL right? (e.g. line 2162)

julian: You are really sure about nvp never being NULL right? (e.g. line 2162)
avgAuthorUnsubmitted
Done
Inline Actions

Yes, I am. With this change, the code detects much earlier if it failed to get nvp. So, past that point nvp cannot be NULL. Modulo bugs, of course.

avg: Yes, I am. With this change, the code detects much earlier if it failed to get nvp. So, past…
avgAuthorUnsubmitted
Done
Inline Actions

Actually, I was very wrong! In the case of NFSv4, if all attribute bits are zero or if the entry has a referral, then we do not look up nvp at all, so it remains NULL.
Thank you for alerting me to this.

avg: Actually, I was //very// wrong! In the case of NFSv4, if all attribute bits are zero or if…
} else { } else {
NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED); NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED);
*tl++ = newnfs_true; *tl++ = newnfs_true;
*tl++ = 0; *tl++ = 0;
*tl = txdr_unsigned(*cookiep); *tl = txdr_unsigned(*cookiep);
dirlen += nfsm_strtom(nd, dp->d_name, nlen); dirlen += nfsm_strtom(nd, dp->d_name, nlen);
if (nvp != NULL) { if (nvp != NULL) {
supports_nfsv4acls = supports_nfsv4acls =
Show All 29 Lines if (dp->d_fileno != 0 && dp->d_type != DT_WHT &&
vrele(nvp); vrele(nvp);
dirlen += (3 * NFSX_UNSIGNED); dirlen += (3 * NFSX_UNSIGNED);
} }
if (needs_unbusy != 0) if (needs_unbusy != 0)
vfs_unbusy(new_mp); vfs_unbusy(new_mp);
if (dirlen <= cnt) if (dirlen <= cnt)
entrycnt++; entrycnt++;
} }
invalid:
cpos += dp->d_reclen; cpos += dp->d_reclen;
dp = (struct dirent *)cpos; dp = (struct dirent *)cpos;
cookiep++; cookiep++;
ncookies--; ncookies--;
} }
vrele(vp); vrele(vp);
vfs_unbusy(mp); vfs_unbusy(mp);
▲ Show 20 Lines • Show All 1,159 Lines • Show Last 20 Lines