From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: Don't unlock vnode(9) in fifo_read() and fifo_write()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 16 Apr 2024 16:30:08 +0200

On Tue, Apr 16, 2024 at 04:15:05PM +0300, Vitaliy Makkoveev wrote:
> I suspect syzkaller reported panics [1] and [2] happened because sockets
> were closed due vnode(9) lock release.
> 
> The solock() designed to be take after vnode(9) lock, and unix(4)
> sockets perform solock() release for that purpose. No reason to release
> vnode(9) lock in fifo_read() and fifo_write() around soreceive() and
> sosend() respectively.
> 
> I can't reproduce syzkaller panics, so I can't say the issues were
> fixed, but the unnecessary re-locking is the bad thing in any case.
> 
> I performed regress/sys/fifofs/ and regress/sys/kern/unixsockets/ test
> without witness(4) warnings.
> 
> 1. https://syzkaller.appspot.com/bug?extid=5779bc64fc4fdd0a5140
> 2. https://syzkaller.appspot.com/bug?extid=b7ef44521c90d0740ba3

I'm not sure if this diff is correct. I doubt it will fix this issue.

The problem here at hand is that a vnode gets repurposed while active.
That happens outside of the fifo code since e.g. fifo_read is called via
function pointer.

The call in [1] starts as a ufs_symlink() call but in the end tries to
write to a FIFO. That makes no sense. The ufs_symlink should call write on
a vnode that points into ufs not to a FIFO.

In [2] the issue is similar, a read of a vnd block ends up on a FIFO.
Again the file that vnd is backed with is most probably on UFS.

This VOP_UNLOCK() and then vn_lock() dance is also done by specfs for char
devices. In my opinion this looks more like a missing vref() which allows
the system to recycle a vnode where it should not.
VOP_UNLOCK() and vn_lock() are more about concurrent access to a file /
block.  The idea is that you can have multiple reader or writers since the
writes are atomic (seems a bit strange on FIFOs where a write may not be
atomic) but makes sense for things like /dev/tun or /dev/bpf.

I guess you need to reduce the number of vnodes in your system to trigger
aggressive vnode reuse to tickle this bug.
 
> Index: sys/miscfs/fifofs/fifo_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 fifo_vnops.c
> --- sys/miscfs/fifofs/fifo_vnops.c	26 Mar 2024 09:46:47 -0000	1.104
> +++ sys/miscfs/fifofs/fifo_vnops.c	16 Apr 2024 13:04:07 -0000
> @@ -252,9 +252,7 @@ fifo_read(void *v)
>  		return (0);
>  	if (ap->a_ioflag & IO_NDELAY)
>  		flags |= MSG_DONTWAIT;
> -	VOP_UNLOCK(ap->a_vp);
>  	error = soreceive(rso, NULL, uio, NULL, NULL, &flags, 0);
> -	vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY);
>  	if (ap->a_ioflag & IO_NDELAY) {
>  		if (error == EWOULDBLOCK &&
>  		    ap->a_vp->v_fifoinfo->fi_writers == 0)
> @@ -271,7 +269,7 @@ fifo_write(void *v)
>  {
>  	struct vop_write_args *ap = v;
>  	struct socket *wso = ap->a_vp->v_fifoinfo->fi_writesock;
> -	int error, flags = 0;
> +	int flags = 0;
>  
>  #ifdef DIAGNOSTIC
>  	if (ap->a_uio->uio_rw != UIO_WRITE)
> @@ -279,10 +277,7 @@ fifo_write(void *v)
>  #endif
>  	if (ap->a_ioflag & IO_NDELAY)
>  		flags |= MSG_DONTWAIT;
> -	VOP_UNLOCK(ap->a_vp);
> -	error = sosend(wso, NULL, ap->a_uio, NULL, NULL, flags);
> -	vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY);
> -	return (error);
> +	return sosend(wso, NULL, ap->a_uio, NULL, NULL, flags);
>  }
>  
>  /*
> 

-- 
:wq Claudio