: Make sure fib6_rule_lookup doesn't return NULL
Subsystems (3)
PATCH 1/1 : Make sure fib6_rule_lookup doesn't return NULL
Hi David,
please apply the attached patch!
Regards,
Ville
commit 1fb9864632af5a493353643e7acf970da1d4f59f
tree 569fd122cad5000077f80138a3014c390a7999f5
parent a66c990b9f6b0b9b4c4d2b84f96ddd019b7a8eb3 author Ville Nuorvala <[email protected]> 1155073548 +0300 committer Ville Nuorvala <[email protected]> 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.
net/ipv6/fib6_rules.c +5 -1
@@ -63,7 +63,11 @@ struct dst_entry *fib6_rule_lookup(strucif (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,
* Ville Nuorvala <[email protected]> 2006-08-09 01:05
Quoted 6 lines
[IPV6]: Make sure
fib6_rule_lookupdoesn't return NULLThe callers of
fib6_rule_lookupdon't expect it to return NULL, therefore it must returnip6_null_entrywheneverfib_rule_lookupfails.Signed-off-by Ville Nuorvala <[email protected]>
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.
David Miller
maintainerFrom: Thomas Graf <[email protected]>
Date: Wed, 9 Aug 2006 00:16:51 +0200
Quoted 11 lines
* Ville Nuorvala <[email protected]> 2006-08-09 01:05
Quoted 6 lines
[IPV6]: Make sure
fib6_rule_lookupdoesn't return NULLThe callers of
fib6_rule_lookupdon't expect it to return NULL, therefore it must returnip6_null_entrywheneverfib_rule_lookupfails.Signed-off-by Ville Nuorvala <[email protected]>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.
Quoted 9 lines · David Miller
From: Thomas Graf <[email protected]>
Date: Wed, 9 Aug 2006 00:16:51 +0200Quoted 6 lines
* Ville Nuorvala <[email protected]> 2006-08-09 01:05
Quoted 5 lines
[IPV6]: Make sure
fib6_rule_lookupdoesn't return NULLThe callers of
fib6_rule_lookupdon't expect it to return NULL, therefore it must returnip6_null_entrywheneverfib_rule_lookupfails.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Ville Nuorvala <[email protected]>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
David Miller
maintainerFrom: Ville Nuorvala <[email protected]>
Date: Wed, 09 Aug 2006 09:31:15 +0300
Quoted 3 lines
But the
ip6_null_entrynot 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).
Quoted 9 lines · David Miller
From: Ville Nuorvala <[email protected]>
Date: Wed, 09 Aug 2006 09:31:15 +0300Quoted 3 lines
But the
ip6_null_entrynot 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.
Quoted 2 lines
Perhaps both cases can be accomodated with a SHIM inline for the forwarding case that returns
&ip6_null_entrywhenIS_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
* Ville Nuorvala <[email protected]> 2006-08-09 11:36
Quoted 6 lines
Of the three original route lookup functions (
ip6_route_input,ip6_route_outputandrt6_lookup),rt6_lookupwas the only one that was allowed to produce a NULL entry. Of these threert6_lookupwas also the only one not actually being used for routing.The function that absolutely requires
ip6_null_entryisip6_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.
Quoted 7 lines
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 aFR_ACT_UNREACHABLEpolicy 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.
Quoted 6 lines · Thomas Graf
* Ville Nuorvala <[email protected]> 2006-08-09 11:36
Quoted 5 lines
Of the three original route lookup functions (
ip6_route_input,ip6_route_outputandrt6_lookup),rt6_lookupwas the only one that was allowed to produce a NULL entry. Of these threert6_lookupwas also the only one not actually being used for routing.
Quoted 6 lines
Quoted 1 line
The function that absolutely requires
ip6_null_entryisip6_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 :-)
Quoted 9 lines
Quoted 7 lines
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 aFR_ACT_UNREACHABLEpolicy 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
* Ville Nuorvala <[email protected]> 2006-08-09 12:42
Quoted 2 lines
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.
Quoted 6 lines
You know, I'm starting to think we could perhaps get rid of
ip6_null_entryaltogether. 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_entryandip6_blk_hole_entryas 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.
Quoted 5 lines · Thomas Graf
* Ville Nuorvala <[email protected]> 2006-08-09 12:42
Quoted 2 lines
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