Date: Tue, 29 Jun 2010 21:26:04 GMT From: Efstratios Karatzas <gpf@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 180327 for review Message-ID: <201006292126.o5TLQ40E087548@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@180327?ac=10 Change 180327 by gpf@gpf_desktop on 2010/06/29 21:25:59 - fixed a minor bug in nfsrv_auditpath() wrapper function. We don't really need to have a file handle to get the parent of the vnode if e.g. our fs is ZFS, VOP_GETPARENT() does require a 'hint'. - found & fixed an interesting bug: I hadn't spotted this before because I wasn't auditing the class 'ad' (administrative), but now I saw that auditing this class causes my code to kernel panic. The kernel thread that traps via the nfssvc(2) syscall never really exits. It stays in the kernel as nfsd and services various NFS RPCs. So it never really audit_syscall_exit()s and the first time it will try to service an RPC and audit_nfs_enter(), it will kernel panic because we already have an active td_ar. Providing audit support for multiple simultaneous td_ars does not solve this problem so I applied a fix of sorts, please refer to the comment+code in sys/nfs/nfs_nfssvc.c. - minor typo fixes in /etc/audit_event - tried to add audit support for a few nfsv4 ops and hit a wall. I'm tired of saying that auditing paths from vnodes for UFS requires a connection between vnode + parent directory that contains it. For NFSv 2 & 3, we keep a 'hint' directory inode. This hint is kept with the file handle that is sent back to the client, et voila. NFSv4 has a peculiar way of doing a lot of things. For example, say we wish to acquire the fh of a regular file. This is the compound RPC: putfh(directory filehandle), lookup(file entry name), getfh(). Note that in NFSv4, lookup() is supposed to change the 'current file handle' (to that of the file that is looked up), and getfh() is supposed to return the file handle via the reply buffer of the RPC. In FreeBSD's nfsv4 implementation, the 'current' and saved file handles' are actually current and saved vnode pointers. The author notes that it might be cleaner to save & use fhs instead of vps. So, although NFSv3 would return a filehandle inside lookup(), NFSv4 will return the filehandle through getfh(). getfh() should push the current filehandle into the reply buffer of the rpc. But since the current filehandle is actually a vnode pointer, it can only do a VOP_VPTOFH() and therefore, not save the beforementioned hint. To put it simply, we can't audit file paths when the namecache fails us, because of the way nfsv4 is implemented. It will require some considerable change to work the way I want it to. Note: excuse the lengthy description but I'm writing this in hopes of my mentor and Rick Macklem one day reading this. Thank you Affected files ... .. //depot/projects/soc2010/gpf_audit/freebsd/src/contrib/openbsm/etc/audit_event#4 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/fs/nfsserver/nfs_nfsdsocket.c#9 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/fs/nfsserver/nfs_nfsdsubs.c#3 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/nfs/nfs_nfssvc.c#2 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/nfsserver/nfs_serv.c#17 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/security/audit/audit.c#8 edit .. //depot/projects/soc2010/gpf_audit/freebsd/src/sys/security/audit/audit_bsm.c#11 edit Differences ... ==== //depot/projects/soc2010/gpf_audit/freebsd/src/contrib/openbsm/etc/audit_event#4 (text) ==== @@ -391,7 +391,7 @@ 2023:AUE_NFS_CLOSE:nfsrv_close():cl 2024:AUE_NFS_DELEGPURGE:nfsrv_delegpurge():ad 2025:AUE_NFS_DELEGRETURN:nfsrv_delegreturn():ad -2026:AUE_NFSv4_GETFH:nfsrv_getfh():ad +2026:AUE_NFSv4_GETFH:nfsrv4_getfh():ad 2027:AUE_NFS_LOCK:nfsrv_lock():fm 2028:AUE_NFS_LOCKT:nfsrv_lockt():fm 2029:AUE_NFS_LOCKU:nfsrv_locku():fm @@ -403,7 +403,7 @@ 2035:AUE_NFS_OPENDOWNGRADE:nfsrv_opendowngrade():fm 2036:AUE_NFS_PUTFH:nfsrv_putfh():ad 2037:AUE_NFS_PUTPUBFH:nfsrv_putpubfh():ad -2038:AUE_NFS_PUTROOTFH:nfsrv_rootfh():ad +2038:AUE_NFS_PUTROOTFH:nfsrv_putrootfh():ad 2039:AUE_NFS_RENEW:nfsrv_renew():ad 2040:AUE_NFS_RESTOREFH:nfsrv_restorefh():ad 2041:AUE_NFS_SAVEFH:nfsrv_savefh():ad ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/fs/nfsserver/nfs_nfsdsocket.c#9 (text+ko) ==== @@ -436,6 +436,7 @@ nfsrvd_compound(nd, isdgram, p); printf("compound rpc exit\n"); } else { + printf("non compound rpc %d\n", nd->nd_procnum); if (nd->nd_flag & ND_NFSV2) nfsprot = ND_NFSV2; else @@ -736,14 +737,19 @@ } if (nfsv4_opflag[op].savereply) nd->nd_flag |= ND_SAVEREPLY; - NFSINCRGLOBAL(newnfsstats.srvrpccnt[nd->nd_procnum]); + NFSINCRGLOBAL(newnfsstats.srvrpccnt[nd->nd_procnum]); + AUDIT_NFS_ENTER(op, nd->nd_cred, curthread, ND_NFSV4); + if (nd->nd_nam != NULL) + AUDIT_ARG_SOCKADDR_IN((struct sockaddr_in *)nd->nd_nam); switch (op) { /* xxx gpf */ printf("op = %d\n", op); case NFSV4OP_PUTFH: error = nfsrv_mtofh(nd, &fh); - if (error) + if (error) { + printf("error! %p\n", curthread); goto nfsmout; + } if (!nd->nd_repstat) { nes.nes_vfslocked = vpnes.nes_vfslocked; nfsd_fhtovp(nd, &fh, &nvp, &nes, &mp, @@ -754,7 +760,12 @@ if (vp) vrele(vp); vp = nvp; + vref(vp); + AUDIT_ARG_VNODE1(vp); NFSVOPUNLOCK(vp, 0, p); + nfsrv_auditpath(vp, NULL, NULL, + (fhandle_t *)fh.nfsrvfh_data, 1); + vrele(vp); vpnes = nes; } break; @@ -770,7 +781,12 @@ if (vp) vrele(vp); vp = nvp; + vref(vp); + AUDIT_ARG_VNODE1(vp); NFSVOPUNLOCK(vp, 0, p); + nfsrv_auditpath(vp, NULL, NULL, + (fhandle_t *)nfs_pubfh.nfsrvfh_data, 1); + vrele(vp); vpnes = nes; } break; @@ -783,7 +799,12 @@ if (vp) vrele(vp); vp = nvp; + vref(vp); + AUDIT_ARG_VNODE1(vp); NFSVOPUNLOCK(vp, 0, p); + nfsrv_auditpath(vp, NULL, NULL, + (fhandle_t *)nfs_rootfh.nfsrvfh_data, 1); + vrele(vp); vpnes = nes; } } else if (nfsv4root_vp && nfsv4root_set) { @@ -813,6 +834,14 @@ savevpnes = vpnes; savemp = mp; } + /* XXXgpf: is this the correct filehandle? */ + if (savevp) { + nfsrv_auditpath(savevp, NULL, NULL, + (fhandle_t *)fh.nfsrvfh_data, 1); + vn_lock(savevp, LK_EXCLUSIVE); + AUDIT_ARG_VNODE1(savevp); + VOP_UNLOCK(savevp, 0); + } } else { nd->nd_repstat = NFSERR_NOFILEHANDLE; } @@ -820,6 +849,14 @@ case NFSV4OP_RESTOREFH: if (savevp) { nd->nd_repstat = 0; + /* XXXgpf: file handle? */ + vref(savevp); + nfsrv_auditpath(savevp, NULL, NULL, + NULL, 1); + vn_lock(savevp, LK_EXCLUSIVE); + AUDIT_ARG_VNODE1(savevp); + VOP_UNLOCK(savevp, 0); + vrele(savevp); /* If vp == savevp, a no-op */ if (vp != savevp) { VREF(savevp); @@ -945,8 +982,16 @@ if (nfsv4_opflag[op].modifyfs) NFS_STARTWRITE(NULL, &mp); NFSVOPLOCK(savevp, LK_EXCLUSIVE | LK_RETRY, p); + if (savevp) + vref(savevp); error = (*(nfsrv4_ops2[op]))(nd, isdgram, savevp, vp, p, &savevpnes, &vpnes); + if (savevp) { + if (nd->nd_procnum == NFSPROC_LINK) + nfsrv_auditpath(savevp, NULL, NULL, + (fhandle_t *)fh.nfsrvfh_data, 2); + vrele(savevp); + } if (nfsv4_opflag[op].modifyfs) NFS_ENDWRITE(mp); } else { @@ -981,6 +1026,7 @@ } } }; + AUDIT_NFS_EXIT(nd->nd_repstat, curthread); if (error) { if (error == EBADRPC || error == NFSERR_BADXDR) { nd->nd_repstat = NFSERR_BADXDR; @@ -999,6 +1045,9 @@ } } nfsmout: + /* XXXgpf: when error occurs, we may jump here */ + AUDIT_NFS_EXIT(nd->nd_repstat, curthread); + KASSERT(curthread->td_ar == NULL, ("gamw sto nfsmout: td->td_ar != NULL")); if (error) { if (error == EBADRPC || error == NFSERR_BADXDR) nd->nd_repstat = NFSERR_BADXDR; ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/fs/nfsserver/nfs_nfsdsubs.c#3 (text+ko) ==== @@ -2061,6 +2061,9 @@ * fname - name used to reference vp inside dvp * fhp - file handle for vp * n - AUDIT_ARG_UPATH1 or AUDIT_ARG_UPATH2 + * + * Note: vp and dvp may be vref'd but not locks should be held on them as the + * two vn_fullpath_* KPIs may try to lock them themselves. */ void nfsrv_auditpath(vnode_t vp, vnode_t dvp, char *fname, fhandle_t *fhp, int n) @@ -2085,11 +2088,13 @@ success = 1; goto out; } + /* if our cache fails us */ - if (fhp != NULL && vp->v_mount != NULL) { + if (vp->v_mount != NULL) { uint64_t parent_hint; /* get the hint stored inside the file handle */ - VFS_FHHINT(vp->v_mount, &fhp->fh_fid, &parent_hint); + if (fhp != NULL) + VFS_FHHINT(vp->v_mount, &fhp->fh_fid, &parent_hint); vn_fullpath_nocache(vp, &fullpath, &freepath, parent_hint, PARENTHINT | WANTNAME); if (freepath != NULL) { ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/nfs/nfs_nfssvc.c#2 (text+ko) ==== @@ -79,7 +79,18 @@ KASSERT(!mtx_owned(&Giant), ("nfssvc(): called with Giant")); - AUDIT_ARG_CMD(uap->flag); + AUDIT_ARG_CMD(uap->flag); + /* + * XXX:gpf quick fix until I figure out something better. + * Even if Audit supports multiple simultaneous td_ars per + * kernel thread, we still want to end the audit record for + * nfs_svc so that Audit records for NFS RPCs that are serviced + * by this thread are recorded without any trouble such as waiting + * for nfssvc() to return before commiting them to the audit daemon. + * Problem is that we are now *not* recording the return value from + * the nfs server. + */ + AUDIT_SYSCALL_EXIT(0, td); error = priv_check(td, PRIV_NFS_DAEMON); if (error) ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/nfsserver/nfs_serv.c#17 (text+ko) ==== @@ -203,6 +203,9 @@ * fname - name used to reference vp inside dvp * fhp - file handle for vp * n - AUDIT_ARG_UPATH1 or AUDIT_ARG_UPATH2 + * + * Note: vp and dvp may be vref'd but not locks should be held on them as the + * two vn_fullpath_* KPIs may try to lock them themselves. */ static void nfsrv_auditpath(struct vnode *vp, struct vnode *dvp, char *fname, fhandle_t *fhp, int n) @@ -226,10 +229,11 @@ } /* if our cache fails us */ - if (fhp != NULL && vp->v_mount != NULL) { + if (vp->v_mount != NULL) { uint64_t parent_hint; /* get the hint stored inside the file handle */ - VFS_FHHINT(vp->v_mount, &fhp->fh_fid, &parent_hint); + if (fhp != NULL) + VFS_FHHINT(vp->v_mount, &fhp->fh_fid, &parent_hint); vn_fullpath_nocache(vp, &fullpath, &freepath, parent_hint, PARENTHINT | WANTNAME); if (freepath != NULL) { ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/security/audit/audit.c#8 (text) ==== @@ -706,6 +706,9 @@ au_id_t auid; int error; + if (td->td_ar != NULL) { + printf("bug event = %d\n", td->td_ar->k_ar.ar_event); + } KASSERT(td->td_ar == NULL, ("audit_nfs_enter: td->td_ar != NULL")); KASSERT((td->td_pflags & TDP_AUDITREC) == 0, ("audit_nfs_enter: TDP_AUDITREC set")); @@ -797,7 +800,6 @@ audit_nfs_exit(int error, struct thread *td) { int retval; - /* * Commit the audit record as desired; once we pass the record into * audit_commit(), the memory is owned by the audit subsystem. The ==== //depot/projects/soc2010/gpf_audit/freebsd/src/sys/security/audit/audit_bsm.c#11 (text) ==== @@ -1648,6 +1648,44 @@ } break; + case AUE_NFS_PUTFH: + case AUE_NFS_PUTPUBFH: + case AUE_NFS_PUTROOTFH: + case AUE_NFS_RESTOREFH: + case AUE_NFS_SAVEFH: + UPATH1_VNODE1_TOKENS; + if (ARG_IS_VALID(kar, ARG_TEXT)) { + tok = au_to_text(ar->ar_arg_text); + kau_write(rec, tok); + } + break; + + /* XXXgpf: temporary fallthrough for nfsv4 events */ + case AUE_NFS_CLOSE: + case AUE_NFS_DELEGPURGE: + case AUE_NFS_DELEGRETURN: + case AUE_NFSv4_GETFH: + case AUE_NFS_LOCK: + case AUE_NFS_LOCKT: + case AUE_NFS_LOCKU: + case AUE_NFS_LOOKUPP: + case AUE_NFS_NVERIFY: + case AUE_NFS_OPEN: + case AUE_NFS_OPENATTR: + case AUE_NFS_OPENCONFIRM: + case AUE_NFS_OPENDOWNGRADE: + case AUE_NFS_RENEW: + case AUE_NFS_SECINFO: + case AUE_NFS_SETCLIENTID: + case AUE_NFS_SETCLIENTIDCFRM: + case AUE_NFS_VERIFY: + case AUE_NFS_RELEASELCKOWN: + if (ARG_IS_VALID(kar, ARG_TEXT)) { + tok = au_to_text(ar->ar_arg_text); + kau_write(rec, tok); + } + break; + case AUE_WAIT4: PROCESS_PID_TOKENS(1); if (ARG_IS_VALID(kar, ARG_VALUE)) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201006292126.o5TLQ40E087548>