Address @markj comment, remove redundant *m0 = m assignment

Address @markj comment

LGTM

This revision is ready to review.
kind ping :D
@glebius @markj @melifaro

In D58294#1337514, @ivy wrote:

with D58305 + the new version of D58294, everything seems to be working fine, including with BIRD. (i only did cursory testing, though.)

This should fix it.
Could you please test it again? @ivy @zarychtam_plan-b.pwste.edu.pl

In contrast to ifa_ifwithnet the ifaof_ifpforaddr returns an ifa even if it's not appropriate for prefsrc.
Fix when the interface is specified (which is usually done by ospf) by removing ifaof_ifpforaddr and use ifa_ifwithaddr_fib instead.
Parent: D58305

In D58294#1337455, @ivy wrote:

i've done some basic testing with this. IPv4 seems to work with both route(8) and BIRD. IPv6 works with route(8) but not with BIRD:

protocol static {
        ipv6;

        route 2001:db8::/32 via 2a00:1098:6b:401::11 {
                krt_prefsrc = 2001:db8::10;
        };
}

Idx   Type         IFA                           Gateway                        Flags      Use Mtu               Netif           Addrif Refcnt Prepend
10           v6/gw fe80::5a9c:fcff:fe09:c9fd%vtnet0 2a00:1098:6b:401::11       G             0   1500           vtnet0                     2

setting krt_prefsrc in the kernel protocol doesn't make a difference. 2001:db8::10/128 is assigned to lo0 on the test system.

@bnovkov Should I wait for you to test, or are we good to land this?

Tests are passed.
Refactor by breaking prefsrc netlink logic into nl_set_nexthop_prefsrc() and setting the forgotten nh_aifp.
Now, I will wait for D58305, unless the consensus is to NOT limit the ability to set a prefsrc from another fib for VRF route leak.

Set prefsrc after setting the nh_ifp in non-multipath route.

Revert removed ARGUSED comments on other functions.
Address @bnovkov comment by adding ifa_ifwithaddr_fib_check() version.

Remove ARGUSED comment.

In D58281#1336902, @bnovkov wrote:

In D58281#1336697, @glebius wrote:

Instead of calling fib4_lookup() that has a different purpose, we need ifa_ifwithaddr() version that has fibnum argument.

Looking through net/if.c I noticed that ifa_ifwithnet seems to match what you're suggesting here, do you think we can use this function instead of adding a fibnum-aware version of `ifa_ifwithaddr()`?

Remove dump_prefsrc from mpnh.

Sorry for noise. I'm really used to arc.

Fix bird by adding dump_prefsrc.
@ivy, it works now:

[root@ftsr1] [~] # fgrep -A4 -B4 prefsrc /usr/local/etc/bird.conf
protocol kernel kernel6 {
  ipv6 {
    import all;
    export filter {
      krt_prefsrc = fdb5:c59b:114e::a;
      accept;
    };
  };
}
--
protocol kernel kernel4 {
  ipv4 {
    import all;
    export filter {
      krt_prefsrc = 172.23.1.101;
      accept;
    };
  };
}
[root@ftsr1] [~] # ifconfig lo0 inet
lo0: flags=1008049<UP,LOOPBACK,RUNNING,MULTICAST,LOWER_UP> metric 0 mtu 16384
options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
inet 127.0.0.1 netmask 0xff000000
inet 172.23.1.101 netmask 0xffffffff
[root@ftsr1] [~] # ifconfig vtnet0 inet6 | grep fdb
inet6 fdb5:c59b:114e::a prefixlen 64
[root@ftsr1] [~] # birdc show ospf neigh
BIRD 3.2.0 ready.
ospf1:
Router ID       Pri          State      DTime   Interface  Router IP
100.64.10.101     1     Full/DR         30.313  epair0a    fe80::5a9c:fcff:fe10:1020

Oops, these days I'm trying to use git arc instead of arc and messed up :)

Remove NL_DEBUG

Remove NL_DEBUG

This is a WIP. I'm not running tests for it.
@ivy, how it looks?

In D58287#1336899, @ivy wrote:

In D58287#1336891, @pouria wrote:

I'd appreciate if you could test this patch in your lab and see if it does really affect it or not.

i applied your patch on my development VM and tested using tun(4) in lieu of PPPoE, and confirmed this does not allow my existing configuration:

[13!] tansy ~# ifconfig epair create
epair0a
[14!] tansy ~# ifconfig tun create
tun0
[15!] tansy ~# ifconfig epair0a inet 10.0.0.1/32
[16!] tansy ~# ifconfig tun0 inet 10.0.0.1/32 10.0.0.2
ifconfig: ioctl (SIOCAIFADDR): File exists

(unsurprisingly, since this is the intent of the patch, so i just verified that the patch does what it says.)

Thank you so much for your explanation @ivy

In D58287#1336895, @ivy wrote:

In D58287#1336894, @pouria wrote:

Oh, so my original thought was correct, we don't need the ip address on the loopback, we only need its route to the loopback:

[...]
vtnet0: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
options=880028<VLAN_MTU,JUMBO_MTU,LINKSTATE,HWSTATS>
inet 172.23.1.101 netmask 0xffffffff broadcast 172.23.1.101

well, you have here an IPv4 address on vtnet0, so i expect this would work and source address is not 0.0.0.0. the case that doesn't work is when there is no IPv4 address on the outgoing interface.

but yes, you are correct that the IP address doesn't have to be configured on the loopback. i prefer to do this, because:

• the address is always available
• lo0 can be configured as a stub interface in OSPF to distribute a type 1 LSA for the address

even if i remove the IP address from the loopback, this doesn't solve the problem, because i need to assign the /32 to other interfaces such as Ethernet interface, wg(4) VPN interface, etc.

In D58287#1336892, @ivy wrote:

In D58287#1336891, @pouria wrote:

With this patch, you're still able to do this.

okay, but you said:

We only going to disallow setting same /32 address on multiple interfaces.

so it seems like you intend to remove the ability to do this. have i misunderstood?

In D58287#1336871, @ivy wrote:

you have to have an IPv4 address assigned to every interface that sends IPv4 traffic. for a router, this means any interface that routes IPv4 traffic, because of ICMP error generation. an IPv4 address on the outbound interface is also required for locally-originated outbound connections that aren't bind(2)ed to a specific IP address (this applies to hosts as well as routers).

if you don't do this, the outgoing packets will have a source address of 0.0.0.0. having an IPv4 address on the loopback is not enough, it also needs to be assigned to the outgoing interface or the source selection algorithm won't use it.

in "traditional" IPv4 setups, this isn't an issue because you always have at least one link net assigned to every interface that participates in IPv4. however, modern networks are more likely to route IPv4 using IPv6 nexthops, in which case there's no need for an IPv4 address on the interface because the nexthop resolution uses IPv6 ND. but you still need to assign an address to the interface for the source selection algorithm to be happy, and in nearly all cases, you will want to use the loopback address for that.

that means a practical FreeBSD router configuration in a modern network will look something like this:

ifconfig_lo0_alias0="1.2.3.4/32"
ifconfig_ix0="inet 1.2.3.4/32"
ifconfig_ix0_ipv6="inet6 auto_linklocal"
ifconfig_ix1="inet 1.2.3.4/32"
ifconfig_ix1_ipv6="inet6 auto_linklocal"
ifconfig_ix2="inet 1.2.3.4/32"
ifconfig_ix2_ipv6="inet6 auto_linklocal"
[ ... etc ... ]

In D58287#1336725, @ivy wrote:

In D58287#1336724, @pouria wrote:

Assigning the same prefix (e.g. in your case /32) to two different interfaces in the same FIB shouldn't be allowed.

why not?

In D58287#1336793, @pouria wrote:

I'm confused how this patch might affect those BRASes. (I had those in my last ISP job for ADSL services. But I'm no longer working there.)
The loopback route is still here.
We only going to disallow setting same /32 address on multiple interfaces.
On BRAS, each user gets it's own tun and using same loopback address on multiple p2p ifps (tun) is still working since the in_addprefix() -> in_hasrtprefix() will take care of that.
For e.g:

tun0: flags=1008051<UP,POINTOPOINT,RUNNING,MULTICAST,LOWER_UP> metric 0 mtu 1500
options=4080000<LINKSTATE,MEXTPG>
inet 192.168.0.1 --> 192.168.0.2 netmask 0xffffff00
groups: tun
nd6 options=829<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL,STABLEADDR>
tun1: flags=1008051<UP,POINTOPOINT,RUNNING,MULTICAST,LOWER_UP> metric 0 mtu 1500
options=4080000<LINKSTATE,MEXTPG>
inet 192.168.0.1 --> 192.168.0.3 netmask 0xffffff00
groups: tun
nd6 options=829<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL,STABLEADDR>
[root@ftsr1] [~] # netstat -rn4
Routing tables

Internet:
Destination        Gateway            Flags         Netif Expire
default            172.23.1.254       UGS          vtnet0
127.0.0.1          link#3             UH              lo0
192.168.0.1        link#3             UHS             lo0
192.168.0.2        link#6             UH             tun0
192.168.0.3        link#7             UH             tun1

In D58287#1336725, @ivy wrote:

the same reason you put any address on a loopback interface: so it's available when the physical interface is down (or even not present, in case of virtual interfaces like netgraph). otherwise, as soon as the interface goes down, the address is removed from the IGP and you can't connect to the router.

In D58287#1336720, @ivy wrote:

i would rather not go with this approach as it will break my existing, functional network configuration:

Name          Mtu Network                  Address                                     Ipkts Ierrs Idrop       Opkts Oerrs  Coll
lo1             - 90.155.69.65/32          90.155.69.65                                66289     -     -           0     -     -
ng0             - 90.155.69.65/32          90.155.69.65                              1427173     -     -     1459675     -     -

we don't otherwise restrict assigning the same /32 to two interfaces; is there a reason it can't be allowed here?

In D58096#1334930, @zlei wrote:

@pouria @adrian Do you want me to commit it now ?

LGTM

I'll fix it in another commit if approved.
Thank you

Address @markj comments.
Thanks!

It helps, but not much. it might increase ifi->timer.tv_sec to PROBE_INTERVAL which is 60s.

Address @salvadore comments.
Thanks!

@adrian Hi, could you please MFC this one into stable/15?
I'm not sure how to apply this patch from phabricator while cherry-picking it from CURRENT..

Use const bool instead of bool, oops!

Rebase to parent

Address @glebius comment.
Thank you!

In D58096#1333064, @vinicius_ferrao.net.br wrote:

Yes. How is the procedure? I should open other Phabricator review or just attach a patch here?

It's really hard to MFC this change to stable/15.
Could you please prepare a patch for stable/15 too?
If you're willing to do that, I could talk to release manager.

Address @markj comment on code spacing between tests.

Address @glebius comment.

Address @glebius and @markj comments by using wlock.

In D58096#1332939, @vinicius_ferrao.net.br wrote:

Yes, please @pouria.

I've tested the new changes against CX-6 and CX-5, but only on x86_64. Everything is working so far. I've also run this patch in a stress test in the context of another project (a NFS over RDMA server, which is how I found out about this issue in first place) and it worked.

To test the redirect I've used 3 VMs, being a FreeBSD router plus two Linux clients. Both input and output worked fine from everyone.

Address @markj comments.
Thank you for all of your great reviews.

Do you want me to commit this one? @vinicius_ferrao.net.br