Agree, but changing the control flow of this function is really
problematic , it was even tried before if you remember commit
"e4103312d7b7a" , it got something to do with port allocation, I'll take
another look over the code to see what other options we have though.
Since in short, you are right it is racy now.
On 12/12/2022 15:43, Jason Gunthorpe wrote:
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
