On Thu, 19 Dec 2024 at 16:05, Michal Luczaj <mhal@xxxxxxx> wrote: > > 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). Nope, isn't null, it's assigned to a transport, because for example it has to send a packet to connect to the remote CID and wait back for a response that for example says the CID doesn't exist. > 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 :) >
