__ni_rtnl_parse_newaddr: Use peer-to-peer condition from comment instead of relying on interface's p2p flag

[boomer41]

An ethernet interface is not a peer-to-peer interface. However, we may bind peer to peer addresses on it, which won't get removed with the current code (ni_nl_talk() in __ni_rtnl_send_deladdr() returns an error because ap->peer_addr stays empty for that p2p-address).

Instead, use the peer-to-peer condition from the comment below to check whether the interface address is P2P.

[mtomaschewski]

AFAIS, the outcome would be, that tunnel ipv6 link layer addresses would be stored in peer_addr instead in local_addr, isn't it? Hmm... This needs closer look / tests with several interface types.

[boomer41]

Unfortunately we don't have a v6 setup (yet) to test it. This bug currently only affects removal of peer-to-peer IPs from non-peer-to-peer interfaces. Verified only with v4.

[mtomaschewski]

I'll take a look / test it ASAP.

[mtomaschewski]

I'd not replace the ifflags & NI_IFF_POINT_TO_POINT by !tb[IFA_BROADCAST] which defaults to NULL (no brd in IPv6, even in IPv4 it's completely optional on brd interfaces) and rather adjust the else part, but it seems to be possible to unify this for brd+ptp as just:

        if (tb[IFA_LOCAL]) {
                __ni_nla_get_addr(ifa->ifa_family, &ap->local_addr, tb[IFA_LOCAL]);
                __ni_nla_get_addr(ifa->ifa_family, &ap->peer_addr, tb[IFA_ADDRESS]);
                if (ni_sockaddr_equal(&ap->local_addr, &ap->peer_addr))
                        memset(&ap->peer_addr, 0, sizeof(ap->peer_addr));
        } else {
                __ni_nla_get_addr(ifa->ifa_family, &ap->local_addr, tb[IFA_ADDRESS]);
        }

In IPv4 I'm getting:

# ip addr add 10.0.0.10/32 peer 10.0.0.1 dev eth4
::: eth4: newaddr(brd): family 2, prefixlen 32, scope 0, flags 128
::: eth4: newaddr[IFA_LOCAL]: 10.0.0.10
::: eth4: newaddr[IFA_ADDRESS]: 10.0.0.1
::: eth4: newaddr[IFA_BROADCAST]: NULL
::: eth4: newaddr[IFA_ANYCAST]: NULL
# ip addr add 10.0.0.11/24 dev eth4
::: eth4: newaddr(brd): family 2, prefixlen 24, scope 0, flags 128
::: eth4: newaddr[IFA_LOCAL]: 10.0.0.11
::: eth4: newaddr[IFA_ADDRESS]: 10.0.0.11
::: eth4: newaddr[IFA_BROADCAST]: NULL
::: eth4: newaddr[IFA_ANYCAST]: NULL
# ip addr add 10.0.0.12/32 dev eth4
::: eth4: newaddr(brd): family 2, prefixlen 32, scope 0, flags 128
::: eth4: newaddr[IFA_LOCAL]: 10.0.0.12
::: eth4: newaddr[IFA_ADDRESS]: 10.0.0.12
::: eth4: newaddr[IFA_BROADCAST]: NULL
::: eth4: newaddr[IFA_ANYCAST]: NULL
# ip addr add 10.0.0.14/24 brd + dev eth4
::: eth4: newaddr(brd): family 2, prefixlen 24, scope 0, flags 128
::: eth4: newaddr[IFA_LOCAL]: 10.0.0.14
::: eth4: newaddr[IFA_ADDRESS]: 10.0.0.14
::: eth4: newaddr[IFA_BROADCAST]: 10.0.0.255
::: eth4: newaddr[IFA_ANYCAST]: NULL

In IPv6:

# ip addr add 2001:db8::10/128 peer 2001:db8::1 dev eth4
::: eth4: newaddr(brd): family 10, prefixlen 128, scope 0, flags 192
::: eth4: newaddr[IFA_LOCAL]: 2001:db8::10
::: eth4: newaddr[IFA_ADDRESS]: 2001:db8::1
::: eth4: newaddr[IFA_BROADCAST]: NULL
::: eth4: newaddr[IFA_ANYCAST]: NULL
# ip addr add 2001:db8::11/64 dev eth4
::: eth4: newaddr(brd): family 10, prefixlen 64, scope 0, flags 192
::: eth4: newaddr[IFA_LOCAL]: NULL
::: eth4: newaddr[IFA_ADDRESS]: 2001:db8::11
::: eth4: newaddr[IFA_BROADCAST]: NULL
::: eth4: newaddr[IFA_ANYCAST]: NULL

and the IFA_LOCAL/ADDRESS combinations work also for an gre ptp interface. (You can start testing/rtnl-test that enabled debug3 and shows the attributes).

See commit https://github.com/mtomaschewski/wicked/commit/b031bceb5fd2843d60ed40b91df7d84dbfb5aaf5 in https://github.com/mtomaschewski/wicked/commits/rtnl-newaddr-parse branch adding also ifname to the log messages.

@boomer41, may you retest that this work for you?

[boomer41]

may you retest that this work for you?

@mtomaschewski Just tested with mtomaschewski@b031bceb. All IP addresses (whether non-p2p or p2p) can be added and removed on ifup/down. So it seems your changes work!

Feel free to close this PR when you merged your branch in :)

[mtomaschewski]

See https://github.com/openSUSE/wicked/pull/988

The IFA_BROADCAST condition is not needed, __ni_nla_get_addr already contains it, thus:

-   if (tb[IFA_BROADCAST])
-       __ni_nla_get_addr(ifa->ifa_family, &ap->bcast_addr, tb[IFA_BROADCAST]);
+   __ni_nla_get_addr(ifa->ifa_family, &ap->bcast_addr, tb[IFA_BROADCAST]);

[cfconrad]

Thx for your contribution, as mentioned in comment https://github.com/openSUSE/wicked/pull/987#issuecomment-1764651046 we merged the successor PR https://github.com/openSUSE/wicked/pull/988