(In reply to comment #0)
> Upon creation an Ipv4Interface is set to forwarding state (which is not a
> default value for most systems). That's a glitch.
> In other words: all the IPv4 nodes are routers by default.

I seem to recall that we discussed this a long time ago and we decided to enable forwarding by default, since this is a network simulator that is heavily used for routing.

> 
> The bug is that if you disable the forwarding, the interface keeps forwarding,
> happy as a clam. I.e., Ipv4Interface::m_forwarding isn't checked before doing
> the actual forwarding.
> 
> Trivial bug, but it does present a side-effect: if you fix it you'll need also
> a way to enable the forwarding, possibly using an helper.
> 
> The *real* issue is that, upon fixing this bug, I'm pretty sure that 99% of the
> simulations using IPv4 will stop working.

Not if the helper enables this, or it is set to on by default.

Yes, this is a bug.  Let's discuss how to fix.

There are two levels of configuration:  whether the kernel itself is in forwarding state or not, and whether each specific interface is enabled for forwarding.  

What situations arise in which the kernel itself is enabled for forwarding, but selected interfaces are disabled for forwarding?  What user-space API exists to change the interface forwarding flag to allow this to happen?

If we can't identify use cases to disable specific interface forwarding, then perhaps we could just delete this parameter from the Ipv4Interface struct and just use the global forwarding flag?

Hi,

the "kernel" seems to be always in forwarding state, no matter what. Better: I can't find any option to disable the forwarding globally... probably my fault.

Speaking in Linuxish,
I can't find where is (net.ipv4.ip_forward = 1/0) and I suspect it's not even there. Probably assumed always true. IPv6 seems to work in the same way.
I found (net.ipv4.conf.<interface>.forwarding = 1/0) and it's not used in IPv4, while it is for IPv6.

The forwarding seems to be enabled on a per-interface basis.
For example, Ipv6L3Protocol::IsForwarding() is checked when receiving NS, RA, and other stuff.
The interesting part is in Ipv6XxxxRouting::RouteInput
[...]
  // Check if input device supports IP forwarding
  if (m_ipv6->IsForwarding (iif) == false)
    {
      NS_LOG_LOGIC ("Forwarding disabled for this interface");
      ecb (p, header, Socket::ERROR_NOROUTETOHOST);
      return false;
    }
  // Next, try to find a route
[...]

This check seems to be not performed by IPv4's routing protocols, thus making the state ineffective.

In my opinion we can "fix" this bug in two steps:
1) add the required checks in the relevant protocols (Static, List, Nix).
2) document that the check is *not* done in other protocols (e.g., OLSR, DSR, etc.), as it's pointless in these cases (nodes have to have a forwarding capability)
3) leave the m_forwarding to true by default, to not break user's simulations (if they didn't check the m_forwarding properly).

This seems to me the most logical thing to do.

PS: I found this bug by checking what was the IPv4 helper's API to setup the forwarding (and default routers)... simply none, it works out of the box :/

(In reply to comment #2)
> Hi,
> 
> the "kernel" seems to be always in forwarding state, no matter what. Better: I
> can't find any option to disable the forwarding globally... probably my fault.

Yes, this is an aspect of the bug.

> 
> Speaking in Linuxish,
> I can't find where is (net.ipv4.ip_forward = 1/0) and I suspect it's not even
> there. Probably assumed always true. 

by default, ip_forward is 0, but sysctl or proc can be used to set this to 1.  I think we want to make IPv4L3Protocol respect this value.

> IPv6 seems to work in the same way.
> I found (net.ipv4.conf.<interface>.forwarding = 1/0) and it's not used in IPv4,
> while it is for IPv6.

There is a check in ip_error in which returning an ICMP unreachable message is suppressed if the input interface is not enabled for forwarding:
http://lxr.linux.no/#linux+v3.9.6/net/ipv4/route.c#L844

Otherwise, I'm not aware of how this is used in practice on Linux, or how a user (using user-space tools) is able to selectively disable forwarding on an interface-by-interface basis.  This is why I suggest that it may be easier on us to remove this.


> 
> The forwarding seems to be enabled on a per-interface basis.
> For example, Ipv6L3Protocol::IsForwarding() is checked when receiving NS, RA,
> and other stuff.
> The interesting part is in Ipv6XxxxRouting::RouteInput
> [...]
>   // Check if input device supports IP forwarding
>   if (m_ipv6->IsForwarding (iif) == false)
>     {
>       NS_LOG_LOGIC ("Forwarding disabled for this interface");
>       ecb (p, header, Socket::ERROR_NOROUTETOHOST);
>       return false;
>     }
>   // Next, try to find a route
> [...]
> 
> This check seems to be not performed by IPv4's routing protocols, thus making
> the state ineffective.
> 
> In my opinion we can "fix" this bug in two steps:
> 1) add the required checks in the relevant protocols (Static, List, Nix).
> 2) document that the check is *not* done in other protocols (e.g., OLSR, DSR,
> etc.), as it's pointless in these cases (nodes have to have a forwarding
> capability)
> 3) leave the m_forwarding to true by default, to not break user's simulations
> (if they didn't check the m_forwarding properly).
> 
> This seems to me the most logical thing to do.

I think we should agree on some policy first.

- what is the default configuration of an ns-3 node?  I suggest that we keep the default as ip_forward=1 (it could either be enabled in the helper, or in the constructor of the object), and make sure that this can be disabled via the Ipv4L3Protocol/Ipv4 API.  In the forwarding path within Ipv4L3Protocol, we should align with where this is checked/enforced in the IPv4 Linux stack, although we are not sending ICMP as a result, but just logging a drop.

- what is the default configuration of an ns-3 interface?  Should we support forwarding status (on/off) at an interface level?  Since we are not sending ICMP, perhaps it doesn't make sense to bother with this now, and perhaps we remove the API from Ipv4Interface.

- what is the impact on higher layer routing protocols (all that you mentioned above)?  Dynamic routing protocols need to know and react to this state change (probably by listening to a trace source).  Static routing is debatable; I would probably not bother with flushing routes upon a state change and just keep them around in case ip_forward is re-enabled.

We should also describe test cases, then implement them.  For each routing protocol that we support, we could test that at time 0, IPv4 forwarding works, then disable IPv4 routing at time t, check that the routing protocols respond to this correctly, and then at time (t+delta), enable it and make sure that the routing protocols again respond correctly by some later time.  For static routing, we could have a separate special test case that toggled the Ipv4 routing state, and confirmed that forwarding was blocked when the forward flag was false, and that forwarding returned when the forward flag returned to true.

> 
> PS: I found this bug by checking what was the IPv4 helper's API to setup the
> forwarding (and default routers)... simply none, it works out of the box :/

My sense is that this is a bug that hasn't really impacted anyone since forwarding is always on and there is little incentive to try to disable it.

(In reply to comment #3)
> (In reply to comment #2)
> I think we should agree on some policy first.

Policies are always good, as they allow us to build a consistent behaviour.
 
> - what is the default configuration of an ns-3 node?  I suggest that we keep
> the default as ip_forward=1 (it could either be enabled in the helper, or in
> the constructor of the object), and make sure that this can be disabled via the
> Ipv4L3Protocol/Ipv4 API.  In the forwarding path within Ipv4L3Protocol, we
> should align with where this is checked/enforced in the IPv4 Linux stack,
> although we are not sending ICMP as a result, but just logging a drop.

I do agree on this. It means a new state in the stack (I'm not sure if it's better in Ipv[46] or Ipv[46]L3Protocol). Since it's a new state, we can simply add an Attribute.

> - what is the default configuration of an ns-3 interface?  Should we support
> forwarding status (on/off) at an interface level?  Since we are not sending
> ICMP, perhaps it doesn't make sense to bother with this now, and perhaps we
> remove the API from Ipv4Interface.

It does make sense. In a real system it would be a double check (global *and* interface-specific state). I can't figure out a practical case where interface-specific setting could be interesting for ns-3.
In a real system the flag might be used to enforce security policies (e.g., an administrative interface on a router), but on ns-3 I can't see a practical use-case.

> - what is the impact on higher layer routing protocols (all that you mentioned
> above)?  Dynamic routing protocols need to know and react to this state change
> (probably by listening to a trace source).  Static routing is debatable; I
> would probably not bother with flushing routes upon a state change and just
> keep them around in case ip_forward is re-enabled.

I won't bother either. I'm not even sure that a linux stack would flush the routes upon forwarding state change...
Anyway, about the routing protocols reaction, there are two ways:
1) The specific routing protocol, upon installation, night force the forwarding state to true. This might be needed by multihop routing protocols, as it doesn't make sense to have a non-forwarding node.
2) The routing protocol might simply ignore the state.
I'd like more the first option, as it would allow to simulate a "strange" behaviour, where a node is not willing to forward anything.
On the other hand, it's also the most intrusive choice, as the protocols will have to be modified.

> We should also describe test cases, then implement them.  For each routing
> protocol that we support, we could test that at time 0, IPv4 forwarding works,
> then disable IPv4 routing at time t, check that the routing protocols respond
> to this correctly, and then at time (t+delta), enable it and make sure that the
> routing protocols again respond correctly by some later time.  For static
> routing, we could have a separate special test case that toggled the Ipv4
> routing state, and confirmed that forwarding was blocked when the forward flag
> was false, and that forwarding returned when the forward flag returned to true.

I hate building tests, but I totally agree on this point. Sadly a test is needed.

Anyway, the bug is a side-product of another bug, related to IPv6.

Since these bugs are probably going to change the APIs a lot, I'll copy-paste the thread to ns-dev, also to collect the opinions of those not looking daily at bugs :P

T.

Created attachment 1619 [details]
IPv6 and IPv4 forwarding test

It turns out that I was completely wrong. Building the test for IPv6 and IPv4 made it clear that everything works as intended for IPv4. Cumbersome maybe, but working.
The test should make it clear how to use the APIs.

NB: the test works if and only if the default state of the IPv6 is set to forwarding.

Fixed in changeset: 9882:a975d133fa12