RE: [patch] Problem with two NIC on same NET (in_scrubprefix: err=17, new prefix add failed)

  Hi,

> 
> I've started my work with not point-to-point interfaces and I've
> found two problems. The first one - 
>
    <snip>
> 
> When I've done more investigation, it looks similar to
> http://svnweb.freebsd.org/base?view=revision&revision=201543
> So, I propose the following patch.
>

  I agree with your fix.

  As you've noted, I made the r201543 patch in IPv6 almost 2 years ago. 

  Turned out I had a note-to-self to verify if there are other similar 
  problems at the time but busy day job took me away ...

>
> The second one - submitted patch and description is bellow:
> 
>  http://www.freebsd.org/cgi/query-pr.cgi?pr=159603
> 

   I agree with your fix.

   <snip>

> 
>  I have no problem with loopback routes when I work with not
> point-to-point interfaces as I can NOT set same source address on
> them. However, if the interface is going down and up, then loopback
> route is deleted without checking IFA_RTSELF flag (it must be
> consistent! especially in kernel) and re-added regardless of
> "useloopback" setting. So, at least, a loopback route is installed
> even if useloopback is NOT allowed!
> 

   I hope the question does not offend you, but you do know the history
   behind IFA_RTSELF loopback route for each interface address, right ?

   The interface address loopback route is used for reaching the
   interface address within the system after the L2/L3 separation
   redesign, that's why "useloopback" setting is inapplicable. 

   The check in various code paths may have a bit of consistency issue,
   but "useloopback" setting does not apply here.

>
> After that I've continued with point-to-point interfaces on same net,
> i.e. I've work with un-numbered interfaces. Firstly, I could not set
> parallel links on them. The fix is following and is already
> commmitted:
> 
>  http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
> 

   I had a second look at it after some sleep, I agree with your fix.

>
> The bigger problem was with loopback routes on un-numbered
> interfaces. In in_ifinit(), when un-numbered interface is setting
> loopback route, then refcount on existing route is incremented and
> IFA_RTSELF flags is set on its address. This is done if and only if
> useloopback is set and interface is not IFF_LOOPBACK. It is OK. The
> rest is hacked somehow and I don't know why.
>

   The loopback route for the IFA should be installed unconditionally.
   So the check in in_ifinit() for "V_useloopback" needs to be removed.
 
>
> In in_ifscrubprefix() which is used either when address is being
> deleted or interface is going down, I found first inconsistence.
> Refcount on existing route is decremented always (in both cases), but
> the route is deleted only when address is being deleted.
>

   That's by design.

>
> Futhermore,
> IFA_RTSELF flag is NOT cleared when refcount is decremented! The fix
> and behavour is following:
> 
>  http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
> 

   I agree with your fix.

   <snip>

> 
> In the view of this inconsistence, I understand a next one in
> rip_ctlinput(). When interface is going up, then loopback route is
> being deleted and re-added regardless of IFA_RTSELF flag and
> useloopback setting. If un-numbered interfaces are used, then it
> damages refcount on existing loopback route!!
> 

   I will fix that.

> 
> If useloopback IS NOT set and IFA_RTSELF IS set, then loopback route
> is deleted, but with correct refcount game. And if useloopback is SET
> and IFA_RTSELF IS NOT set and interface IS NOT IFF_LOOPBACK, then
> loopback route is added, but again with correct refcount game too. It
> (with previous patch) should ensure IFA_RTSELF and loopback route
> consistence.
> 

   No, see above, the IFA_RTSELF route should be unconditionally.

   I agree with you about the consistency issue and will fix it.

   <snip>

>>
>> Unless you have a really good reason, other than code inspection,
>> and have a set of test cases, please leave this code alone for now. 
> 
> I have good reason, but I can hack kernel just for me only in worse
> scenario. However, I always try to minimalize the hacks count.
> 

   You can hack the kernel however you see fit, but when you are 
   ready for a patch commit, please provide sufficient context and
   problem description, and test cases whenever possible to make the 
   code review process effective.

   <snip>

> 
> I understand, but I use my own DHCP client. Well, I try to look at it,
> but maybe, someone else can test it.
> 

   Again, my only point is, since these areas are core to the networking 
   kernel, please test as many scenarios as possible, more than just your 
   specific setup. (I made this mistake myself sometimes.)

   In any case, thank you very much for your fixes.

   -- Qing