On 12/19/24 15:48, Stefano Garzarella wrote: > On Thu, 19 Dec 2024 at 15:36, Michal Luczaj <mhal@xxxxxxx> wrote: >> >> On 12/19/24 09:19, Stefano Garzarella wrote: >>> ... >>> I think the best thing though is to better understand how to handle >>> deassign, rather than checking everywhere that it's not null, also >>> because in some cases (like the one in virtio-vsock), it's also >>> important that the transport is the same. >> >> My vote would be to apply your virtio_transport_recv_pkt() patch *and* make >> it impossible-by-design to switch ->transport from non-NULL to NULL in >> vsock_assign_transport(). > > I don't know if that's enough, in this case the problem is that some > response packets are intended for a socket, where the transport has > changed. So whether it's null or assigned but different, it's still a > problem we have to handle. > > So making it impossible for the transport to be null, but allowing it > to be different (we can't prevent it from changing), doesn't solve the > problem for us, it only shifts it. Got it. I assumed this issue would be solved by `vsk->transport != &t->transport` in the critical place(s). (Note that BPF doesn't care if transport has changed; BPF just expects to have _a_ transport.) >> If I'm not mistaken, that would require rewriting vsock_assign_transport() >> so that a new transport is assigned only once fully initialized, otherwise >> keep the old one (still unhurt and functional) and return error. Because >> failing connect() should not change anything under the hood, right? >> > > Nope, connect should be able to change the transport. > > Because a user can do an initial connect() that requires a specific > transport, this one fails maybe because there's no peer with that cid. > Then the user can redo the connect() to a different cid that requires > a different transport. But the initial connect() failing does not change anything under the hood (transport should/could stay NULL). Then a successful re-connect assigns the transport (NULL -> non-NULL). And it's all good because all I wanted to avoid (because of BPF) was non-NULL -> NULL. Anyway, that's my possibly shallow understanding :)
