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

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

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

seen with CONFIG_IPV6 disabled. Wrap the code
around IS_ENABLED(CONFIG_IPV6)

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/mpls/af_mpls.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 49f1b0e..040dfff 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -351,6 +351,7 @@ errout:
 	return dev;
 }
 
+#if IS_ENABLED(CONFIG_IPV6)
 static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
 {
 	struct net_device *dev = NULL;
@@ -371,6 +372,12 @@ errout:
 
 	return dev;
 }
+#else
+static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
+{
+	return NULL;
+}
+#endif
 
 static struct net_device *find_outdev(struct net *net,
 				      struct mpls_route_config *cfg)
-- 
1.7.10.4

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

[Thomas Graf]

On 07/22/15 at 12:10am, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> seen with CONFIG_IPV6 disabled. Wrap the code
> around IS_ENABLED(CONFIG_IPV6)
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

We need the same for CONFIG_INET=n in inet_fib_lookup_dev:

/home/tgraf/dev/linux/net-next/include/net/route.h:122: undefined reference to `ip_route_output_flow'

Is it worth returning EAFNOSUPPORT instead of ENODEV in these
cases?  Something like this:

>  }
> +#else
> +static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
> +{
 +	return ERR_PTR(-EAFNOSUPPORT);
> +}
> +#endif

And then IS_ERR() in mpls_route_add()?

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

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 22 Jul 2015 14:23:09 +0200

> On 07/22/15 at 12:10am, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>> 
>> seen with CONFIG_IPV6 disabled. Wrap the code
>> around IS_ENABLED(CONFIG_IPV6)
>> 
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> We need the same for CONFIG_INET=n in inet_fib_lookup_dev:
> 
> /home/tgraf/dev/linux/net-next/include/net/route.h:122: undefined reference to `ip_route_output_flow'
> 
> Is it worth returning EAFNOSUPPORT instead of ENODEV in these
> cases?  Something like this:
> 
>>  }
>> +#else
>> +static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
>> +{
>  +	return ERR_PTR(-EAFNOSUPPORT);
>> +}
>> +#endif
> 
> And then IS_ERR() in mpls_route_add()?

Also IS_ENABLED() _DOES NOT_ work, what if MPLS is 'y' and IPV6 is 'm'?

You can't reache the modular symbol from statically built code.

What a mess.

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

[roopa]

On 7/22/15, 10:49 AM, David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 22 Jul 2015 14:23:09 +0200
>
>> On 07/22/15 at 12:10am, Roopa Prabhu wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> seen with CONFIG_IPV6 disabled. Wrap the code
>>> around IS_ENABLED(CONFIG_IPV6)
>>>
>>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> We need the same for CONFIG_INET=n in inet_fib_lookup_dev:
>>
>> /home/tgraf/dev/linux/net-next/include/net/route.h:122: undefined reference to `ip_route_output_flow'
>>
>> Is it worth returning EAFNOSUPPORT instead of ENODEV in these
>> cases?  Something like this:
>>
>>>   }
>>> +#else
>>> +static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
>>> +{
>>   +	return ERR_PTR(-EAFNOSUPPORT);
>>> +}
>>> +#endif
>> And then IS_ERR() in mpls_route_add()?
> Also IS_ENABLED() _DOES NOT_ work, what if MPLS is 'y' and IPV6 is 'm'?
>
> You can't reache the modular symbol from statically built code.
>
> What a mess.
>
Sorry about that, I was looking at other examples where this is done.
For ipvlan, CONFIG_IPVLAN depends on CONFIG_INET and CONFIG_INET6 and I
don't see any additional checks in the driver.
I am wondering if I should submit a patch that does the same for 
MPLS_ROUTING (I am trying to avoid that though).

diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
index 5c467ef..2b28615 100644
--- a/net/mpls/Kconfig
+++ b/net/mpls/Kconfig
@@ -24,6 +24,8 @@ config NET_MPLS_GSO

  config MPLS_ROUTING
         tristate "MPLS: routing support"
+       depends on INET
+       depends on IPV6
         ---help---
          Add support for forwarding of mpls packets.


Or I will rework the original patch in a different way.
This was introduced by my below commit. Please revert the below commit 
if there is no easy way to make it work
with MPLS_ROUTING = 'y' and IPV6 = 'm'. I will resubmit the patch. Thanks.


commit 01faef2cebae02685e2bcfc9bbee8416d5ec19fc
Author: Roopa Prabhu <roopa@cumulusnetworks.com>
Date:   Tue Jul 21 09:16:24 2015 -0700

     mpls: make RTA_OIF optional

     If user did not specify an oif, try and get it from the via address.
     If failed to get device, return with -ENODEV.

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

[Thomas Graf]

On 07/22/15 at 12:30pm, roopa wrote:
> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
> index 5c467ef..2b28615 100644
> --- a/net/mpls/Kconfig
> +++ b/net/mpls/Kconfig
> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
> 
>  config MPLS_ROUTING
>         tristate "MPLS: routing support"
> +       depends on INET
> +       depends on IPV6
>         ---help---
>          Add support for forwarding of mpls packets.

This looks like a much better fix to me and resolves the
module/built-in dependency mess.

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

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 22 Jul 2015 21:57:06 +0200

> On 07/22/15 at 12:30pm, roopa wrote:
>> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
>> index 5c467ef..2b28615 100644
>> --- a/net/mpls/Kconfig
>> +++ b/net/mpls/Kconfig
>> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
>> 
>>  config MPLS_ROUTING
>>         tristate "MPLS: routing support"
>> +       depends on INET
>> +       depends on IPV6
>>         ---help---
>>          Add support for forwarding of mpls packets.
> 
> This looks like a much better fix to me and resolves the
> module/built-in dependency mess.

It's only OK if we don't create a new hard dependency on IPV6,
which this patch does.

Consitently across the tree we give the user the option of
using a bi-AF facility with or without IPV6.

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

[Thomas Graf]

On 07/22/15 at 01:04pm, David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 22 Jul 2015 21:57:06 +0200
> 
> > On 07/22/15 at 12:30pm, roopa wrote:
> >> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
> >> index 5c467ef..2b28615 100644
> >> --- a/net/mpls/Kconfig
> >> +++ b/net/mpls/Kconfig
> >> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
> >> 
> >>  config MPLS_ROUTING
> >>         tristate "MPLS: routing support"
> >> +       depends on INET
> >> +       depends on IPV6
> >>         ---help---
> >>          Add support for forwarding of mpls packets.
> > 
> > This looks like a much better fix to me and resolves the
> > module/built-in dependency mess.
> 
> It's only OK if we don't create a new hard dependency on IPV6,
> which this patch does.
> 
> Consitently across the tree we give the user the option of
> using a bi-AF facility with or without IPV6.

OK. I guess there is an MPLS routing use case which does not
depend on INET or IPV6 if all routes specify an RTA_OIF. Not
enough of an expert to know if that is the common case or not.

Otherwise I would have argued to start dropping the special status
for IPv6 and start treating IP dependency as a combination of both
to promote it further/faster. It can still be explicitly disabled.
Then again, I might be too optimistic in assuming that this will
be the year of IPv6 ;-)

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

[roopa]

On 7/22/15, 1:17 PM, Thomas Graf wrote:
> On 07/22/15 at 01:04pm, David Miller wrote:
>> From: Thomas Graf <tgraf@suug.ch>
>> Date: Wed, 22 Jul 2015 21:57:06 +0200
>>
>>> On 07/22/15 at 12:30pm, roopa wrote:
>>>> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
>>>> index 5c467ef..2b28615 100644
>>>> --- a/net/mpls/Kconfig
>>>> +++ b/net/mpls/Kconfig
>>>> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
>>>>
>>>>   config MPLS_ROUTING
>>>>          tristate "MPLS: routing support"
>>>> +       depends on INET
>>>> +       depends on IPV6
>>>>          ---help---
>>>>           Add support for forwarding of mpls packets.
>>> This looks like a much better fix to me and resolves the
>>> module/built-in dependency mess.
>> It's only OK if we don't create a new hard dependency on IPV6,
>> which this patch does.
>>
>> Consitently across the tree we give the user the option of
>> using a bi-AF facility with or without IPV6.
> OK. I guess there is an MPLS routing use case which does not
> depend on INET or IPV6 if all routes specify an RTA_OIF. Not
> enough of an expert to know if that is the common case or not.

yes, there is ie., the kernel supports it. though we have never used it 
nor plan to. Not sure about the use case.
>
> Otherwise I would have argued to start dropping the special status
> for IPv6 and start treating IP dependency as a combination of both
> to promote it further/faster. It can still be explicitly disabled.
> Then again, I might be too optimistic in assuming that this will
> be the year of IPv6 ;-)
:).

I cant think of a way to fix the current problem with my patch....ie, 
when CONFIG_IPV6=m and my module does not have a hard dependency on 
CONFIG_IPV6.
If there are suggestions, pls let me know (I am ok with a revert until 
there is a solution).

I do see ip6_route_output used from several modules, they all quite 
likely have a hard dependency on CONFIG_IPV6.

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

[David Miller]

From: roopa <roopa@cumulusnetworks.com>
Date: Wed, 22 Jul 2015 13:38:31 -0700

> I cant think of a way to fix the current problem with my patch...

I guess it's not obvious that adding CONFIG_MPLS_IPV6 would solve
the problem perfectly.

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

[roopa]

On 7/23/15, 12:09 AM, David Miller wrote:
> From: roopa <roopa@cumulusnetworks.com>
> Date: Wed, 22 Jul 2015 13:38:31 -0700
>
>> I cant think of a way to fix the current problem with my patch...
> I guess it's not obvious that adding CONFIG_MPLS_IPV6 would solve
> the problem perfectly.
I  thought that was not an option because CONFIG_MPLS_ROUTING is already 
out and supports IPV6 when RTA_OIF is
specified.

I did submit another version yesterday using IS_BUILTIN (sorry missed 
the v2 in the patch prefix).
https://patchwork.ozlabs.org/patch/498903/

I can certainly introduce CONFIG_MPLS_IPV6, if that is a more acceptable.

thanks.

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

[David Miller]

From: roopa <roopa@cumulusnetworks.com>
Date: Thu, 23 Jul 2015 06:44:13 -0700

> On 7/23/15, 12:09 AM, David Miller wrote:
>> From: roopa <roopa@cumulusnetworks.com>
>> Date: Wed, 22 Jul 2015 13:38:31 -0700
>>
>>> I cant think of a way to fix the current problem with my patch...
>> I guess it's not obvious that adding CONFIG_MPLS_IPV6 would solve
>> the problem perfectly.
> I thought that was not an option because CONFIG_MPLS_ROUTING is
> already out and supports IPV6 when RTA_OIF is
> specified.
> 
> I did submit another version yesterday using IS_BUILTIN (sorry missed
> the v2 in the patch prefix).
> https://patchwork.ozlabs.org/patch/498903/
> 
> I can certainly introduce CONFIG_MPLS_IPV6, if that is a more
> acceptable.

Also, IS_BUILTIN() is the wrong check, because this works properly also
when both MPLS_ROUTING and IPV6 are modular.

You need to encode this in the Kconfig dependencies of a new option
(which uses the dependency expression "IPV6 || IPV6=n"), then you can
key off of that Kconfig symbol to determine whether you need to
provide those NOP fallbacks or not.  There really is no other option.

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

[roopa]

On 7/22/15, 1:04 PM, David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 22 Jul 2015 21:57:06 +0200
>
>> On 07/22/15 at 12:30pm, roopa wrote:
>>> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
>>> index 5c467ef..2b28615 100644
>>> --- a/net/mpls/Kconfig
>>> +++ b/net/mpls/Kconfig
>>> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
>>>
>>>   config MPLS_ROUTING
>>>          tristate "MPLS: routing support"
>>> +       depends on INET
>>> +       depends on IPV6
>>>          ---help---
>>>           Add support for forwarding of mpls packets.
>> This looks like a much better fix to me and resolves the
>> module/built-in dependency mess.
> It's only OK if we don't create a new hard dependency on IPV6,
> which this patch does.
>
> Consitently across the tree we give the user the option of
> using a bi-AF facility with or without IPV6.
yes, ack. I have been trying to avoid that ...also because
af_mpls does support a family AF_PACKET mode that should continue to 
work without CONFIG_INET and CONFIG_INET6
(Though I have not tried it).

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

[David Miller]

From: roopa <roopa@cumulusnetworks.com>
Date: Wed, 22 Jul 2015 12:30:19 -0700

> diff --git a/net/mpls/Kconfig b/net/mpls/Kconfig
> index 5c467ef..2b28615 100644
> --- a/net/mpls/Kconfig
> +++ b/net/mpls/Kconfig
> @@ -24,6 +24,8 @@ config NET_MPLS_GSO
> 
>  config MPLS_ROUTING
>         tristate "MPLS: routing support"
> +       depends on INET
> +       depends on IPV6
>         ---help---
>          Add support for forwarding of mpls packets.

Having to enable IPV6 in order to use MPLS is not reasonable.

You have to seperate out the individual components and give
the user a choice in the matter.

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

[roopa]

On 7/22/15, 5:23 AM, Thomas Graf wrote:
> On 07/22/15 at 12:10am, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> seen with CONFIG_IPV6 disabled. Wrap the code
>> around IS_ENABLED(CONFIG_IPV6)
>>
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> We need the same for CONFIG_INET=n in inet_fib_lookup_dev:
>
> /home/tgraf/dev/linux/net-next/include/net/route.h:122: undefined reference to `ip_route_output_flow'
  I will fix it.
>
> Is it worth returning EAFNOSUPPORT instead of ENODEV in these
> cases?  Something like this:

sure, I am ok with changing it to return EAFNOSUPPORT.
I decided on -ENODEV because I was already sending -ENODEV
for cases where the dev cannot be resolved.

>
>>   }
>> +#else
>> +static struct net_device *inet6_fib_lookup_dev(struct net *net, void *addr)
>> +{
>   +	return ERR_PTR(-EAFNOSUPPORT);
>> +}
>> +#endif
> And then IS_ERR() in mpls_route_add()?

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

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

seen with CONFIG_IPV6 disabled. Wrap the code around
IS_BUILTIN(CONFIG_IPV6). Also fixes undefined reference
to ip_route_output with CONFIG_INET=n

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Reported-by: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
could not think of a better way. This uses IS_BUILTIN instead
of IS_ENABLED to avoid the case where CONFIG_IPV6=m.
I am not sure if this could confuse the user.

 net/mpls/af_mpls.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 49f1b0e..71dbc4b 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_INET)
 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_BUILTIN(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