> -----Original Message----- > From: Simon Horman <[email protected]> > Sent: Tuesday, July 30, 2024 9:37 AM > To: Lobakin, Aleksander <[email protected]> > Cc: [email protected]; Nguyen, Anthony L > <[email protected]>; David S. Miller <[email protected]>; Eric > Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo > Abeni <[email protected]>; NEX SW NCIS OSDT ITP Upstreaming > <[email protected]>; [email protected]; linux- > [email protected] > Subject: Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while > performing a soft reset > > On Mon, Jul 29, 2024 at 10:54:50AM +0200, Alexander Lobakin wrote: > > From: Simon Horman <[email protected]> > > Date: Fri, 26 Jul 2024 17:09:54 +0100 > > > > > On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote: > > >> The second tagged commit introduced a UAF, as it removed restoring > > >> q_vector->vport pointers after reinitializating the structures. > > >> This is due to that all queue allocation functions are performed here > > >> with the new temporary vport structure and those functions rewrite > > >> the backpointers to the vport. Then, this new struct is freed and > > >> the pointers start leading to nowhere. > > > > [...] > > > > >> err_reset: > > >> - idpf_vport_queues_rel(new_vport); > > >> + idpf_send_add_queues_msg(vport, vport->num_txq, vport- > >num_complq, > > >> + vport->num_rxq, vport->num_bufq); > > >> + > > >> +err_open: > > >> + if (current_state == __IDPF_VPORT_UP) > > >> + idpf_vport_open(vport); > > > > > > Hi Alexander, > > > > > > Can the system end up in an odd state if this call to idpf_vport_open(), > > > or > > > the one above, fails. Likewise if the above call to > > > idpf_send_add_queues_msg() fails. > > > > Adding the queues with the parameters that were before changing them > > almost can't fail. But if any of these two fails, it really will be in > > an odd state... > > Thanks for the clarification, this is my concern. > > > Perhaps we need to do a more powerful reset then? Can we somehow tell > > the kernel that in fact our iface is down, so that the user could try > > to enable it manually once again? > > Anyway, feels like a separate series or patch to -next, what do you think? > > Yes, sure. I agree that this patch improves things, and more extreme > corner cases can be addressed separately. > > With the above in mind, I'm happy with this patch. > > Reviewed-by: Simon Horman <[email protected]> >
Tested-by: Krishneil Singh <[email protected]>
