IPv6: Blackhole route support partial ?

IPv6: Blackhole route support partial ?

[Kamala R]

Hi,

On adding IPv6 blackhole routes, ICMP unreachable messages are being
sent back to source. According to the definition, packets destined to
a blackhole address must be dropped silently.


I applied the patch submitted to the 3.7 kernel that indicates that it
supports blackhole and prohibit routes correctly. However, the patch
only sets the error code and route type correctly, so the show command
displays the appropriate output.


It seems to me that the input and output function pointers of the dst
variable, which determine packet processing, need to be set to
dst_discard. This would enable correct behaviour for blackhole routes.
Am I on the right path here ?

Thanks and Regards,
Kamala

Re: IPv6: Blackhole route support partial ?

[Hannes Frederic Sowa]

Hello!

On Mon, Nov 11, 2013 at 07:25:14PM +0530, Kamala R wrote:
> On adding IPv6 blackhole routes, ICMP unreachable messages are being
> sent back to source. According to the definition, packets destined to
> a blackhole address must be dropped silently.

Yes, this is a bug.

> I applied the patch submitted to the 3.7 kernel that indicates that it
> supports blackhole and prohibit routes correctly. However, the patch
> only sets the error code and route type correctly, so the show command
> displays the appropriate output.
> 
> 
> It seems to me that the input and output function pointers of the dst
> variable, which determine packet processing, need to be set to
> dst_discard. This would enable correct behaviour for blackhole routes.
> Am I on the right path here ?

I think you are. ip6_pkt_discard is not the correct input/output
function for blackhole routes. In ip6_route_add simply set up the
function pointers in the switch instead to just initializing them to
ip6_pkt_discard. dst_discard is fine. Looks like prohibit rules are not
handled correctly either. They should go to ip6_pkt_prohibit. (Just look at
how the templates are initialized.)

Could you cook a patch?

Thanks,

  Hannes

Re: IPv6: Blackhole route support partial ?

[Kamala R]

Hi,

Sure, here it is.

--- linux-3.12/net/ipv6/route.c.orig    2013-11-12 16:23:46.000000000 +0530
+++ linux-3.12/net/ipv6/route.c 2013-11-12 16:30:51.000000000 +0530
@@ -1570,9 +1570,13 @@ int ip6_route_add(struct fib6_config *cf
                switch (cfg->fc_type) {
                case RTN_BLACKHOLE:
                        rt->dst.error = -EINVAL;
+                       rt->dst.input = dst_discard;
+                       rt->dst.discard = dst_discard;
                        break;
                case RTN_PROHIBIT:
                        rt->dst.error = -EACCES;
+                       rt->dst.input = ip6_pkt_prohibit;
+                       rt->dst.output = ip6_pkt_prohibit_out;
                        break;
                case RTN_THROW:
                        rt->dst.error = -EAGAIN;

Is this ok ?

Regards,
Kamala


On Tue, Nov 12, 2013 at 2:09 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hello!
>
> On Mon, Nov 11, 2013 at 07:25:14PM +0530, Kamala R wrote:
>> On adding IPv6 blackhole routes, ICMP unreachable messages are being
>> sent back to source. According to the definition, packets destined to
>> a blackhole address must be dropped silently.
>
> Yes, this is a bug.
>
>> I applied the patch submitted to the 3.7 kernel that indicates that it
>> supports blackhole and prohibit routes correctly. However, the patch
>> only sets the error code and route type correctly, so the show command
>> displays the appropriate output.
>>
>>
>> It seems to me that the input and output function pointers of the dst
>> variable, which determine packet processing, need to be set to
>> dst_discard. This would enable correct behaviour for blackhole routes.
>> Am I on the right path here ?
>
> I think you are. ip6_pkt_discard is not the correct input/output
> function for blackhole routes. In ip6_route_add simply set up the
> function pointers in the switch instead to just initializing them to
> ip6_pkt_discard. dst_discard is fine. Looks like prohibit rules are not
> handled correctly either. They should go to ip6_pkt_prohibit. (Just look at
> how the templates are initialized.)
>
> Could you cook a patch?
>
> Thanks,
>
>   Hannes
>

Re: IPv6: Blackhole route support partial ?

[Hannes Frederic Sowa]

On Tue, Nov 12, 2013 at 04:39:10PM +0530, Kamala R wrote:
> Hi,
> 
> Sure, here it is.
> 
> --- linux-3.12/net/ipv6/route.c.orig    2013-11-12 16:23:46.000000000 +0530
> +++ linux-3.12/net/ipv6/route.c 2013-11-12 16:30:51.000000000 +0530
> @@ -1570,9 +1570,13 @@ int ip6_route_add(struct fib6_config *cf
>                 switch (cfg->fc_type) {
>                 case RTN_BLACKHOLE:
>                         rt->dst.error = -EINVAL;
> +                       rt->dst.input = dst_discard;
> +                       rt->dst.discard = dst_discard;
>                         break;
>                 case RTN_PROHIBIT:
>                         rt->dst.error = -EACCES;
> +                       rt->dst.input = ip6_pkt_prohibit;
> +                       rt->dst.output = ip6_pkt_prohibit_out;
>                         break;
>                 case RTN_THROW:
>                         rt->dst.error = -EAGAIN;
> 
> Is this ok ?

I woud move all the initialization of the function pointer into the
switch-case. You could merge the case RTN_THROW with the default one by just
using a ternary statement to initialize dst.error.

Your patch must be well-formed to get included into the
kernel. For that you should base your patch ontop net-next
or net, write a proper commit message and send the git
format-patch generated patch to this list. Here some hints:
<https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/Documentation/SubmittingPatches>

You can check if your formatting is correct by using scripts/checkpatch
--strict.

Let me know if there are any problems with that.

Thank you,

  Hannes

Re: IPv6: Blackhole route support partial ?

[Kamala R]

Hi,

I have one quick question before I send the patch across. I noticed
that "ip -6 route show" shows an error -22 in the output which
signifies -EINVAL associated with blackhole routes. This behaviour is
not consistent with that of "ip route show" that shows no such error
for a blackhole route. Does this qualify a bug that needs fixing ?

Regards,
Kamala

On Tue, Nov 12, 2013 at 7:24 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Tue, Nov 12, 2013 at 04:39:10PM +0530, Kamala R wrote:
>> Hi,
>>
>> Sure, here it is.
>>
>> --- linux-3.12/net/ipv6/route.c.orig    2013-11-12 16:23:46.000000000 +0530
>> +++ linux-3.12/net/ipv6/route.c 2013-11-12 16:30:51.000000000 +0530
>> @@ -1570,9 +1570,13 @@ int ip6_route_add(struct fib6_config *cf
>>                 switch (cfg->fc_type) {
>>                 case RTN_BLACKHOLE:
>>                         rt->dst.error = -EINVAL;
>> +                       rt->dst.input = dst_discard;
>> +                       rt->dst.discard = dst_discard;
>>                         break;
>>                 case RTN_PROHIBIT:
>>                         rt->dst.error = -EACCES;
>> +                       rt->dst.input = ip6_pkt_prohibit;
>> +                       rt->dst.output = ip6_pkt_prohibit_out;
>>                         break;
>>                 case RTN_THROW:
>>                         rt->dst.error = -EAGAIN;
>>
>> Is this ok ?
>
> I woud move all the initialization of the function pointer into the
> switch-case. You could merge the case RTN_THROW with the default one by just
> using a ternary statement to initialize dst.error.
>
> Your patch must be well-formed to get included into the
> kernel. For that you should base your patch ontop net-next
> or net, write a proper commit message and send the git
> format-patch generated patch to this list. Here some hints:
> <https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/Documentation/SubmittingPatches>
>
> You can check if your formatting is correct by using scripts/checkpatch
> --strict.
>
> Let me know if there are any problems with that.
>
> Thank you,
>
>   Hannes
>

Re: IPv6: Blackhole route support partial ?

[Hannes Frederic Sowa]

Hi!

On Thu, Nov 14, 2013 at 01:50:20PM +0530, Kamala R wrote:
> I have one quick question before I send the patch across. I noticed
> that "ip -6 route show" shows an error -22 in the output which
> signifies -EINVAL associated with blackhole routes. This behaviour is
> not consistent with that of "ip route show" that shows no such error
> for a blackhole route. Does this qualify a bug that needs fixing ?

You mean that we don't show the error for IPv4 routes? Is this a kernel or
iproute problem?

Greetings,

  Hannes

Re: IPv6: Blackhole route support partial ?

[Kamala R]

Hi,

That's right. We don't show the error for IPv4 routes as it follows a
different path in kernel while dumping the information as compared
with IPv6. Therefore, "ip route show" does not show an error while "ip
-6 route show" does.  So it looks to me that this a kernel problem
which needs to be fixed for consistent behavior. The simplest way to
fix this seems to be to set the error to zero while dumping the
information in the v6 path. I have tested this solution and found that
it works fine. Do you think this is the way to go ?

Regards,
Kamala

On Thu, Nov 14, 2013 at 7:41 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi!
>
> On Thu, Nov 14, 2013 at 01:50:20PM +0530, Kamala R wrote:
>> I have one quick question before I send the patch across. I noticed
>> that "ip -6 route show" shows an error -22 in the output which
>> signifies -EINVAL associated with blackhole routes. This behaviour is
>> not consistent with that of "ip route show" that shows no such error
>> for a blackhole route. Does this qualify a bug that needs fixing ?
>
> You mean that we don't show the error for IPv4 routes? Is this a kernel or
> iproute problem?
>
> Greetings,
>
>   Hannes
>

Re: IPv6: Blackhole route support partial ?

[Hannes Frederic Sowa]

Hi!

On Fri, Nov 15, 2013 at 03:06:28PM +0530, Kamala R wrote:
> That's right. We don't show the error for IPv4 routes as it follows a
> different path in kernel while dumping the information as compared
> with IPv6. Therefore, "ip route show" does not show an error while "ip
> -6 route show" does.  So it looks to me that this a kernel problem
> which needs to be fixed for consistent behavior. The simplest way to
> fix this seems to be to set the error to zero while dumping the
> information in the v6 path. I have tested this solution and found that
> it works fine. Do you think this is the way to go ?

If I understand you correctly you propose to drop the output of the error
attribute for IPv6 routes too? It is not that important that those two
outputs are identical and if you make a change, please introduce the
error propagation for IPv4 so one can see the socket errors for those
routes, too. I wouldn't drop those for IPv6 just for consistency reasons.

Greetings,

  Hannes

Re: IPv6: Blackhole route support partial ?

[Kamala R]

Hi,

I have sent across the patch on a separate thread. The subject of the mail is :

[PATCH 1/1] IPv6: Fixed support for blackhole and prohibit routes

Could you review ?

Thanks,
Kamala




On Fri, Nov 15, 2013 at 7:18 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi!
>
> On Fri, Nov 15, 2013 at 03:06:28PM +0530, Kamala R wrote:
>> That's right. We don't show the error for IPv4 routes as it follows a
>> different path in kernel while dumping the information as compared
>> with IPv6. Therefore, "ip route show" does not show an error while "ip
>> -6 route show" does.  So it looks to me that this a kernel problem
>> which needs to be fixed for consistent behavior. The simplest way to
>> fix this seems to be to set the error to zero while dumping the
>> information in the v6 path. I have tested this solution and found that
>> it works fine. Do you think this is the way to go ?
>
> If I understand you correctly you propose to drop the output of the error
> attribute for IPv6 routes too? It is not that important that those two
> outputs are identical and if you make a change, please introduce the
> error propagation for IPv4 so one can see the socket errors for those
> routes, too. I wouldn't drop those for IPv6 just for consistency reasons.
>
> Greetings,
>
>   Hannes
>

Re: IPv6: Blackhole route support partial ?

[David Miller]

From: Kamala R <kamala@aristanetworks.com>
Date: Wed, 20 Nov 2013 10:18:04 +0530

> I have sent across the patch on a separate thread. The subject of the mail is :
> 
> [PATCH 1/1] IPv6: Fixed support for blackhole and prohibit routes
> 
> Could you review ?

Can you just be patient?  You only posted the patch in the last day,
many people review patches in their spare time.