On Tue, Mar 17, 2020 at 1:07 AM Martin Pieuchot <[email protected]> wrote:
> On 16/03/20(Mon) 14:01, Martin Pieuchot wrote:
> > vget(9) might fail, stop right away if that happens.
> >
> > CID 1453020 Unchecked return value.
>
> Updated diff that stops tracing if vget(9) fails, similar to what's
> currently done if VOP_WRITE(9) fails, suggested by visa@.
>
> Code shuffling is there to avoid calling vput(9) if vget(9) doesn't
> succeed.
>
> ok?
>
> 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 09:46:03 -0000
> @@ -649,12 +649,17 @@ 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.
> */
> @@ -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?
Maybe this vget() should be split into vref() + vn_lock(), and the vput()
similarly split into VOP_UNLOCK() + vrele(), so the vrele() can be left
after this LIST_FOREACH() while the VOP_UNLOCK() is hoisted to the
"vn_lock() succeeded" path.
Philip Guenther
