[PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Ville Nuorvala]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

Hi David,

please apply the attached patch!

Regards,
Ville

[-- Attachment #2: fib6_rule_null_route.patch --]
[-- Type: text/x-patch, Size: 1036 bytes --]

commit 1fb9864632af5a493353643e7acf970da1d4f59f
tree 569fd122cad5000077f80138a3014c390a7999f5
parent a66c990b9f6b0b9b4c4d2b84f96ddd019b7a8eb3
author Ville Nuorvala <vnuorval@tcs.hut.fi> 1155073548 +0300
committer Ville Nuorvala <vnuorval@tcs.hut.fi> 1155073548 +0300

    [IPV6]: Make sure fib6_rule_lookup doesn't return NULL
    
    The callers of fib6_rule_lookup don't expect it to return NULL,
    therefore it must return ip6_null_entry whenever fib_rule_lookup fails.
    
    Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>

diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index bf9bba8..22a2fdb 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -63,7 +63,11 @@ struct dst_entry *fib6_rule_lookup(struc
 	if (arg.rule)
 		fib_rule_put(arg.rule);
 
-	return (struct dst_entry *) arg.result;
+	if (arg.result)
+		return (struct dst_entry *) arg.result;
+
+	dst_hold(&ip6_null_entry.u.dst);
+	return &ip6_null_entry.u.dst;
 }
 
 static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Thomas Graf]

* Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 01:05
>     [IPV6]: Make sure fib6_rule_lookup doesn't return NULL
>     
>     The callers of fib6_rule_lookup don't expect it to return NULL,
>     therefore it must return ip6_null_entry whenever fib_rule_lookup fails.
>     
>     Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>

I think it would be a lot cleaner to return an error code
like fib_lookup() and save the atomic operations. We only
destroy the dst again anyway in case of a failure.

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 9 Aug 2006 00:16:51 +0200

> * Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 01:05
> >     [IPV6]: Make sure fib6_rule_lookup doesn't return NULL
> >     
> >     The callers of fib6_rule_lookup don't expect it to return NULL,
> >     therefore it must return ip6_null_entry whenever fib_rule_lookup fails.
> >     
> >     Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>
> 
> I think it would be a lot cleaner to return an error code
> like fib_lookup() and save the atomic operations. We only
> destroy the dst again anyway in case of a failure.

Yes, the current scheme is a poor substitute for IS_ERR/PTR_ERR.

But for now let's fix the bug and apply Ville's patch.

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Ville Nuorvala]

David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 9 Aug 2006 00:16:51 +0200
> 
>> * Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 01:05
>>>     [IPV6]: Make sure fib6_rule_lookup doesn't return NULL
>>>     
>>>     The callers of fib6_rule_lookup don't expect it to return NULL,
>>>     therefore it must return ip6_null_entry whenever fib_rule_lookup fails.
>>>     
>>>     Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>
>> I think it would be a lot cleaner to return an error code
>> like fib_lookup() and save the atomic operations. We only
>> destroy the dst again anyway in case of a failure.
> 
> Yes, the current scheme is a poor substitute for IS_ERR/PTR_ERR.

But the ip6_null_entry not always discarded after a failed route lookup!
On a forwarding router it triggers the ICMPv6 Destination Unreachable
message to the sender.

Regards,
Ville

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[David Miller]

From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Wed, 09 Aug 2006 09:31:15 +0300

> But the ip6_null_entry not always discarded after a failed route lookup!
> On a forwarding router it triggers the ICMPv6 Destination Unreachable
> message to the sender.

Good point, but I remember that in some cases the caller only wants a
real entry or an error.

Perhaps both cases can be accomodated with a SHIM inline for the
forwarding case that returns &ip6_null_entry when IS_ERR(retval).

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Ville Nuorvala]

David Miller wrote:
> From: Ville Nuorvala <vnuorval@tcs.hut.fi>
> Date: Wed, 09 Aug 2006 09:31:15 +0300
> 
>> But the ip6_null_entry not always discarded after a failed route lookup!
>> On a forwarding router it triggers the ICMPv6 Destination Unreachable
>> message to the sender.
> 
> Good point, but I remember that in some cases the caller only wants a
> real entry or an error.

That's technically true, but the IPv6 code has largely been built around
the assumption that the route lookup produces a valid pointer to a
rt6_info entry.

Of the three original route lookup functions (ip6_route_input,
ip6_route_output and rt6_lookup), rt6_lookup was the only one that was
allowed to produce a NULL entry. Of these three rt6_lookup was also the
only one not actually being used for routing.

The function that absolutely requires ip6_null_entry is ip6_route_input.

As for ip6_route_output, even if it can return NULL or an error code, we
still need to check for dst->error separately in all the places we call
the function.

There is also one more issue with ip6_null_entry: previously it has
always been the result of an unsuccessful route lookup, now it can also
be the result of a successful application of a FR_ACT_UNREACHABLE policy
rule. From a networking point of view these two cases should IMO be
considered equivalent and should therefore trigger the same response.
This will however not be true if NULL (or an error code) is the result
of an unsuccessful route lookup.

> Perhaps both cases can be accomodated with a SHIM inline for the
> forwarding case that returns &ip6_null_entry when IS_ERR(retval).

If no one else is worried about the FR_ACT_UNREACHABLE inconsistency, I
can of course move the ip6_null_entry fallback code snippet to
ip6_route_input and perhaps ip6_route_output, while letting the NULL
route fall through fib6_rule_lookup and rt6_lookup.

Regards,
Ville

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Thomas Graf]

* Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 11:36
> Of the three original route lookup functions (ip6_route_input,
> ip6_route_output and rt6_lookup), rt6_lookup was the only one that was
> allowed to produce a NULL entry. Of these three rt6_lookup was also the
> only one not actually being used for routing.
> 
> The function that absolutely requires ip6_null_entry is ip6_route_input.

It would mean to change the logic of handling route errors like in the
IPv4 path and not handle them in .input/.output. Instead of a dst we'd
return a valid dst or a ERR_PTR() which would force the caller to take
appropriate actions such as updating statistics and sending ICMPs.

> There is also one more issue with ip6_null_entry: previously it has
> always been the result of an unsuccessful route lookup, now it can also
> be the result of a successful application of a FR_ACT_UNREACHABLE policy
> rule. From a networking point of view these two cases should IMO be
> considered equivalent and should therefore trigger the same response.
> This will however not be true if NULL (or an error code) is the result
> of an unsuccessful route lookup.

Both would simply result in a -ENETUNREACHABLE.

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Ville Nuorvala]

Thomas Graf wrote:
> * Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 11:36
>> Of the three original route lookup functions (ip6_route_input,
>> ip6_route_output and rt6_lookup), rt6_lookup was the only one that was
>> allowed to produce a NULL entry. Of these three rt6_lookup was also the
>> only one not actually being used for routing.
>>
>> The function that absolutely requires ip6_null_entry is ip6_route_input.
> 
> It would mean to change the logic of handling route errors like in the
> IPv4 path and not handle them in .input/.output. Instead of a dst we'd
> return a valid dst or a ERR_PTR() which would force the caller to take
> appropriate actions such as updating statistics and sending ICMPs.

Ok, it might require quite big changes to the existing code, but if
someone is willing to take a look at it I wouldn't be against it :-)

>> There is also one more issue with ip6_null_entry: previously it has
>> always been the result of an unsuccessful route lookup, now it can also
>> be the result of a successful application of a FR_ACT_UNREACHABLE policy
>> rule. From a networking point of view these two cases should IMO be
>> considered equivalent and should therefore trigger the same response.
>> This will however not be true if NULL (or an error code) is the result
>> of an unsuccessful route lookup.
> 
> Both would simply result in a -ENETUNREACHABLE.

You know, I'm starting to think we could perhaps get rid of
ip6_null_entry altogether. I at least don't really see any good reason
to keep it after such changes.

This would apply even more strongly to the new ip6_prohibit_entry and
ip6_blk_hole_entry as they don't even serve as routing table dummy entries.

Regards,
Ville

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Thomas Graf]

* Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 12:42
> Ok, it might require quite big changes to the existing code, but if
> someone is willing to take a look at it I wouldn't be against it :-)

I'll provide a patch soon.

> You know, I'm starting to think we could perhaps get rid of
> ip6_null_entry altogether. I at least don't really see any good reason
> to keep it after such changes.
> 
> This would apply even more strongly to the new ip6_prohibit_entry and
> ip6_blk_hole_entry as they don't even serve as routing table dummy entries.

Absolutely, that's the whole point from my perspective, they're
only holding an error code and somewhat hiding error handling.

Re: [PATCH][IPV6]: Make sure fib6_rule_lookup doesn't return NULL

[Ville Nuorvala]

Thomas Graf wrote:
> * Ville Nuorvala <vnuorval@tcs.hut.fi> 2006-08-09 12:42
>> Ok, it might require quite big changes to the existing code, but if
>> someone is willing to take a look at it I wouldn't be against it :-)
> 
> I'll provide a patch soon.

Great, but just out of curiosity: about when is soon? My source address
routing changes are likely to cause some conflicts with your code, so
I'm just trying to plan ahead so I don't need to spend half an eternity
merging the afore mentioned conflicts.

Regards,
Ville