On Thu, Jul 14, 2022 at 06:16:14AM +0300, Vitaliy Makkoveev wrote:
> > 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.
For me this chunk of assembly looks equivalent. If this is the
only binary change, I don't expect READ_ONCE() to fix syzkaller.
I am not against adding READ_ONCE(), it prevents unexpected compiler
optimisations. But currently such a optimization does not happen.
> > The loads are surrounded by lock/reference operations that imply
> > memory clobbering. The compiler should not assume that unp->unp_conn
> > is unchanged.
Maybe I did not understand correctly why READ_ONCE() is needed.
> >> if ((unp2 = unp->unp_conn) == NULL)
> >> return NULL;
> >>
> >> so2 = unp2->unp_socket;
Can the compiler modify this code to this?
if ((unp2 = unp->unp_conn) == NULL)
return NULL;
so2 = unp->unp_conn->unp_socket;
Then we can get a NULL deref if another CPU sets unp->unp_conn to
NULL between our two reads. There READ_ONCE() makes sense.
> >> 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);
Can the compiler assume unp->unp_conn == unp2 ? Then it would be
valid to do unp_rele(unp->unp_conn). Not sure whether C standard
allows this. And it would be stupid if the compiler would do that.
Sould we use more READ_ONCE() to be safe?
bluhm
