On Thu, Jul 14, 2022 at 04:39:33AM +0300, Vitaliy Makkoveev wrote:
> It looks like sometimes the `unp_conn' doesn't reloaded in
> the "unp->unp_conn != unp2" check, and READ_ONCE() should prevent this.
Are you sure about the non-reloading of unp->unp_conn? I did a quick
look in the compiled output on amd64, mips64 and riscv64, and did not
spot any obvious change (there is no change on mips64 and riscv64).
The loads are surrounded by lock/reference operations that imply
memory clobbering. The compiler should not assume that unp->unp_conn
is unchanged.
> unp_solock_peer(struct socket *so)
> {
> struct unpcb *unp, *unp2;
> struct socket *so2;
>
> unp = so->so_pcb;
>
> again:
> if ((unp2 = unp->unp_conn) == NULL)
> return NULL;
>
> so2 = unp2->unp_socket;
>
> if (so < so2)
> solock(so2);
> else if (so > so2){
> unp_ref(unp2);
> sounlock(so);
> solock(so2);
> solock(so);
>
> /* Datagram socket could be reconnected due to re-lock. */
> if (unp->unp_conn != unp2) {
> sounlock(so2);
> unp_rele(unp2);
> goto again;
> }
>
> unp_rele(unp2);
> }
>
> return so2;
> }
>
> Index: sys/kern/uipc_usrreq.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
> retrieving revision 1.167
> diff -u -p -r1.167 uipc_usrreq.c
> --- sys/kern/uipc_usrreq.c 2 Jul 2022 11:49:23 -0000 1.167
> +++ sys/kern/uipc_usrreq.c 14 Jul 2022 01:08:22 -0000
> @@ -154,7 +154,7 @@ unp_solock_peer(struct socket *so)
> unp = so->so_pcb;
>
> again:
> - if ((unp2 = unp->unp_conn) == NULL)
> + if ((unp2 = READ_ONCE(unp->unp_conn)) == NULL)
> return NULL;
>
> so2 = unp2->unp_socket;
> @@ -168,7 +168,7 @@ again:
> solock(so);
>
> /* Datagram socket could be reconnected due to re-lock. */
> - if (unp->unp_conn != unp2) {
> + if (READ_ONCE(unp->unp_conn) != unp2) {
> sounlock(so2);
> unp_rele(unp2);
> goto again;
>
