On 17/03/20(Tue) 07:50, Philip Guenther wrote:
> On Tue, Mar 17, 2020 at 5:18 AM Martin Pieuchot <[email protected]> wrote:
> > On 17/03/20(Tue) 04:02, Philip Guenther wrote:
> > > On Tue, Mar 17, 2020 at 1:07 AM Martin Pieuchot <[email protected]> wrote:
> > > [...]
> > > > @@ -663,8 +668,6 @@ ktrwriteraw(struct proc *curp, struct vn
> > > > LIST_FOREACH(pr, &allprocess, ps_list)
> > > > if (pr->ps_tracevp == vp && pr->ps_tracecred == cred)
> > > > ktrcleartrace(pr);
> > > > -
> > > > - vput(vp);
> > > > return (error);
> > > > }
> > > >
> > >
> > > This looks unsafe to me: isn't ktrcleartrace() only safe if the caller
> > > holds a reference to the vnode? Once ktrcleartrace() clears the
> > reference
> > > from the current thread's process and it goes on the freelist, can't the
> > > vnode vp points to be invalidated and reused?
> >
> > As long as a process holds a reference to the vnode, via `ps_tracevp',
> > it wont be recycle. Only the last call of ktrcleartrace() will release
> > the vnode via vrele(9).
>
> ...and after that last reference is released this code will continue to
> walk the allprocess list, comparing a possibly-recycled pointer to
> ps_tracevp pointers in the remaining processes. Good thing that vrele(9)
> is guaranteed to never sleep and thereby let another process start
> ktracing, reusing that vnode.....??
Are you saying that `allprocess' should only be walked if the current
thread is still holding a valid reference to the vnode?
Index: kern/kern_ktrace.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.100
diff -u -p -r1.100 kern_ktrace.c
--- kern/kern_ktrace.c 6 Oct 2019 16:24:14 -0000 1.100
+++ kern/kern_ktrace.c 17 Mar 2020 17:14:46 -0000
@@ -649,22 +649,29 @@ ktrwriteraw(struct proc *curp, struct vn
auio.uio_iovcnt++;
auio.uio_resid += kth->ktr_len;
}
- vget(vp, LK_EXCLUSIVE | LK_RETRY);
+ error = vget(vp, LK_EXCLUSIVE | LK_RETRY);
+ if (error)
+ goto bad;
error = VOP_WRITE(vp, &auio, IO_UNIT|IO_APPEND, cred);
- if (!error) {
- vput(vp);
- return (0);
- }
+ vput(vp);
+ if (error)
+ goto bad;
+
+ return (0);
+
+bad:
/*
* If error encountered, give up tracing on this vnode.
*/
log(LOG_NOTICE, "ktrace write failed, errno %d, tracing stopped\n",
error);
- LIST_FOREACH(pr, &allprocess, ps_list)
+ LIST_FOREACH(pr, &allprocess, ps_list) {
+ if (pr == curp->p_p)
+ continue;
if (pr->ps_tracevp == vp && pr->ps_tracecred == cred)
ktrcleartrace(pr);
-
- vput(vp);
+ }
+ ktrcleartrace(curp->p_p);
return (error);
}
