route: an issue caused by local and main table's merge

route: an issue caused by local and main table's merge

[Xin Long]

Hi, David A.

Mithil reported an issue, which can be reproduced by:

  # ip link  add dummy0 type dummy
  # ip link  set dummy0 up
  # ip route add to broadcast 192.168.122.1 dev dummy0 <--- broadcast
  # ip route add 192.168.122.1 dev dummy0   <--- unicast
  # ip route add 1.1.1.1 via 192.168.122.1  <--- [A]
  Error: Nexthop has invalid gateway.
  # ip rule  add from 2.2.2.2
  # ip route add 1.1.1.1 via 192.168.122.1  <--- [B]

cmd [A] failed , as in fib_check_nh_v4_gw():

    if (table)
            tbl = fib_get_table(net, table);

    if (tbl)
            err = fib_table_lookup_2(tbl, &fl4, &res,
                                   FIB_LOOKUP_IGNORE_LINKSTATE |
                                   FIB_LOOKUP_NOREF);

    if (res.type != RTN_UNICAST && res.type != RTN_LOCAL) { <--- [a]
            NL_SET_ERR_MSG(extack, "Nexthop has invalid gateway");
            goto out;  <--[a]
    }

It gets the route for '192.168.122.1' from the merged (main/local)
table, and the broadcast one returns, and it fails the check [a].

But the same cmd [B] will work after one rule is added, by which
main table and local table get separated, it gets the route from
the main table (the same table for this route), and the unicast
one returns, and it will pass the check [a].

Any idea on how to fix this, and keep it consistent before and
after a rule added?

Thanks.

Re: route: an issue caused by local and main table's merge

[David Ahern]

[ This got lost in the backlog ]

On 3/2/20 1:38 AM, Xin Long wrote:
> Hi, David A.
> 
> Mithil reported an issue, which can be reproduced by:
> 
>   # ip link  add dummy0 type dummy
>   # ip link  set dummy0 up
>   # ip route add to broadcast 192.168.122.1 dev dummy0 <--- broadcast
>   # ip route add 192.168.122.1 dev dummy0   <--- unicast
>   # ip route add 1.1.1.1 via 192.168.122.1  <--- [A]
>   Error: Nexthop has invalid gateway.
>   # ip rule  add from 2.2.2.2
>   # ip route add 1.1.1.1 via 192.168.122.1  <--- [B]
> 
> cmd [A] failed , as in fib_check_nh_v4_gw():
> 
>     if (table)
>             tbl = fib_get_table(net, table);
> 
>     if (tbl)
>             err = fib_table_lookup_2(tbl, &fl4, &res,
>                                    FIB_LOOKUP_IGNORE_LINKSTATE |
>                                    FIB_LOOKUP_NOREF);
> 
>     if (res.type != RTN_UNICAST && res.type != RTN_LOCAL) { <--- [a]
>             NL_SET_ERR_MSG(extack, "Nexthop has invalid gateway");
>             goto out;  <--[a]
>     }
> 
> It gets the route for '192.168.122.1' from the merged (main/local)
> table, and the broadcast one returns, and it fails the check [a].
> 
> But the same cmd [B] will work after one rule is added, by which
> main table and local table get separated, it gets the route from
> the main table (the same table for this route), and the unicast
> one returns, and it will pass the check [a].
> 
> Any idea on how to fix this, and keep it consistent before and
> after a rule added?
> 

I do not have any suggestions off the top of my head.

Adding Alex who as I recall did the table merge.

Re: route: an issue caused by local and main table's merge

[Alexander Duyck]

On Sun, Mar 8, 2020 at 7:31 PM David Ahern <dsahern@gmail.com> wrote:
>
> [ This got lost in the backlog ]
>
> On 3/2/20 1:38 AM, Xin Long wrote:
> > Hi, David A.
> >
> > Mithil reported an issue, which can be reproduced by:
> >
> >   # ip link  add dummy0 type dummy
> >   # ip link  set dummy0 up
> >   # ip route add to broadcast 192.168.122.1 dev dummy0 <--- broadcast
> >   # ip route add 192.168.122.1 dev dummy0   <--- unicast
> >   # ip route add 1.1.1.1 via 192.168.122.1  <--- [A]
> >   Error: Nexthop has invalid gateway.
> >   # ip rule  add from 2.2.2.2
> >   # ip route add 1.1.1.1 via 192.168.122.1  <--- [B]
> >
> > cmd [A] failed , as in fib_check_nh_v4_gw():
> >
> >     if (table)
> >             tbl = fib_get_table(net, table);
> >
> >     if (tbl)
> >             err = fib_table_lookup_2(tbl, &fl4, &res,
> >                                    FIB_LOOKUP_IGNORE_LINKSTATE |
> >                                    FIB_LOOKUP_NOREF);
> >
> >     if (res.type != RTN_UNICAST && res.type != RTN_LOCAL) { <--- [a]
> >             NL_SET_ERR_MSG(extack, "Nexthop has invalid gateway");
> >             goto out;  <--[a]
> >     }
> >
> > It gets the route for '192.168.122.1' from the merged (main/local)
> > table, and the broadcast one returns, and it fails the check [a].
> >
> > But the same cmd [B] will work after one rule is added, by which
> > main table and local table get separated, it gets the route from
> > the main table (the same table for this route), and the unicast
> > one returns, and it will pass the check [a].
> >
> > Any idea on how to fix this, and keep it consistent before and
> > after a rule added?
> >
>
> I do not have any suggestions off the top of my head.
>
> Adding Alex who as I recall did the table merge.

As far as the table merge it is undone as soon as any rules are added
or deleted. From that point on it will not re-merge the table. So if
you are using rules you are going to need to modify the rules first
and then add routes as this will guarantee the table is consistent as
any further rule modifications will not re-merge the table.

Also, is it really a valid configuration to have the same address
configured as both a broadcast and unicast address? I couldn't find
anything that said it wasn't, but at the same time I haven't found
anything saying it is an acceptable practice to configure an IP
address as both a broadcast and unicast destination. Everything I saw
seemed to imply that a subnet should be at least a /30 to guarantee a
pair of IPs and support for broadcast addresses with all 1's and 0 for
the host identifier. As such 192.168.122.1 would never really be a
valid broadcast address since it implies a /31 subnet mask.

- Alex

Re: route: an issue caused by local and main table's merge

[Guillaume Nault]

On Mon, Mar 09, 2020 at 08:53:53AM -0700, Alexander Duyck wrote:
> Also, is it really a valid configuration to have the same address
> configured as both a broadcast and unicast address? I couldn't find
> anything that said it wasn't, but at the same time I haven't found
> anything saying it is an acceptable practice to configure an IP
> address as both a broadcast and unicast destination. Everything I saw
> seemed to imply that a subnet should be at least a /30 to guarantee a
> pair of IPs and support for broadcast addresses with all 1's and 0 for
> the host identifier. As such 192.168.122.1 would never really be a
> valid broadcast address since it implies a /31 subnet mask.
> 
RFC 3031 explicitly allows /31 subnets for point to point links. Also,
I've seen such network configurations in production on Linux boxes.

Re: route: an issue caused by local and main table's merge

[Guillaume Nault]

On Tue, Mar 10, 2020 at 04:56:32PM +0100, Guillaume Nault wrote:
> On Mon, Mar 09, 2020 at 08:53:53AM -0700, Alexander Duyck wrote:
> > Also, is it really a valid configuration to have the same address
> > configured as both a broadcast and unicast address? I couldn't find
> > anything that said it wasn't, but at the same time I haven't found
> > anything saying it is an acceptable practice to configure an IP
> > address as both a broadcast and unicast destination. Everything I saw
> > seemed to imply that a subnet should be at least a /30 to guarantee a
> > pair of IPs and support for broadcast addresses with all 1's and 0 for
> > the host identifier. As such 192.168.122.1 would never really be a
> > valid broadcast address since it implies a /31 subnet mask.
> > 
> RFC 3031 explicitly allows /31 subnets for point to point links.
That RFC 3021, sorry :/

Re: route: an issue caused by local and main table's merge

[Alexander Duyck]

On Tue, Mar 10, 2020 at 9:01 AM Guillaume Nault <gnault@redhat.com> wrote:
>
> On Tue, Mar 10, 2020 at 04:56:32PM +0100, Guillaume Nault wrote:
> > On Mon, Mar 09, 2020 at 08:53:53AM -0700, Alexander Duyck wrote:
> > > Also, is it really a valid configuration to have the same address
> > > configured as both a broadcast and unicast address? I couldn't find
> > > anything that said it wasn't, but at the same time I haven't found
> > > anything saying it is an acceptable practice to configure an IP
> > > address as both a broadcast and unicast destination. Everything I saw
> > > seemed to imply that a subnet should be at least a /30 to guarantee a
> > > pair of IPs and support for broadcast addresses with all 1's and 0 for
> > > the host identifier. As such 192.168.122.1 would never really be a
> > > valid broadcast address since it implies a /31 subnet mask.
> > >
> > RFC 3031 explicitly allows /31 subnets for point to point links.
> That RFC 3021, sorry :/
>

So from what I can tell the configuration as provided doesn't apply to
RFC 3021. Specifically RFC 3021 calls out that you are not supposed to
use the { <network-prefix>, -1 } which is what is being done here. In
addition the prefix is technically a /24 as configured here since a
prefix length wasn't specified so it defaults to a class C.

Looking over the Linux kernel code it normally doesn't add such a
broadcast if using a /31 address:
https://elixir.bootlin.com/linux/v5.6-rc5/source/net/ipv4/fib_frontend.c#L1122

- Alex

Re: route: an issue caused by local and main table's merge

[Guillaume Nault]

On Tue, Mar 10, 2020 at 10:19:24AM -0700, Alexander Duyck wrote:
> On Tue, Mar 10, 2020 at 9:01 AM Guillaume Nault <gnault@redhat.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 04:56:32PM +0100, Guillaume Nault wrote:
> > > On Mon, Mar 09, 2020 at 08:53:53AM -0700, Alexander Duyck wrote:
> > > > Also, is it really a valid configuration to have the same address
> > > > configured as both a broadcast and unicast address? I couldn't find
> > > > anything that said it wasn't, but at the same time I haven't found
> > > > anything saying it is an acceptable practice to configure an IP
> > > > address as both a broadcast and unicast destination. Everything I saw
> > > > seemed to imply that a subnet should be at least a /30 to guarantee a
> > > > pair of IPs and support for broadcast addresses with all 1's and 0 for
> > > > the host identifier. As such 192.168.122.1 would never really be a
> > > > valid broadcast address since it implies a /31 subnet mask.
> > > >
> > > RFC 3031 explicitly allows /31 subnets for point to point links.
> > That RFC 3021, sorry :/
> >
> 
> So from what I can tell the configuration as provided doesn't apply to
> RFC 3021. Specifically RFC 3021 calls out that you are not supposed to
> use the { <network-prefix>, -1 } which is what is being done here. In
> addition the prefix is technically a /24 as configured here since a
> prefix length wasn't specified so it defaults to a class C.
> 
Yes, I was just replying on the use of /31 subnets. I agree that this
case is different.

> Looking over the Linux kernel code it normally doesn't add such a
> broadcast if using a /31 address:
> https://elixir.bootlin.com/linux/v5.6-rc5/source/net/ipv4/fib_frontend.c#L1122
> 
Yes, and that's the right thing to do IMHO.

I think the original problem is that the command is accepted when it's
run after "ip rule add from 2.2.2.2". It should continue to be rejected
instead, as the ip-rule command has no action and is not supposed to
interfere in this case.