[PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

Undefined reference to ip6_route_output and ip_route_output
was reported with CONFIG_INET=n and CONFIG_IPV6=n.

This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
to lookup nexthop device if user has not specified it
in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
depend on INET and (IPV6 || IPV6=n) because it
uses ip6_route_output and ip_route_output.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Reported-by: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---

v1 - v2: use IS_BUILTIN

v2 - v3: Use new Kconfig option that depends on (IPV6 || IPV6=n) as
	 suggested by Dave. Also uses IS_ERR as suggested by Thomas.

v3 - v4: Include missed case of (MPLS_ROUTING=y && IPV6=m) reported by
         Dave.

 net/mpls/Kconfig   |    8 ++++++++
 net/mpls/af_mpls.c |   19 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
index 5c467ef..134764e 100644
--- a/net/mpls/Kconfig
+++ b/net/mpls/Kconfig
@@ -33,4 +33,12 @@ config MPLS_IPTUNNEL
 	---help---
 	 mpls ip tunnel support.
 
+config MPLS_NEXTHOP_DEVLOOKUP
+	bool "MPLS: nexthop oif dev lookup"
+	depends on MPLS_ROUTING && INET && \
+		((IPV6 && !(MPLS_ROUTING=y && IPV6=m)) || IPV6=n)
+	---help---
+	 This enables mpls route nexthop dev lookup when oif is not
+	 specified by user
+
 endif # MPLS
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 49f1b0e..3e0c0212d 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -331,6 +331,7 @@ static unsigned find_free_label(struct net *net)
 	return LABEL_NOT_SPECIFIED;
 }
 
+#if IS_ENABLED(CONFIG_MPLS_NEXTHOP_DEVLOOKUP)
 static struct net_device *inet_fib_lookup_dev(struct net *net, void *addr)
 {
 	struct net_device *dev = NULL;
@@ -350,7 +351,14 @@ static struct net_device *inet_fib_lookup_dev(struct net *net, void *addr)
 errout:
 	return dev;
 }
+#else
+static struct net_device *inet_fib_lookup_dev(struct net *net, void *addr)
+{
+	return ERR_PTR(-EAFNOSUPPORT);
+}
+#endif
 
+#if IS_ENABLED(CONFIG_MPLS_NEXTHOP_DEVLOOKUP) && IS_ENABLED(CONFIG_IPV6)
 static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
 {
 	struct net_device *dev = NULL;
@@ -371,6 +379,12 @@ errout:
 
 	return dev;
 }
+#else
+static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
+{
+	return ERR_PTR(-EAFNOSUPPORT);
+}
+#endif
 
 static struct net_device *find_outdev(struct net *net,
 				      struct mpls_route_config *cfg)
@@ -427,8 +441,11 @@ static int mpls_route_add(struct mpls_route_config *cfg)
 
 	err = -ENODEV;
 	dev = find_outdev(net, cfg);
-	if (!dev)
+	if (IS_ERR(dev)) {
+		err = PTR_ERR(dev);
+		dev = NULL;
 		goto errout;
+	}
 
 	/* Ensure this is a supported device */
 	err = -EINVAL;
-- 
1.7.10.4

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Hannes Frederic Sowa]

On Mon, 2015-07-27 at 23:40 -0700, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> Undefined reference to ip6_route_output and ip_route_output
> was reported with CONFIG_INET=n and CONFIG_IPV6=n.
> 
> This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
> to lookup nexthop device if user has not specified it
> in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
> depend on INET and (IPV6 || IPV6=n) because it
> uses ip6_route_output and ip_route_output.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Reported-by: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
> 
> v1 - v2: use IS_BUILTIN
> 
> v2 - v3: Use new Kconfig option that depends on (IPV6 || IPV6=n) as
> 	 suggested by Dave. Also uses IS_ERR as suggested by Thomas.
> 
> v3 - v4: Include missed case of (MPLS_ROUTING=y && IPV6=m) reported by
>          Dave.
> 
>  net/mpls/Kconfig   |    8 ++++++++
>  net/mpls/af_mpls.c |   19 ++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
> index 5c467ef..134764e 100644
> --- a/net/mpls/Kconfig
> +++ b/net/mpls/Kconfig
> @@ -33,4 +33,12 @@ config MPLS_IPTUNNEL
>  	---help---
>  	 mpls ip tunnel support.
>  
> +config MPLS_NEXTHOP_DEVLOOKUP
> +	bool "MPLS: nexthop oif dev lookup"
> +	depends on MPLS_ROUTING && INET && \
> +		((IPV6 && !(MPLS_ROUTING=y && IPV6=m)) || IPV6=n)
> +	---help---
> +	 This enables mpls route nexthop dev lookup when oif is not
> +	 specified by user
> +

Urks.

Can't you simply use ipv6_stub_impl.ipv6_dst_lookup with sk=NULL to do
that and don't have a run-time dependency on IPv6 at all (for the cost
of a function pointer). Maybe same for IPv4?

If builtin you can inline those calls anyway.

Bye,
Hannes

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[roopa]

On 7/28/15, 6:04 AM, Hannes Frederic Sowa wrote:
> On Mon, 2015-07-27 at 23:40 -0700, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Undefined reference to ip6_route_output and ip_route_output
>> was reported with CONFIG_INET=n and CONFIG_IPV6=n.
>>
>> This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>> to lookup nexthop device if user has not specified it
>> in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>> depend on INET and (IPV6 || IPV6=n) because it
>> uses ip6_route_output and ip_route_output.
>>
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Reported-by: Thomas Graf <tgraf@suug.ch>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>>
>> v1 - v2: use IS_BUILTIN
>>
>> v2 - v3: Use new Kconfig option that depends on (IPV6 || IPV6=n) as
>> 	 suggested by Dave. Also uses IS_ERR as suggested by Thomas.
>>
>> v3 - v4: Include missed case of (MPLS_ROUTING=y && IPV6=m) reported by
>>           Dave.
>>
>>   net/mpls/Kconfig   |    8 ++++++++
>>   net/mpls/af_mpls.c |   19 ++++++++++++++++++-
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
>> index 5c467ef..134764e 100644
>> --- a/net/mpls/Kconfig
>> +++ b/net/mpls/Kconfig
>> @@ -33,4 +33,12 @@ config MPLS_IPTUNNEL
>>   	---help---
>>   	 mpls ip tunnel support.
>>   
>> +config MPLS_NEXTHOP_DEVLOOKUP
>> +	bool "MPLS: nexthop oif dev lookup"
>> +	depends on MPLS_ROUTING && INET && \
>> +		((IPV6 && !(MPLS_ROUTING=y && IPV6=m)) || IPV6=n)
>> +	---help---
>> +	 This enables mpls route nexthop dev lookup when oif is not
>> +	 specified by user
>> +
> Urks.
>
> Can't you simply use ipv6_stub_impl.ipv6_dst_lookup with sk=NULL to do
> that and don't have a run-time dependency on IPv6 at all (for the cost
> of a function pointer).
I did not realize that this could be an option. I now see vxlan using it.
I will try it out.

> Maybe same for IPv4?
I would prefer leaving IPV4 alone with CONFIG_INET. IPV6 was my problem 
case.
Let me see if i can fix that first without introducing a config option.

Thanks,
Roopa


>

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[roopa]

On 7/28/15, 6:04 AM, Hannes Frederic Sowa wrote:
> Can't you simply use ipv6_stub_impl.ipv6_dst_lookup with sk=NULL to do
> that and don't have a run-time dependency on IPv6 at all (for the cost
> of a function pointer).
  ipv6_stub_impl.ipv6_dst_lookup seems to require sk today.
But it only needs it to get 'net' in the beginning and sk is optional 
afterwards.
I will submit a patch to add 'net' as an arg  to ipv6_dst_lookup.
Users of ipv6_dst_lookup are few and that seems like an easy change and 
helps my patch.
If you or others think otherwise, pls let me know.

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Hannes Frederic Sowa]

Hi roopa,

On Tue, Jul 28, 2015, at 21:28, roopa wrote:
> On 7/28/15, 6:04 AM, Hannes Frederic Sowa wrote:
> > Can't you simply use ipv6_stub_impl.ipv6_dst_lookup with sk=NULL to do
> > that and don't have a run-time dependency on IPv6 at all (for the cost
> > of a function pointer).
>   ipv6_stub_impl.ipv6_dst_lookup seems to require sk today.
> But it only needs it to get 'net' in the beginning and sk is optional 
> afterwards.
> I will submit a patch to add 'net' as an arg  to ipv6_dst_lookup.
> Users of ipv6_dst_lookup are few and that seems like an easy change and 
> helps my patch.
> If you or others think otherwise, pls let me know.

No need to extend this function at any cost. Simply add your own
function pointer to the struct if needed.

Probably you have to move the ipv6_stub = &ipv6_stub_impl;
initialization in inet6_init down so you don't expose the function
pointer too early and thus it races with initialization (and error
handling seems to be incorrect in this function, too).

Thanks,
Hannes

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[roopa]

On 7/28/15, 3:22 PM, Hannes Frederic Sowa wrote:
> Hi roopa,
>
> On Tue, Jul 28, 2015, at 21:28, roopa wrote:
>>    ipv6_stub_impl.ipv6_dst_lookup seems to require sk today.
>> But it only needs it to get 'net' in the beginning and sk is optional
>> afterwards.
>> I will submit a patch to add 'net' as an arg  to ipv6_dst_lookup.
>> Users of ipv6_dst_lookup are few and that seems like an easy change and
>> helps my patch.
>> If you or others think otherwise, pls let me know.
> No need to extend this function at any cost. Simply add your own
> function pointer to the struct if needed.

saw your this email after I hit send on the series. Since the new 
function pointer will be exactly similar to ipv6_dst_lookup
with just an additional argument, a new function pointer does not seem 
necessary. But i can certainly change it
to a new function pointer and resend if that is more acceptable.
>
> Probably you have to move the ipv6_stub = &ipv6_stub_impl;
> initialization in inet6_init down so you don't expose the function
> pointer too early and thus it races with initialization (and error
> handling seems to be incorrect in this function, too).
>
ok, will look.

thanks,
Roopa

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Robert Shearman]

On 28/07/15 07:40, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Undefined reference to ip6_route_output and ip_route_output
> was reported with CONFIG_INET=n and CONFIG_IPV6=n.
>
> This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
> to lookup nexthop device if user has not specified it
> in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
> depend on INET and (IPV6 || IPV6=n) because it
> uses ip6_route_output and ip_route_output.
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Reported-by: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Is there a compelling reason to allow the user/applications to not 
specify the output interface and to derive it from the nexthop? If the 
user/application intends to treat this as a recursive route then it has 
to make sure to trigger route updates to the kernel anyway, and an 
application should have the output interface and real nexthop close to 
hand in that case.

If there isn't a compelling reason, then perhaps the best course of 
action is to revert the commit, instead of introducing a level of config 
complexity that means that users/applications may not be able to rely on 
this capability anyway?

Thanks,
Rob

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[roopa]

On 7/28/15, 7:17 AM, Robert Shearman wrote:
> On 28/07/15 07:40, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Undefined reference to ip6_route_output and ip_route_output
>> was reported with CONFIG_INET=n and CONFIG_IPV6=n.
>>
>> This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>> to lookup nexthop device if user has not specified it
>> in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>> depend on INET and (IPV6 || IPV6=n) because it
>> uses ip6_route_output and ip_route_output.
>>
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Reported-by: Thomas Graf <tgraf@suug.ch>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Is there a compelling reason to allow the user/applications to not 
> specify the output interface and to derive it from the nexthop? If the 
> user/application intends to treat this as a recursive route then it 
> has to make sure to trigger route updates to the kernel anyway, and an 
> application should have the output interface and real nexthop close to 
> hand in that case.

RTA_OIF is optional for ipv4 and ipv6 routes and we wanted to keep it 
that way for mpls routes as well (Quagga is the application in our use 
case).
It was a simple patch...until i realized the IPV6 dependency issues (I 
will sure remember this next time).

>
> If there isn't a compelling reason, then perhaps the best course of 
> action is to revert the commit, instead of introducing a level of 
> config complexity that means that users/applications may not be able 
> to rely on this capability anyway?
The config option though looks complex should not introduce any 
complexity for the user. It is on by default and always on for the 
default case.
Only for the cases where the IPV6 is a loaded as a module and 
MPLS_ROUTING is not, the app may get family not supported errors.
I did suggest a revert the first time. Mainly for me to fix the mistake 
i made and resubmit after proper IPV6 dependency testing.

I am in the process of trying the option that hannes suggested.

Thanks,
Roopa

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Robert Shearman]

On 28/07/15 17:16, roopa wrote:
> On 7/28/15, 7:17 AM, Robert Shearman wrote:
>> On 28/07/15 07:40, Roopa Prabhu wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> Undefined reference to ip6_route_output and ip_route_output
>>> was reported with CONFIG_INET=n and CONFIG_IPV6=n.
>>>
>>> This patch adds new CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>>> to lookup nexthop device if user has not specified it
>>> in RTA_OIF attribute. Make CONFIG_MPLS_NEXTHOP_DEVLOOKUP
>>> depend on INET and (IPV6 || IPV6=n) because it
>>> uses ip6_route_output and ip_route_output.
>>>
>>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>>> Reported-by: Thomas Graf <tgraf@suug.ch>
>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Is there a compelling reason to allow the user/applications to not
>> specify the output interface and to derive it from the nexthop? If the
>> user/application intends to treat this as a recursive route then it
>> has to make sure to trigger route updates to the kernel anyway, and an
>> application should have the output interface and real nexthop close to
>> hand in that case.
>
> RTA_OIF is optional for ipv4 and ipv6 routes and we wanted to keep it
> that way for mpls routes as well (Quagga is the application in our use
> case).
> It was a simple patch...until i realized the IPV6 dependency issues (I
> will sure remember this next time).

Having read the code, I realise the nexthop isn't derived from the 
lookup. Given that this can only work for the case where a path is 
recursive via a connected nexthop, it seems to be of limited use.

I'm not familiar with the Quagga code, but is it worth adding this 
additional complexity to the kernel rather than making a change to 
Quagga instead, where presumably it already has code to derive the 
output interface in the case of having a recursive route via a 
non-connected nexthop?

>
>>
>> If there isn't a compelling reason, then perhaps the best course of
>> action is to revert the commit, instead of introducing a level of
>> config complexity that means that users/applications may not be able
>> to rely on this capability anyway?
> The config option though looks complex should not introduce any
> complexity for the user. It is on by default and always on for the
> default case.
> Only for the cases where the IPV6 is a loaded as a module and
> MPLS_ROUTING is not, the app may get family not supported errors.
> I did suggest a revert the first time. Mainly for me to fix the mistake
> i made and resubmit after proper IPV6 dependency testing.
>
> I am in the process of trying the option that hannes suggested.

If we must keep this functionality then that approach looks good to me.

Thanks,
Rob

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Thomas Graf]

On 07/29/15 at 11:38am, Robert Shearman wrote:
> On 28/07/15 17:16, roopa wrote:
> >RTA_OIF is optional for ipv4 and ipv6 routes and we wanted to keep it
> >that way for mpls routes as well (Quagga is the application in our use
> >case).
> >It was a simple patch...until i realized the IPV6 dependency issues (I
> >will sure remember this next time).
> 
> Having read the code, I realise the nexthop isn't derived from the lookup.
> Given that this can only work for the case where a path is recursive via a
> connected nexthop, it seems to be of limited use.
> 
> I'm not familiar with the Quagga code, but is it worth adding this
> additional complexity to the kernel rather than making a change to Quagga
> instead, where presumably it already has code to derive the output interface
> in the case of having a recursive route via a non-connected nexthop?

I think it's wrong to assume that it's always a single management
application that manages both parts of the route. At least for
underlays and overlays it is fairly common to run something like
Quagga to manage the underlay and use multiple other orchestration
tools on top to create virtual networks which should not be aware of
any underlay specifics.

Re: [PATCH net-next v4] af_mpls: fix undefined reference to ip6_route_output

[Robert Shearman]

On 29/07/15 11:51, Thomas Graf wrote:
> On 07/29/15 at 11:38am, Robert Shearman wrote:
>> On 28/07/15 17:16, roopa wrote:
>>> RTA_OIF is optional for ipv4 and ipv6 routes and we wanted to keep it
>>> that way for mpls routes as well (Quagga is the application in our use
>>> case).
>>> It was a simple patch...until i realized the IPV6 dependency issues (I
>>> will sure remember this next time).
>>
>> Having read the code, I realise the nexthop isn't derived from the lookup.
>> Given that this can only work for the case where a path is recursive via a
>> connected nexthop, it seems to be of limited use.
>>
>> I'm not familiar with the Quagga code, but is it worth adding this
>> additional complexity to the kernel rather than making a change to Quagga
>> instead, where presumably it already has code to derive the output interface
>> in the case of having a recursive route via a non-connected nexthop?
>
> I think it's wrong to assume that it's always a single management
> application that manages both parts of the route. At least for
> underlays and overlays it is fairly common to run something like
> Quagga to manage the underlay and use multiple other orchestration
> tools on top to create virtual networks which should not be aware of
> any underlay specifics.

I agree, but this kernel mechanism doesn't serve that purpose in the 
general case - it only works for a connected nexthop. In other cases, 
the application creating the overlay network (assuming it's not using 
tunnels) will need to the recursive route resolution itself.

I'd like to see recursive route resolution including responding to 
changes in the via route being done in the kernel for other reasons too, 
but IMHO just the derivation of the output interface for a connected 
nexthop on the initial route add doesn't offer much benefit.

Thanks,
Rob