Bug 2145

Created attachment 2102 [details]
patch for solution-1

Created attachment 2103 [details]
patch for solution-2

I found that ArpL3Protocol::Lookup() method was not aware of the newly created "PERMANENT" entries. Any entry marked as PERMANENT is being excluded from the ArpCache Lookup. I can think of two possible solutions for this bug.

Solution-1
----------
Add additional lines to ArpL3Protocol::Lookup() to check if an entry is marked PERMANENT. If yes exctract the MacAddress from the entry.

Solution-2
----------
Modify the ArpCache::Entry::IsAlive () method to return true for PERMANENT entries as well. Currently ArpL3Protocol::Lookup() checks if an entry is ALIVE. As PERMANENT entries are in a way ALIVE (with no time out), we can leverage this flexibility to our advantage.

I've added two patch files for both type of solutions. Feel free to comment.

(In reply to Saswat K. Mishra from comment #3)
> I found that ArpL3Protocol::Lookup() method was not aware of the newly
> created "PERMANENT" entries. Any entry marked as PERMANENT is being excluded
> from the ArpCache Lookup. I can think of two possible solutions for this bug.
> 
> Solution-1
> ----------
> Add additional lines to ArpL3Protocol::Lookup() to check if an entry is
> marked PERMANENT. If yes exctract the MacAddress from the entry.
> 
> Solution-2
> ----------
> Modify the ArpCache::Entry::IsAlive () method to return true for PERMANENT
> entries as well. Currently ArpL3Protocol::Lookup() checks if an entry is
> ALIVE. As PERMANENT entries are in a way ALIVE (with no time out), we can
> leverage this flexibility to our advantage.
> 
> I've added two patch files for both type of solutions. Feel free to comment.


I prefer solution 1; it is more in line with the intent of the existing API to query for a specific existing state (IsAlive() or IsPermanent() are two different states).

I agree with Tom. The fist solution is way cleaner.

However, I'd like to point some missing things.
1) the ArpCache::PrintArpCache function must be updated to reflect the new state, and
2) there are some cases where the new state is not considered. E.g., check aodv and dsr and how they use IsAlive. Also in these cases the new state is not considered. Lastly,
3) I'd slightly rework all the if-then-else in ArpL3Protocol::Lookup. Currently it's if - elseif - elseif, with no else (i.e., no default action). This makes it hard to catch residual bugs like the one you had.

OK, We'll go with solution-1 then.

>I'll prepare a patch for the ArpCache::PrintArpCache(). Thanks for pointing it  out.
>We can change the implementation a bit to accommodate PERMANENT entries in case of aodv and dsr. I'll also put that in the next patch.
>About all the if-else statements, what do you guys suggest that we do? Using a switch might help in this scenario.

Created attachment 2123 [details]
modified print arp cache which takes PERMANENT into account

Created attachment 2124 [details]
modified aodv and dsr

link to the github branch that i'll be working on for this bug.
https://github.com/clicksaswat/ns-3-dev/commits/static-entries

(In reply to Saswat K. Mishra from comment #6)
> >About all the if-else statements, what do you guys suggest that we do? Using a switch might help in this scenario.

I'm neutral. Perhaps a switch could be even more evident (and it helps in catching the not-considered enums, since modern compilers can check this as well).

Ok then. I'll make the changes to use switch instead.

Sorry for the late comment as I was out of town recently.

1)About implementing the switch cases in ArpL3Protocol::Lookup(). Current implementation of if...else...if calls the IsXXX() methods to check the entry state and then take corresponding action for it. I think it would be lot more cleaner if we could use a getter method for ArpCache::Entry::m_state and then use the switch cases for each of the enum types.

2)Tom pointed out that currently there's no way to add an entry with PERMANENT state because because to modify the MacAddress value one have to call MarkAlive(Address macAddress) which only asserts for WAIT_REPLY entries.
So, a possible solution would be to change MarkPermanent as MarkPermanent(Address macAddress).

(In reply to Saswat K. Mishra from comment #12)
> Sorry for the late comment as I was out of town recently.
> 
> 1)About implementing the switch cases in ArpL3Protocol::Lookup(). Current
> implementation of if...else...if calls the IsXXX() methods to check the
> entry state and then take corresponding action for it. I think it would be
> lot more cleaner if we could use a getter method for
> ArpCache::Entry::m_state and then use the switch cases for each of the enum
> types.

I do not have a preference on whether we use switch or if-else construct; I think the main point that Tommaso raised was that there is no default 'else' to catch possible errors in the future.  So I recommend to either use switch with a default: case or else to implement the 'else' case where there is no match to the current state (e.g. raising a fatal error if there is no match, or asserting, or logging a warning).  

For example, where it says this:

          else if (entry->IsWaitReply ())
              {
                  NS_FATAL_ERROR ("Test for possibly unreachable code-- please file a bug report, with a test case, if this is ever hit");

could be change to simply 'else'


I agree that it seems cleaner to me to get the state and switch on it than to repeatedly ask the IsXXX() methods.

> 
> 2)Tom pointed out that currently there's no way to add an entry with
> PERMANENT state because because to modify the MacAddress value one have to
> call MarkAlive(Address macAddress) which only asserts for WAIT_REPLY entries.
> So, a possible solution would be to change MarkPermanent as
> MarkPermanent(Address macAddress).

I suggest to keep your proposed MarkPermanent () and document that it is to be used only to mark pre-existing entries.  To create new entries that are permanent, I suggest to add "CreatePermanent (Address macAddress)".

(In reply to Tom Henderson from comment #13)
> (In reply to Saswat K. Mishra from comment #12)
> > Sorry for the late comment as I was out of town recently.
> > 
> > 1)About implementing the switch cases in ArpL3Protocol::Lookup(). Current
> > implementation of if...else...if calls the IsXXX() methods to check the
> > entry state and then take corresponding action for it. I think it would be
> > lot more cleaner if we could use a getter method for
> > ArpCache::Entry::m_state and then use the switch cases for each of the enum
> > types.
> 
> I do not have a preference on whether we use switch or if-else construct; I
> think the main point that Tommaso raised was that there is no default 'else'
> to catch possible errors in the future.  So I recommend to either use switch
> with a default: case or else to implement the 'else' case where there is no
> match to the current state (e.g. raising a fatal error if there is no match,
> or asserting, or logging a warning).  
> 
> For example, where it says this:
> 
>           else if (entry->IsWaitReply ())
>               {
>                   NS_FATAL_ERROR ("Test for possibly unreachable code--
> please file a bug report, with a test case, if this is ever hit");
> 
> could be change to simply 'else'
> 
> 
> I agree that it seems cleaner to me to get the state and switch on it than
> to repeatedly ask the IsXXX() methods.
> 

I will try to implement the switch case by tomorrow before code freeze.


> > 
> > 2)Tom pointed out that currently there's no way to add an entry with
> > PERMANENT state because because to modify the MacAddress value one have to
> > call MarkAlive(Address macAddress) which only asserts for WAIT_REPLY entries.
> > So, a possible solution would be to change MarkPermanent as
> > MarkPermanent(Address macAddress).
> 
> I suggest to keep your proposed MarkPermanent () and document that it is to
> be used only to mark pre-existing entries.  To create new entries that are
> permanent, I suggest to add "CreatePermanent (Address macAddress)".

CreatePermanent (Address macAddress) seems just fine to me.

Ok. Here are some thoughts.

1)To add a getter method for m_state, the declaration of ArpCacheEntryState_e has to be made public. Are we allowed to do so?(We have to if we are to use the switch case in ArpL3Protocol::Lookup ())

2)About the CreatePermanent (). I think it should take both Ipv4Address and Address as parameter.

3)Wouldn't it be better if we just create a setter method for m_macAddress instead of going all the way to CreatePermanet () because the real problem here is we are unable to set the macAddress of an Entry after creating one.

(In reply to Saswat K. Mishra from comment #15)
> Ok. Here are some thoughts.
> 
> 1)To add a getter method for m_state, the declaration of
> ArpCacheEntryState_e has to be made public. Are we allowed to do so?(We have
> to if we are to use the switch case in ArpL3Protocol::Lookup ())
> 
> 2)About the CreatePermanent (). I think it should take both Ipv4Address and
> Address as parameter.
> 
> 3)Wouldn't it be better if we just create a setter method for m_macAddress
> instead of going all the way to CreatePermanet () because the real problem
> here is we are unable to set the macAddress of an Entry after creating one.

We can move ArpCacheEntryState_e to public if needed.  However, after I consider your questions, I lean towards making some more minimal changes along the lines of your suggestions.

- avoid a getter method for m_state and just add IsPermanent (), and keep the existing if ...else if .. construct that checks the IsXXX() methods.  But to respond to Tommaso's comment, in the non-expired case, add an additional 'else' clause that can catch unexpected state.

- avoid adding CreatePermanent () and instead add SetMacAddress (Address); CreatePermanent () or equivalent could be added as a future helper.

In this case, to make a permanent entry, then we require users to create a new entry using operator new, then SetMacAddress (Address), then SetIpv4Address (Ipv4Address), then MarkPermanent ().  MarkPermanent () should check that m_macAddress and m_ipv4Address have previously been set, but shouldn't care about the previous state, should it?  (i.e. remove the NS_ASSERT checking whether it was in ALIVE state).

At least this functionally allows us to create these entries, and we could add a helper method to do it all in one statement in the future.

(In reply to Tom Henderson from comment #16)
> (In reply to Saswat K. Mishra from comment #15)
> > Ok. Here are some thoughts.
> > 
> > 1)To add a getter method for m_state, the declaration of
> > ArpCacheEntryState_e has to be made public. Are we allowed to do so?(We have
> > to if we are to use the switch case in ArpL3Protocol::Lookup ())
> > 
> > 2)About the CreatePermanent (). I think it should take both Ipv4Address and
> > Address as parameter.
> > 
> > 3)Wouldn't it be better if we just create a setter method for m_macAddress
> > instead of going all the way to CreatePermanet () because the real problem
> > here is we are unable to set the macAddress of an Entry after creating one.
> 
> We can move ArpCacheEntryState_e to public if needed.  However, after I
> consider your questions, I lean towards making some more minimal changes
> along the lines of your suggestions.
> 
> - avoid a getter method for m_state and just add IsPermanent (), and keep
> the existing if ...else if .. construct that checks the IsXXX() methods. 
> But to respond to Tommaso's comment, in the non-expired case, add an
> additional 'else' clause that can catch unexpected state.
> 
> - avoid adding CreatePermanent () and instead add SetMacAddress (Address);
> CreatePermanent () or equivalent could be added as a future helper.
> 
> In this case, to make a permanent entry, then we require users to create a
> new entry using operator new, then SetMacAddress (Address), then
> SetIpv4Address (Ipv4Address), then MarkPermanent ().  MarkPermanent ()
> should check that m_macAddress and m_ipv4Address have previously been set,
> but shouldn't care about the previous state, should it?  (i.e. remove the
> NS_ASSERT checking whether it was in ALIVE state).
> 
> At least this functionally allows us to create these entries, and we could
> add a helper method to do it all in one statement in the future.

Another way to add PERMANENT entries would be to use ArpCache::Add (Ipv4Address) which will create Entry in ALIVE state. Then use SetMacAddress on this entry and then call MarkPermanent (). But, as you've suggested we've to check for a valid MacAddress for this entry otherwise a null Address can cause all kind of errors (i.e. in ArpL3Protocol::Lookup () we would be retrieving an null macAddress value.).

I don't see any reason an entry has to be ALIVE before marking PERMANENT. So, it would be better if we remove NS_ASSERT (m_state == ALIVE).

Created attachment 2140 [details]
SetMacAddress and modified ArpL3Protocol::Lookup()

+1 on my side. it seems that we didn't forget anything - beside to do the same for IPv6. I'll do it post 3.24.

Seem OK to me. I think we can add this one.

I've posted the agreed-upon patches to ns-3-dev, in two changesets:
11651:feff0e88048b
11652:af6897a9e544

I'll leave this bug open for IPv6 equivalent patches.

I found some unexpected behavior in testing the permanent entries today.  While we have added support to add these permanent entries to the ArpCache, I found that it is possible to add them during simulation configuration phase, only to have them flushed upon running the simulation.  The reason is that ArpL3Protocol registers the Flush() method with the NetDevice LinkChange callback.  Flush is defined to flush all entries:

  /**
   * \brief Clear the ArpCache of all entries
   */
  void Flush (void);


so when a link comes up in Wi-Fi (LinkUp() is called when association succeeds), it causes Flush () to be called.

It is somewhat burdensome to have to schedule the re-addition of these permanent entries when this occurs, so one possibility is to modify the Flush() behavior to only have it clear the ArpCache of all non-permanent entries upon link change events.  A patch along these lines is attached.

Created attachment 2143 [details]
possible patch to deal with Flush() behavior

> 
> It is somewhat burdensome to have to schedule the re-addition of these
> permanent entries when this occurs, so one possibility is to modify the
> Flush() behavior to only have it clear the ArpCache of all non-permanent
> entries upon link change events.  A patch along these lines is attached.

Another possibility is to keep Flush() as is, but register a different variant of Flush() (preserving permanent entries) with the LinkChange callback.

I'm not sure if the actual Flush behaviour is *the* good behaviour.

Usually when a UE changes its AP, its address is changed as well, and all the ARP entries are invalid, even the static ones.
The same goes for an Ethernet card. if I unplug it and re-plug, I expect the AR entries to be flushed, no matter if they're static or not.

If there's a reason to keep the static entries valid, then the same argument could be applied to the non-static entries. 

Note that my arguments are based on this double fact: 1) if you change AP, I expect that you change subnet -> ARP entries are for direct routing, and 2) if you change AP, you could have a valid host with the same MAC address as in the previous net but different IP.

Still, there are a lot of counterexamples, and the definitive answer is always the same: what Linux does ?


(In reply to Tom Henderson from comment #24)
> > 
> > It is somewhat burdensome to have to schedule the re-addition of these
> > permanent entries when this occurs, so one possibility is to modify the
> > Flush() behavior to only have it clear the ArpCache of all non-permanent
> > entries upon link change events.  A patch along these lines is attached.
> 
> Another possibility is to keep Flush() as is, but register a different
> variant of Flush() (preserving permanent entries) with the LinkChange
> callback.

> 
> Still, there are a lot of counterexamples, and the definitive answer is
> always the same: what Linux does ?
> 

Usually I agree with this default guidance.  However, I think that this issue is more about simulation configuration than aligning with implementation.

What is happening now is that it is impossible to add an entry that survives the configuration time to simulation time transition, in the case of WiFi or any other NetDevice that decides to move to a LinkUp state upon initialization.  

IMO, one of the main purposes of the static ARP is to make it easy for users to disable the effects of ARP in their simulations.  So, I would like some solution that allows me to configure this at simulation configuration time and not have to schedule the addition of entries for some simulation time in the future.  To me, it is somewhat similar to our GlobalRouting use case.

So from my perspective, it would be best if we prioritize the support of permanent entries that survive the LinkChange callback, and document that these are "really permanent entries-- use them if you don't want to have to touch ARP and your simulations are not changing the address bindings, and you need to manually remove them if you don't want them anymore" and then if we have a need to introduce a different type of static ARP binding that instead does not survive the LinkChange callback, someone can propose that patch later.

I see your point, and I agree.

If we go in this direction, I'm more favorable to have two flushes (like you proposed earlier) and to bind by default the one not flushing the permanent entries.

It could be nice to allow the user to override this behaviour tho, eventually changing manually the binding. Another option is to have an attribute in the ArpCache itself, e.g., "FlushCleanPermanentEntries".


(In reply to Tom Henderson from comment #26)
> > 
> > Still, there are a lot of counterexamples, and the definitive answer is
> > always the same: what Linux does ?
> > 
> 
> Usually I agree with this default guidance.  However, I think that this
> issue is more about simulation configuration than aligning with
> implementation.
> 
> What is happening now is that it is impossible to add an entry that survives
> the configuration time to simulation time transition, in the case of WiFi or
> any other NetDevice that decides to move to a LinkUp state upon
> initialization.  
> 
> IMO, one of the main purposes of the static ARP is to make it easy for users
> to disable the effects of ARP in their simulations.  So, I would like some
> solution that allows me to configure this at simulation configuration time
> and not have to schedule the addition of entries for some simulation time in
> the future.  To me, it is somewhat similar to our GlobalRouting use case.
> 
> So from my perspective, it would be best if we prioritize the support of
> permanent entries that survive the LinkChange callback, and document that
> these are "really permanent entries-- use them if you don't want to have to
> touch ARP and your simulations are not changing the address bindings, and
> you need to manually remove them if you don't want them anymore" and then if
> we have a need to introduce a different type of static ARP binding that
> instead does not survive the LinkChange callback, someone can propose that
> patch later.

I think the problem here becomes that It is impossible to do any kind of pre configuration of Arp Cache during the simulation configuration. Redefining Flush() will only solve the problems with PERMANENT entries.

Anyway, I think an easier solution to the problem you're facing would be first add the Net Device to the channel, then add ARP cache entries to its Ip interface. This way entries will be added after LinkUp() is called and will be saved from Flush(). (I don't know if its the same for Wifi Net Device but I think it will work for Csma device.)

I've added a patch to implement Ipv6 static entries. Please check it for any errors.

Some of the things I would like to mention:-

1) I guess Ipv6 and Icmpv6 implementation of ns-3 are in compliance with the RFC 2146 standard. But, RFC 2146 doesn't mention any kind of PERMANENT entries. It recommends only the following kind of entry states.

-INCOMPLETE
-REACHABLE
-STALE
-DELAY
-PROBE

So, with the addition of this PERMANENT entries we're kind of violating the RFC guideline.

The good thing is linux implementation of NdiscCache supports PERMANENT. But, the entry state have different kind of labeling in here.

-PERMANENT
-NOARP
-STALE 
-REACHABLE

My point here being with the addition of these new kind of entries, we're neither perfectly compatible with RFC nor linux. Is it a problem?

2)About the Flush() implementation discussed earlier, did adding the device on to the channel first before injecting arp entries worked? If it works, then I think it is better to leave the Flush() implementation as is. Because avoiding Static entries from flushing may lead to hard to find bugs.

When you're adding entries one by one during configuration phase it may be easy to remember to delete them manually. But, consider a situation where you're adding them in bulk(e.g using ArpCacheHelper::PopulateArpCache). It may be difficult to remember to remove them.

Created attachment 2145 [details]
ipv6 static entry

Hi,

I'm ok with the IPv6 implementation. I'd just add a "StopNudTimer ();" to "Entry::MarkPermanent ()", just to be sure.

About the Linux / RFC thing, I suspect that the Linux implementation does consider the missing states somewhere. perhaps in a non-obvious part. Just consider that the INCOMPLETE and DELAY states could be managed by timers and other flags.

Shall we commit this ?

(In reply to Saswat K. Mishra from comment #29)
> I've added a patch to implement Ipv6 static entries. Please check it for any
> errors.
> 
> Some of the things I would like to mention:-
> 
> 1) I guess Ipv6 and Icmpv6 implementation of ns-3 are in compliance with the
> RFC 2146 standard. But, RFC 2146 doesn't mention any kind of PERMANENT
> entries. It recommends only the following kind of entry states.
> 
> -INCOMPLETE
> -REACHABLE
> -STALE
> -DELAY
> -PROBE
> 
> So, with the addition of this PERMANENT entries we're kind of violating the
> RFC guideline.
> 
> The good thing is linux implementation of NdiscCache supports PERMANENT.
> But, the entry state have different kind of labeling in here.
> 
> -PERMANENT
> -NOARP
> -STALE 
> -REACHABLE
> 
> My point here being with the addition of these new kind of entries, we're
> neither perfectly compatible with RFC nor linux. Is it a problem?
> 
> 2)About the Flush() implementation discussed earlier, did adding the device
> on to the channel first before injecting arp entries worked? If it works,
> then I think it is better to leave the Flush() implementation as is. Because
> avoiding Static entries from flushing may lead to hard to find bugs.
> 
> When you're adding entries one by one during configuration phase it may be
> easy to remember to delete them manually. But, consider a situation where
> you're adding them in bulk(e.g using ArpCacheHelper::PopulateArpCache). It
> may be difficult to remember to remove them.

(In reply to Tommaso Pecorella from comment #31)
> Hi,
> 
> I'm ok with the IPv6 implementation. I'd just add a "StopNudTimer ();" to
> "Entry::MarkPermanent ()", just to be sure.
> 
> About the Linux / RFC thing, I suspect that the Linux implementation does
> consider the missing states somewhere. perhaps in a non-obvious part. Just
> consider that the INCOMPLETE and DELAY states could be managed by timers and
> other flags.
> 
> Shall we commit this ?
> 
> (In reply to Saswat K. Mishra from comment #29)
> > I've added a patch to implement Ipv6 static entries. Please check it for any
> > errors.
> > 
> > Some of the things I would like to mention:-
> > 
> > 1) I guess Ipv6 and Icmpv6 implementation of ns-3 are in compliance with the
> > RFC 2146 standard. But, RFC 2146 doesn't mention any kind of PERMANENT
> > entries. It recommends only the following kind of entry states.
> > 
> > -INCOMPLETE
> > -REACHABLE
> > -STALE
> > -DELAY
> > -PROBE
> > 
> > So, with the addition of this PERMANENT entries we're kind of violating the
> > RFC guideline.
> > 
> > The good thing is linux implementation of NdiscCache supports PERMANENT.
> > But, the entry state have different kind of labeling in here.
> > 
> > -PERMANENT
> > -NOARP
> > -STALE 
> > -REACHABLE
> > 
> > My point here being with the addition of these new kind of entries, we're
> > neither perfectly compatible with RFC nor linux. Is it a problem?
> > 
> > 2)About the Flush() implementation discussed earlier, did adding the device
> > on to the channel first before injecting arp entries worked? If it works,
> > then I think it is better to leave the Flush() implementation as is. Because
> > avoiding Static entries from flushing may lead to hard to find bugs.
> > 
> > When you're adding entries one by one during configuration phase it may be
> > easy to remember to delete them manually. But, consider a situation where
> > you're adding them in bulk(e.g using ArpCacheHelper::PopulateArpCache). It
> > may be difficult to remember to remove them.

I'll add StopNudTimer() to MarkPermanent(). This looks good for now. But, i think I'll take another look for any other left out corners.

Created attachment 2148 [details]
ipv6 static entry

In the Ipv6 static entries implementation, whenever we are getting a Neighbor Advertisement or a LLA, StartReachableTimer() is being called even for PERMANENT entries, which on consequent timeout will mark the entry as STALE.

A patch is attached which starts the reachable timer only for Non-PERMANENT entries.

Created attachment 2270 [details]
StartReachableTimer() only for non-permanent entries

This behaviour is still present in the actual code.
bout the specific behaviour, I'm neutral with the solution and/or if we decide to fix it, but I agree that it's kinda counter-intuitive.

Perhaps the solution could be different, tho. Why the Flush is performed in LinkUp ?
Shouldn't it be in LinkDown ?

This, however, is another bug: we have a LinkUp signalling, but not a LinkDown.

T.

(In reply to Tom Henderson from comment #22)
> I found some unexpected behavior in testing the permanent entries today. 
> While we have added support to add these permanent entries to the ArpCache,
> I found that it is possible to add them during simulation configuration
> phase, only to have them flushed upon running the simulation.  The reason is
> that ArpL3Protocol registers the Flush() method with the NetDevice
> LinkChange callback.  Flush is defined to flush all entries:
> 
>   /**
>    * \brief Clear the ArpCache of all entries
>    */
>   void Flush (void);
> 
> 
> so when a link comes up in Wi-Fi (LinkUp() is called when association
> succeeds), it causes Flush () to be called.
> 
> It is somewhat burdensome to have to schedule the re-addition of these
> permanent entries when this occurs, so one possibility is to modify the
> Flush() behavior to only have it clear the ArpCache of all non-permanent
> entries upon link change events.  A patch along these lines is attached.

Pushed IPv6 in changeset:   11878:3e6eb8126875
Sorry, I forgot to push it earlier.

I'll keep the bug open to fix Tom's inconsistent behavior when the link goes up.

Comment on attachment 2148 [details]
ipv6 static entry

changeset:   11878:3e6eb8126875

Comment on attachment 2270 [details]
StartReachableTimer() only for non-permanent entries

changeset:   11878:3e6eb8126875

There's is a mistake in the last patch(i.e StartReachableTimer() only for non-permanent entries). Somehow missed the brackets for IsPermanent() method call. Adding a new patch to fix it.

Created attachment 2273 [details]
fix missing brackets

About the flush behaviour, here's what happening

1) There's no call to ArpCache::Flush () when the simulation phase begins.
2) The only call that's being made to Flush() is when you attach a Channel to the device, its calling device's NotifyLinkUp (), which is calling the ArpCache::Flush () method.
3) So, to prevent flushing of entries added during Configuration Phase. We just have to add the Arp Entries after we have installed the Channel onto the  Device.
4) I wrote a test script to verify this. And, It's working. Permanent entries are surviving the Simulation Phase.

P.S. - All the testing has been done for a CsmaNetDevice. I'm not so sure about how it will go for Wi-Fi device.

This is a reminder that we should return to this.  I spent some time debugging a static ARP issue that I traced back to this Flush() issue; after moving the code after channel connection (on SimpleNetDevice), I got it to work, but IMO this is too fragile for users to have to remember the specific configuration sequence steps that will avoid a Flush().

Also, the current code for adding permanent entries is still very low level; e.g.

  Ptr<Ipv4L3Protocol> ip1 = node1->GetObject<Ipv4L3Protocol> ();
  int32_t interface1 = ip1->GetInterfaceForDevice (dev1);
  Ptr<Ipv4Interface> ipv4Interface1 = ip1->GetInterface (interface1);
  Ptr<ArpCache> cache1 = ipv4Interface1->GetArpCache ();
  ArpCache::Entry* entry1 = cache1->Add (Ipv4Address ("192.168.1.1"));
  entry1->SetMacAddress (Mac48Address ("00:00:00:00:00:01"));
  entry1->MarkPermanent ();

I'd prefer something like we have for global routing:

  static void Ipv4GlobalArpHelper::PopulateArpTables (void);

or with link-by-link scope:

  static void Ipv4GlobalArpHelper::PopulateArpTables (Ipv4Address, Ipv4Mask);

or on a node scope (or node/interface scope):

  static void Ipv4GlobalArpHelper::PopulateArpTables (Ptr<Node>);

Issue moved to https://gitlab.com/nsnam/ns-3-dev/-/issues/311

Closing.