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.
and what I'm trying to say, is that once the CM is added the tree(aka
passed resolve addr once + resolve route) , there can't be a
good(success) case for the resolve_prepare_src again, since it is
already bound so every consecutive call should fail, meaning the
cma_dst_addr is technically frozen.
ie completely block resolve_prepare_src()
Most probably this suggests that the id is being inserted into the
rbtree at the wrong time, before the dst_add becomes unchangable.
Jason
