From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: socket filter read once
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 8 Nov 2024 18:42:26 +0300

On Fri, Nov 08, 2024 at 01:50:20PM +0100, Alexander Bluhm wrote:
> Hi,
> 
> The socket filt_...() funtions are called with shared netlock, but
> without per socket lock.  This can be done as they are read only.
> After unlocking, TCP will modify socket variables in parallel.  So
> I would like to explicitly mark with READ_ONCE() where we have
> unlocked access.
> 
> ok?
> 

`so_oobmark' belongs to reception path, so I want to use `so_rcv'
mutex(9) to protect it and already marked this in locking description.
This mutex(9) is held, so you don't need to load it to local variable.

> bluhm
> 
> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -r1.344 uipc_socket.c
> --- kern/uipc_socket.c	31 Oct 2024 12:51:55 -0000	1.344
> +++ kern/uipc_socket.c	8 Nov 2024 11:48:05 -0000
> @@ -2433,6 +2433,8 @@ int
>  filt_soread(struct knote *kn, long hint)
>  {
>  	struct socket *so = kn->kn_fp->f_data;
> +	u_int state = READ_ONCE(so->so_state);
> +	u_int error = READ_ONCE(so->so_error);
>  	int rv = 0;
>  
>  	MUTEX_ASSERT_LOCKED(&so->so_rcv.sb_mtx);
> @@ -2440,18 +2442,20 @@ filt_soread(struct knote *kn, long hint)
>  		soassertlocked_readonly(so);
>  
>  	if (so->so_options & SO_ACCEPTCONN) {
> +		short qlen = READ_ONCE(so->so_qlen);
> +
>  		if (so->so_rcv.sb_flags & SB_MTXLOCK)
>  			soassertlocked_readonly(so);
>  
> -		kn->kn_data = so->so_qlen;
> +		kn->kn_data = qlen;
>  		rv = (kn->kn_data != 0);
>  
>  		if (kn->kn_flags & (__EV_POLL | __EV_SELECT)) {
> -			if (so->so_state & SS_ISDISCONNECTED) {
> +			if (state & SS_ISDISCONNECTED) {
>  				kn->kn_flags |= __EV_HUP;
>  				rv = 1;
>  			} else {
> -				rv = soreadable(so);
> +				rv = qlen || soreadable(so);
>  			}
>  		}
>  
> @@ -2467,12 +2471,12 @@ filt_soread(struct knote *kn, long hint)
>  	if (so->so_rcv.sb_state & SS_CANTRCVMORE) {
>  		kn->kn_flags |= EV_EOF;
>  		if (kn->kn_flags & __EV_POLL) {
> -			if (so->so_state & SS_ISDISCONNECTED)
> +			if (state & SS_ISDISCONNECTED)
>  				kn->kn_flags |= __EV_HUP;
>  		}
> -		kn->kn_fflags = so->so_error;
> +		kn->kn_fflags = error;
>  		rv = 1;
> -	} else if (so->so_error) {
> +	} else if (error) {
>  		rv = 1;
>  	} else if (kn->kn_sfflags & NOTE_LOWAT) {
>  		rv = (kn->kn_data >= kn->kn_sdata);
> @@ -2495,6 +2499,8 @@ int
>  filt_sowrite(struct knote *kn, long hint)
>  {
>  	struct socket *so = kn->kn_fp->f_data;
> +	u_int state = READ_ONCE(so->so_state);
> +	u_int error = READ_ONCE(so->so_error);
>  	int rv;
>  
>  	MUTEX_ASSERT_LOCKED(&so->so_snd.sb_mtx);
> @@ -2505,14 +2511,14 @@ filt_sowrite(struct knote *kn, long hint
>  	if (so->so_snd.sb_state & SS_CANTSENDMORE) {
>  		kn->kn_flags |= EV_EOF;
>  		if (kn->kn_flags & __EV_POLL) {
> -			if (so->so_state & SS_ISDISCONNECTED)
> +			if (state & SS_ISDISCONNECTED)
>  				kn->kn_flags |= __EV_HUP;
>  		}
> -		kn->kn_fflags = so->so_error;
> +		kn->kn_fflags = error;
>  		rv = 1;
> -	} else if (so->so_error) {
> +	} else if (error) {
>  		rv = 1;
> -	} else if (((so->so_state & SS_ISCONNECTED) == 0) &&
> +	} else if (((state & SS_ISCONNECTED) == 0) &&
>  	    (so->so_proto->pr_flags & PR_CONNREQUIRED)) {
>  		rv = 0;
>  	} else if (kn->kn_sfflags & NOTE_LOWAT) {
> @@ -2540,15 +2546,19 @@ filt_soexcept(struct knote *kn, long hin
>  	} else
>  #endif /* SOCKET_SPLICE */
>  	if (kn->kn_sfflags & NOTE_OOB) {
> -		if (so->so_oobmark || (so->so_rcv.sb_state & SS_RCVATMARK)) {
> +		u_long oobmark = READ_ONCE(so->so_oobmark);
> +
> +		if (oobmark || (so->so_rcv.sb_state & SS_RCVATMARK)) {
>  			kn->kn_fflags |= NOTE_OOB;
> -			kn->kn_data -= so->so_oobmark;
> +			kn->kn_data -= oobmark;
>  			rv = 1;
>  		}
>  	}
>  
>  	if (kn->kn_flags & __EV_POLL) {
> -		if (so->so_state & SS_ISDISCONNECTED) {
> +		u_int state = READ_ONCE(so->so_state);
> +
> +		if (state & SS_ISDISCONNECTED) {
>  			kn->kn_flags |= __EV_HUP;
>  			rv = 1;
>  		}
>