> On 14 Jul 2022, at 05:51, Visa Hankala <[email protected]> wrote:
>
> 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).
>
It seems to be. Otherwise I have no explanation of syzkaller reports.
May be it is not compiler, but CPU cache specific. I see the
difference on amd64, compiler loads to register before compare.
mov 0x20(%r13),%rax
cmp %r12,%rax
je 0xffff...
instead of
cmp %r12,0x20(%r13)
je 0xffff...
I can’t say what is the real difference in cmp behaviour in both
cases. May be some one could explain.
> 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;
>>
