Re: recvmsg() "short receive" after FIONREAD

On Sat, Sep 11, 2021 at 09:25:42PM +0300, Andriy Gapon wrote:
> On 11/09/2021 17:28, Andriy Gapon wrote:
> > On 11/09/2021 17:16, Andriy Gapon wrote:
> >> On 11/09/2021 17:13, Mark Johnston wrote:
> >>> I think the semantic change is ok.  Did you change FIONREAD to lock the
> >>> sockbuf?  I think it would be necessary to avoid races with pulseaudio:
> >>> sb_acc is modified before sb_ctl, so there could be windows where
> >>> sbavail(sb) - sb->sb_ctl gives a larger.
> >>>
> >>> And, it is not really safe to lock the sockbuf itself, since it may be
> >>> overwritten by a listen(2) call.  SOCK_RECVBUF_LOCK(so) should be used
> >>> instead.
> >>
> >> I didn't think about the locking, so I didn't add it.
> >> My current patch is trivial:
> >> @@ -210,7 +210,7 @@ soo_ioctl(struct file *fp, u_long cmd, void *data, struct 
> >> ucred *active_cred,
> >>                  if (SOLISTENING(so)) {
> >>                          error = EINVAL;
> >>                  } else {
> >> -                       *(int *)data = sbavail(&so->so_rcv);
> >> +                       *(int *)data = sbavail(&so->so_rcv) - so->so_rcv.sb_ctl;
> >>                  }
> >>                  break;
> >>
> >> Let me try adding the lock.
> > 
> > By the way, soo_stat() seems to be another good example to follow.
> 
> So, this is what I've got:
> diff --git a/sys/kern/sys_socket.c b/sys/kern/sys_socket.c
> index e53b0367960b..11ee03703407 100644
> --- a/sys/kern/sys_socket.c
> +++ b/sys/kern/sys_socket.c
> @@ -210,7 +210,12 @@ soo_ioctl(struct file *fp, u_long cmd, void *data, struct 
> ucred *active_cred,
>   		if (SOLISTENING(so)) {
>   			error = EINVAL;
>   		} else {
> -			*(int *)data = sbavail(&so->so_rcv);
> +			struct sockbuf *sb;
> +
> +			sb = &so->so_rcv;
> +			SOCKBUF_LOCK(sb);
> +			*(int *)data = sbavail(sb) - sb->sb_ctl;
> +			SOCKBUF_UNLOCK(sb);
>   		}
>   		break;

It should use SOCK_RECVBUF_LOCK() (see
https://cgit.freebsd.org/src/commit/?id=74a68313b503940158a2e8e8f02626d7cdbdaff9
):

	sb = &so->so_rcv;
	SOCK_RECVBUF_LOCK(so);
	if (SOLISTENING(so))
		error = EINVAL;
	else
		*(int *)data = sbavail(sb) - sb->sb_ctl;
	SOCK_RECVBUF_UNLOCK(so);

Otherwise a concurrent listen(2) will clobber the pointer used by
SOCKBUF_LOCK().