Created attachment 2422 [details]
patch

The patch is huge, because it contains also a change on the forwarding callbacks signature.

This is necessary to make them similar to their IPv6 counterparts, and to make it possible to catch there the case of a routing protocol not handling correctly the forwarding state.

The patch also changes some return cases. If a packet is discarded because the interface is in non-forwarding state, it's not necessary to continue looking for a routing protocol that can forward it.

The changes are in internet, aodv, nix, dsdv and olsr.

Created attachment 2423 [details]
patch with bindings

New patch with all bindings rescanned.

+  if (GetInterface (GetInterfaceForDevice (idev))->IsForwarding () == false)
+    {
+      NS_LOG_LOGIC ("Forwarding disabled for this interface");
+      RouteInputError (p, header, Socket::ERROR_NOROUTETOHOST);
+      return;
+    }

This is present in the Ipv4L3Protocol:IpForward() and all the routing protocols, with the proposed patch.

Is the above the right error to be returning in this case?  Should any error be conveyed (this is, after all, a configured choice by the user)?

I think we also need to consider where in the process the check should be made, because statistics and trace sources are changed by this decision.  Presently, it seems that Ipv4L3Protocol::Receive () -> RouteInput callback -> Ipv4L3Protocol::IpForward ().

It seems that the patch proposes to carry the idev through to IpForward() so that a check can be made there, but also, it patches the routing protocol to check also there and suppress the call to IpForward() if this is detected.  So it seems that the check being present in IpForward() is in case the routing protocol RouteInput() forgot to check?

So, is it the responsibility of Ipv4L3Protocol::Receive to do this check, and pass a boolean to the RouteInput such as 'inputInterfaceForwarding' (= true or false)?  Or is responsibility of routing protocol to check?  Or is it responsibility of downstream IpForward () to check?

If we conclude that it is responsibility of routing protocol to check, then are any signature changes needed?   It seems to me, perhaps not.

(In reply to Tom Henderson from comment #3)
> +  if (GetInterface (GetInterfaceForDevice (idev))->IsForwarding () == false)
> +    {
> +      NS_LOG_LOGIC ("Forwarding disabled for this interface");
> +      RouteInputError (p, header, Socket::ERROR_NOROUTETOHOST);
> +      return;
> +    }
> 
> This is present in the Ipv4L3Protocol:IpForward() and all the routing
> protocols, with the proposed patch.
> 
> Is the above the right error to be returning in this case?  Should any error
> be conveyed (this is, after all, a configured choice by the user)?

The "error" is not a true error. The error lies in the fact that the packet has been sent to the wrong node.
In all the code, even IPv6, when a packet reaches a node and should be forwarded but it can't due to forwaring being disabled, this error is raised.

> I think we also need to consider where in the process the check should be
> made, because statistics and trace sources are changed by this decision. 
> Presently, it seems that Ipv4L3Protocol::Receive () -> RouteInput callback
> -> Ipv4L3Protocol::IpForward ().
> 
> It seems that the patch proposes to carry the idev through to IpForward() so
> that a check can be made there, but also, it patches the routing protocol to
> check also there and suppress the call to IpForward() if this is detected. 
> So it seems that the check being present in IpForward() is in case the
> routing protocol RouteInput() forgot to check?

Exactly. After submitting the patch I realized this and clarified the message. Now it points that the routing protocol didn't catch a packet that should have been blocked.

> So, is it the responsibility of Ipv4L3Protocol::Receive to do this check,
> and pass a boolean to the RouteInput such as 'inputInterfaceForwarding' (=
> true or false)?  Or is responsibility of routing protocol to check?  Or is
> it responsibility of downstream IpForward () to check?

It could be possible also to do a forward check. I.e., nullify Unicast/Multicast forward callbacks. However the check must be done *also* i the routing protocol.
IpForward () can check for erroneous cases, and the signature change is mostly done to:
1) align with IPv6,
2) foolproof check,
3) simple protocols like StaticRouting.

In more complex cases (reactive routing, AODV for example) the check must be done inside the protocol. It wouldn't be nice to start a RREQ/RREP and discover *after* that you didn't had to forward the packet.

> If we conclude that it is responsibility of routing protocol to check, then
> are any signature changes needed?   It seems to me, perhaps not.

You have a point, but ICMP Destination Unreachable are there in IPv4 too (and we don't send them).
I'll think about it, but we could end up with a change in the signatures anyway... in IPv6 tho.

Created attachment 2425 [details]
new patch

New patch - no signature changes needed.

After a good sleep I decided that the functions signature uniformity is not worth the hassle - until we find a better reason to do it.

Rejecting a packet from a non-forwarding interface is a routing protocol responsibility, and it can't be avoided or done before *or* after. On both cases we'd introduce bugs and anomalies.

Sending the ICMP route error is done in RouteInputError, as it should be.

changeset: 12158:b1418062aa29