On Mon, Dec 12, 2022 at 03:42:07PM +0200, Patrisious Haddad wrote: > I think we have the same realization but different understanding of the > code, please correct what I'm missing, rest inline: > > On 12/12/2022 15:27, Jason Gunthorpe wrote: > > On Sun, Dec 11, 2022 at 11:08:30AM +0200, Leon Romanovsky wrote: > > > From: Patrisious Haddad <phaddad@xxxxxxxxxx> > > > > > > resolve_prepare_src() changes the destination address of the id, > > > regardless of success, and on failure zeroes it out. > > > > > > Instead on function failure keep the original destination address > > > of the id. > > > > > > Since the id could have been already added to the cm id tree and > > > zeroing its destination address, could result in a key mismatch or > > > multiple ids having the same key(zero) in the tree which could lead to: > > > > Oh, this can't be right > > > > The destination address is variable and it is changed by resolve even > > in good cases. > This is what I don't think can happen, since one address is resolved(bound), > it can't be bound again so each an other try of resolve would fail and enter > the error flow which I just fixed. > > > > So this part of the rb search is nonsense: > > > > result = compare_netdev_and_ip( > > node_id_priv->id.route.addr.dev_addr.bound_dev_if, > > cma_dst_addr(node_id_priv), this); > > > > The only way to fix it is to freeze the dst_addr before inserting > > things into the rb tree. > I completely agree, and this was my assumption that after resolve address, > and resolve route(where I add to the tree), the dst_addr is frozen, the only > scenario where it isn't was the resolve_prepare_src failure which some why > nullified the value instead of keeping the original. Then fix the control flow so it doesn't do the nullification if it didn't change the value You can't just change it while it is in the rb tree, that is racy Jason
