Re: special case mpe(4) in in6_ifattach()
On Thu, Nov 10, 2022 at 04:13:33PM +, Klemens Nanni wrote: > On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote: > > So mpe(4) is a special device. It is a point-to-multipoint interface that > > does not do multicast. So setting IFF_MULTICAST on the interface is not > > correct but IPv6 depends on it because neighbor discovery. > > > > Now there is no neighbor discovery on mpe(4) the neighbors are handled via > > BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and > > with that BGP IPv6 L3VPN should work. > > > > It may be an idea to move the IFF_MULTCAST check down into the > > ifp->if_type switch but right now this is good enough. I wonder if we have > > other interfaces that need similar treatment (e.g. mgre). > > Good enough for now, I also have a few minues for nd6.c but prefer > simpler changes first, then the cleanup come later. Seeing this thread again, my OK was meant for the later "I think this is a better diff." mail, of course, just to be clear.
Re: special case mpe(4) in in6_ifattach()
On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote:
> So mpe(4) is a special device. It is a point-to-multipoint interface that
> does not do multicast. So setting IFF_MULTICAST on the interface is not
> correct but IPv6 depends on it because neighbor discovery.
>
> Now there is no neighbor discovery on mpe(4) the neighbors are handled via
> BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and
> with that BGP IPv6 L3VPN should work.
>
> It may be an idea to move the IFF_MULTCAST check down into the
> ifp->if_type switch but right now this is good enough. I wonder if we have
> other interfaces that need similar treatment (e.g. mgre).
Good enough for now, I also have a few minues for nd6.c but prefer
simpler changes first, then the cleanup come later.
OK kn
> --
> :wq Claudio
>
> Index: netinet6/in6_ifattach.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
> retrieving revision 1.119
> diff -u -p -r1.119 in6_ifattach.c
> --- netinet6/in6_ifattach.c 8 Sep 2022 10:22:07 - 1.119
> +++ netinet6/in6_ifattach.c 4 Nov 2022 12:52:36 -
> @@ -373,7 +373,8 @@ in6_ifattach(struct ifnet *ifp)
> if (ifp->if_mtu < IPV6_MMTU)
> return (EINVAL);
>
> - if ((ifp->if_flags & IFF_MULTICAST) == 0)
> + /* ND needs multicast but mpe(4) does neither multicast nor ND */
> + if ((ifp->if_flags & IFF_MULTICAST) == 0 && ifp->if_type != IFT_MPLS)
> return (EINVAL);
>
> /*
> @@ -389,10 +390,14 @@ in6_ifattach(struct ifnet *ifp)
> return (error);
> }
>
> - /* Interfaces that rely on strong a priori cryptographic binding of
> - * IP addresses are incompatible with automatically assigned llv6. */
> + /*
> + * Some interface don't need an automatically assigned link-local
> + * address either because it think it is special (wireguard) or
s/it think//
> + * because there is no need and no neighbor discovery (mpe).
s/because//
> + */
> switch (ifp->if_type) {
> case IFT_WIREGUARD:
> + case IFT_MPLS:
> return (0);
> }
>
>
Re: special case mpe(4) in in6_ifattach()
> On 10 Nov 2022, at 10:56 pm, Claudio Jeker wrote:
>
> On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote:
>> So mpe(4) is a special device. It is a point-to-multipoint interface that
>> does not do multicast. So setting IFF_MULTICAST on the interface is not
>> correct but IPv6 depends on it because neighbor discovery.
>>
>> Now there is no neighbor discovery on mpe(4) the neighbors are handled via
>> BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and
>> with that BGP IPv6 L3VPN should work.
>>
>> It may be an idea to move the IFF_MULTCAST check down into the
>> ifp->if_type switch but right now this is good enough. I wonder if we have
>> other interfaces that need similar treatment (e.g. mgre).
>
> I think this is a better diff.
> Only require the IFF_MULTCAST bit if nd6_need_cache() returns true.
> In other words full ND is running on the interface.
> Such tunnel interfaces will not get a link-local address automatically
> assigned anymore but such an address can still be set up by hand.
this makes sense to me. ok.
>
> --
> :wq Claudio
>
> Index: netinet6/in6_ifattach.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
> retrieving revision 1.119
> diff -u -p -r1.119 in6_ifattach.c
> --- netinet6/in6_ifattach.c 8 Sep 2022 10:22:07 - 1.119
> +++ netinet6/in6_ifattach.c 10 Nov 2022 12:18:09 -
> @@ -373,7 +373,7 @@ in6_ifattach(struct ifnet *ifp)
> if (ifp->if_mtu < IPV6_MMTU)
> return (EINVAL);
>
> - if ((ifp->if_flags & IFF_MULTICAST) == 0)
> + if (nd6_need_cache(ifp) && (ifp->if_flags & IFF_MULTICAST) == 0)
> return (EINVAL);
>
> /*
> @@ -389,12 +389,12 @@ in6_ifattach(struct ifnet *ifp)
> return (error);
> }
>
> - /* Interfaces that rely on strong a priori cryptographic binding of
> - * IP addresses are incompatible with automatically assigned llv6. */
> - switch (ifp->if_type) {
> - case IFT_WIREGUARD:
> + /*
> + * Only interfaces that need the ND cache should automatically
> + * assign a link local address.
> + */
> + if (!nd6_need_cache(ifp))
> return (0);
> - }
>
> /* Assign a link-local address, if there's none. */
> if (in6ifa_ifpforlinklocal(ifp, 0) == NULL) {
>
Re: special case mpe(4) in in6_ifattach()
On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote:
> So mpe(4) is a special device. It is a point-to-multipoint interface that
> does not do multicast. So setting IFF_MULTICAST on the interface is not
> correct but IPv6 depends on it because neighbor discovery.
>
> Now there is no neighbor discovery on mpe(4) the neighbors are handled via
> BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and
> with that BGP IPv6 L3VPN should work.
>
> It may be an idea to move the IFF_MULTCAST check down into the
> ifp->if_type switch but right now this is good enough. I wonder if we have
> other interfaces that need similar treatment (e.g. mgre).
I think this is a better diff.
Only require the IFF_MULTCAST bit if nd6_need_cache() returns true.
In other words full ND is running on the interface.
Such tunnel interfaces will not get a link-local address automatically
assigned anymore but such an address can still be set up by hand.
--
:wq Claudio
Index: netinet6/in6_ifattach.c
===
RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
retrieving revision 1.119
diff -u -p -r1.119 in6_ifattach.c
--- netinet6/in6_ifattach.c 8 Sep 2022 10:22:07 - 1.119
+++ netinet6/in6_ifattach.c 10 Nov 2022 12:18:09 -
@@ -373,7 +373,7 @@ in6_ifattach(struct ifnet *ifp)
if (ifp->if_mtu < IPV6_MMTU)
return (EINVAL);
- if ((ifp->if_flags & IFF_MULTICAST) == 0)
+ if (nd6_need_cache(ifp) && (ifp->if_flags & IFF_MULTICAST) == 0)
return (EINVAL);
/*
@@ -389,12 +389,12 @@ in6_ifattach(struct ifnet *ifp)
return (error);
}
- /* Interfaces that rely on strong a priori cryptographic binding of
-* IP addresses are incompatible with automatically assigned llv6. */
- switch (ifp->if_type) {
- case IFT_WIREGUARD:
+ /*
+* Only interfaces that need the ND cache should automatically
+* assign a link local address.
+*/
+ if (!nd6_need_cache(ifp))
return (0);
- }
/* Assign a link-local address, if there's none. */
if (in6ifa_ifpforlinklocal(ifp, 0) == NULL) {
Re: special case mpe(4) in in6_ifattach()
On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote:
> So mpe(4) is a special device. It is a point-to-multipoint interface that
> does not do multicast. So setting IFF_MULTICAST on the interface is not
> correct but IPv6 depends on it because neighbor discovery.
>
> Now there is no neighbor discovery on mpe(4) the neighbors are handled via
> BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and
> with that BGP IPv6 L3VPN should work.
this is the most interesting point here. as you said above, there's
an assumption in the v6 stack that to do any ipv6 on a (non loopback)
interface you're going to be able to run the neighbor discovery
protocol, and ND relies on multicast, hence the check.
however, i think at this point we have pretty good evidence that
this assumption is wrong because of point to multipoint interfaces.
> It may be an idea to move the IFF_MULTCAST check down into the
> ifp->if_type switch but right now this is good enough. I wonder if we have
> other interfaces that need similar treatment (e.g. mgre).
mgre does set the IFF_MULTICAST for this exact reason, and only this
reason.
rather than keep growing the list of exceptional if_types, we should
have a chat about whether we can make a better decision based on
if_flags or if we could use a more generic if_type (eg, mgre used
IFT_L3IPVLAN instead of making up an IFT_MGRE).
> --
> :wq Claudio
>
> Index: netinet6/in6_ifattach.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
> retrieving revision 1.119
> diff -u -p -r1.119 in6_ifattach.c
> --- netinet6/in6_ifattach.c 8 Sep 2022 10:22:07 - 1.119
> +++ netinet6/in6_ifattach.c 4 Nov 2022 12:52:36 -
> @@ -373,7 +373,8 @@ in6_ifattach(struct ifnet *ifp)
> if (ifp->if_mtu < IPV6_MMTU)
> return (EINVAL);
>
> - if ((ifp->if_flags & IFF_MULTICAST) == 0)
> + /* ND needs multicast but mpe(4) does neither multicast nor ND */
> + if ((ifp->if_flags & IFF_MULTICAST) == 0 && ifp->if_type != IFT_MPLS)
> return (EINVAL);
>
> /*
> @@ -389,10 +390,14 @@ in6_ifattach(struct ifnet *ifp)
> return (error);
> }
>
> - /* Interfaces that rely on strong a priori cryptographic binding of
> - * IP addresses are incompatible with automatically assigned llv6. */
> + /*
> + * Some interface don't need an automatically assigned link-local
> + * address either because it think it is special (wireguard) or
> + * because there is no need and no neighbor discovery (mpe).
> + */
> switch (ifp->if_type) {
> case IFT_WIREGUARD:
> + case IFT_MPLS:
> return (0);
> }
>
>
