Quantcast

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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Svatopluk Kraus
I have two NIC on same NET (both are up). If a NIC which installs
network route is going down then an error happens during network route
replacement (in_scrubprefix: err=17, new prefix add failed).

  I've done a little bit investigation. In rtinit1(), before
rtrequest1_fib() is called, info.rti_flags is initialized by flags
(function argument) or-ed with ifa->ifa_flags. Both NIC has a loopback
route to itself, so IFA_RTSELF is set on ifa(s). As IFA_RTSELF is
defined by RTF_HOST, rtrequest1_fib() is called with RTF_HOST flag
even if netmask is not NULL. Consequently, netmask is set to zero in
rtrequest1_fib(), and request to add network route is changed under
hands to request to add host route. It is the reason of logged info
and my problem.

  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.

Index: sys/net/route.c
===================================================================
--- sys/net/route.c (revision 224635)
+++ sys/net/route.c (working copy)
@@ -1478,7 +1478,7 @@
  */
  bzero((caddr_t)&info, sizeof(info));
  info.rti_ifa = ifa;
- info.rti_flags = flags | ifa->ifa_flags;
+ info.rti_flags = flags | (ifa->ifa_flags & ~IFA_RTSELF);
  info.rti_info[RTAX_DST] = dst;
  /*
  * doing this for compatibility reason


  Is the patch sufficient?

      Svata
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Boyer-2
We found and fixed a similar issue with an identical patch.  It has been working fine for us under stable/8.

Unfortunately I am weeks and weeks behind on pushing fixes back to the tree, so you had to duplicate the work.  If it can be committed (and MFC'd to 8, please) it would save others the trouble.

-Andrew

On Aug 3, 2011, at 10:51 AM, Svatopluk Kraus wrote:

> I have two NIC on same NET (both are up). If a NIC which installs
> network route is going down then an error happens during network route
> replacement (in_scrubprefix: err=17, new prefix add failed).
>
>  I've done a little bit investigation. In rtinit1(), before
> rtrequest1_fib() is called, info.rti_flags is initialized by flags
> (function argument) or-ed with ifa->ifa_flags. Both NIC has a loopback
> route to itself, so IFA_RTSELF is set on ifa(s). As IFA_RTSELF is
> defined by RTF_HOST, rtrequest1_fib() is called with RTF_HOST flag
> even if netmask is not NULL. Consequently, netmask is set to zero in
> rtrequest1_fib(), and request to add network route is changed under
> hands to request to add host route. It is the reason of logged info
> and my problem.
>
>  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.
>
> Index: sys/net/route.c
> ===================================================================
> --- sys/net/route.c (revision 224635)
> +++ sys/net/route.c (working copy)
> @@ -1478,7 +1478,7 @@
> */
> bzero((caddr_t)&info, sizeof(info));
> info.rti_ifa = ifa;
> - info.rti_flags = flags | ifa->ifa_flags;
> + info.rti_flags = flags | (ifa->ifa_flags & ~IFA_RTSELF);
> info.rti_info[RTAX_DST] = dst;
> /*
> * doing this for compatibility reason
>
>
>  Is the patch sufficient?
>
>      Svata
> _______________________________________________
> [hidden email] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "[hidden email]"

--------------------------------------------------
Andrew Boyer [hidden email]




_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kevin Lo-2
Hi Andrew,

I just committed Svatopluk's fix to HEAD, thanks!

        Kevin

On Wed, 2011-08-03 at 11:11 -0400, Andrew Boyer wrote:

> We found and fixed a similar issue with an identical patch.  It has been working fine for us under stable/8.
>
> Unfortunately I am weeks and weeks behind on pushing fixes back to the tree, so you had to duplicate the work.  If it can be committed (and MFC'd to 8, please) it would save others the trouble.
>
> -Andrew
>
> On Aug 3, 2011, at 10:51 AM, Svatopluk Kraus wrote:
>
> > I have two NIC on same NET (both are up). If a NIC which installs
> > network route is going down then an error happens during network route
> > replacement (in_scrubprefix: err=17, new prefix add failed).
> >
> >  I've done a little bit investigation. In rtinit1(), before
> > rtrequest1_fib() is called, info.rti_flags is initialized by flags
> > (function argument) or-ed with ifa->ifa_flags. Both NIC has a loopback
> > route to itself, so IFA_RTSELF is set on ifa(s). As IFA_RTSELF is
> > defined by RTF_HOST, rtrequest1_fib() is called with RTF_HOST flag
> > even if netmask is not NULL. Consequently, netmask is set to zero in
> > rtrequest1_fib(), and request to add network route is changed under
> > hands to request to add host route. It is the reason of logged info
> > and my problem.
> >
> >  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.
> >
> > Index: sys/net/route.c
> > ===================================================================
> > --- sys/net/route.c (revision 224635)
> > +++ sys/net/route.c (working copy)
> > @@ -1478,7 +1478,7 @@
> > */
> > bzero((caddr_t)&info, sizeof(info));
> > info.rti_ifa = ifa;
> > - info.rti_flags = flags | ifa->ifa_flags;
> > + info.rti_flags = flags | (ifa->ifa_flags & ~IFA_RTSELF);
> > info.rti_info[RTAX_DST] = dst;
> > /*
> > * doing this for compatibility reason
> >
> >
> >  Is the patch sufficient?
> >
> >      Svata
> > _______________________________________________
> > [hidden email] mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
> > To unsubscribe, send any mail to "[hidden email]"
>
> --------------------------------------------------
> Andrew Boyer [hidden email]
>
>
>
>
> _______________________________________________
> [hidden email] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "[hidden email]"


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Svatopluk Kraus
Thanks for committing the fix.

I've continued with work on two NIC on same NET. Now, with
point-to-point interfaces too and I have more small fixes which I
submitted today:

http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
http://www.freebsd.org/cgi/query-pr.cgi?pr=159602
http://www.freebsd.org/cgi/query-pr.cgi?pr=159603

I have one more related problem, but I'm not sure how complex the fix should be.

When an interface is marked down a network route is deleted (or
replaced) and a loopback route persists in routing table. It is OK.
However, when an interface is marked up again, then a network route is
installed unconditionally (but error is ignored) and a loopbak route
is deleted and added immediately and unconditionally too. IMHO, it is
not correct behaviour. I think that a second half of in_ifinit()
should be here starting by in_addprefix() call with some small or
bigger changes.

Maybe, adding network route and ignoring error could be OK, but
deleting loopback route should be done under IFA_RTSELF flag is set
condition (with existing route refcount check). V_useloopback should
be check before re-adding the route and existing route must be check
to evaluate refcount correctly. The proposed patch is attached.

However, I prefer to call in_addprefix() (which is static now) instead
of rtinit() and add some more checks from in_ifinit(). Can you (or
anyone) review the patch?

 Thanks once again,

    Svata


On Mon, Aug 8, 2011 at 7:28 AM, Kevin Lo <[hidden email]> wrote:

> Hi Andrew,
>
> I just committed Svatopluk's fix to HEAD, thanks!
>
>        Kevin
>
> On Wed, 2011-08-03 at 11:11 -0400, Andrew Boyer wrote:
>> We found and fixed a similar issue with an identical patch.  It has been working fine for us under stable/8.
>>
>> Unfortunately I am weeks and weeks behind on pushing fixes back to the tree, so you had to duplicate the work.  If it can be committed (and MFC'd to 8, please) it would save others the trouble.
>>
>> -Andrew
>>
>> On Aug 3, 2011, at 10:51 AM, Svatopluk Kraus wrote:
>>
>> > I have two NIC on same NET (both are up). If a NIC which installs
>> > network route is going down then an error happens during network route
>> > replacement (in_scrubprefix: err=17, new prefix add failed).
>> >
>> >  I've done a little bit investigation. In rtinit1(), before
>> > rtrequest1_fib() is called, info.rti_flags is initialized by flags
>> > (function argument) or-ed with ifa->ifa_flags. Both NIC has a loopback
>> > route to itself, so IFA_RTSELF is set on ifa(s). As IFA_RTSELF is
>> > defined by RTF_HOST, rtrequest1_fib() is called with RTF_HOST flag
>> > even if netmask is not NULL. Consequently, netmask is set to zero in
>> > rtrequest1_fib(), and request to add network route is changed under
>> > hands to request to add host route. It is the reason of logged info
>> > and my problem.
>> >
>> >  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.
>> >
>> > Index: sys/net/route.c
>> > ===================================================================
>> > --- sys/net/route.c (revision 224635)
>> > +++ sys/net/route.c (working copy)
>> > @@ -1478,7 +1478,7 @@
>> >              */
>> >             bzero((caddr_t)&info, sizeof(info));
>> >             info.rti_ifa = ifa;
>> > -           info.rti_flags = flags | ifa->ifa_flags;
>> > +           info.rti_flags = flags | (ifa->ifa_flags & ~IFA_RTSELF);
>> >             info.rti_info[RTAX_DST] = dst;
>> >             /*
>> >              * doing this for compatibility reason
>> >
>> >
>> >  Is the patch sufficient?
>> >
>> >      Svata
>> > _______________________________________________
>> > [hidden email] mailing list
>> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
>> > To unsubscribe, send any mail to "[hidden email]"
>>
>> --------------------------------------------------
>> Andrew Boyer  [hidden email]
>>
>>
>>
>>
>> _______________________________________________
>> [hidden email] mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-current
>> To unsubscribe, send any mail to "[hidden email]"
>
>
>

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"

patch_raw_ip.txt (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Svatopluk Kraus
Hi,

  after more testing of all matter related to two NIC on same net, I
just updated my submit
http://www.freebsd.org/cgi/query-pr.cgi?pr=159601

The patch I send here yesterday must be updated too. IFA_RTSELF flag
must be treat correctly on more places. The new patch is attached and
commented output of my testing too. To achieve same testing result
following patches must be used too:

http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
http://www.freebsd.org/cgi/query-pr.cgi?pr=159603

   Svata


On Mon, Aug 8, 2011 at 4:57 PM, Svatopluk Kraus <[hidden email]> wrote:

> Thanks for committing the fix.
>
> I've continued with work on two NIC on same NET. Now, with
> point-to-point interfaces too and I have more small fixes which I
> submitted today:
>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159602
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159603
>
> I have one more related problem, but I'm not sure how complex the fix should be.
>
> When an interface is marked down a network route is deleted (or
> replaced) and a loopback route persists in routing table. It is OK.
> However, when an interface is marked up again, then a network route is
> installed unconditionally (but error is ignored) and a loopbak route
> is deleted and added immediately and unconditionally too. IMHO, it is
> not correct behaviour. I think that a second half of in_ifinit()
> should be here starting by in_addprefix() call with some small or
> bigger changes.
>
> Maybe, adding network route and ignoring error could be OK, but
> deleting loopback route should be done under IFA_RTSELF flag is set
> condition (with existing route refcount check). V_useloopback should
> be check before re-adding the route and existing route must be check
> to evaluate refcount correctly. The proposed patch is attached.
>
> However, I prefer to call in_addprefix() (which is static now) instead
> of rtinit() and add some more checks from in_ifinit(). Can you (or
> anyone) review the patch?
>
>  Thanks once again,
>
>    Svata
>
>
> On Mon, Aug 8, 2011 at 7:28 AM, Kevin Lo <[hidden email]> wrote:
>> Hi Andrew,
>>
>> I just committed Svatopluk's fix to HEAD, thanks!
>>
>>        Kevin
>>
>> On Wed, 2011-08-03 at 11:11 -0400, Andrew Boyer wrote:
>>> We found and fixed a similar issue with an identical patch.  It has been working fine for us under stable/8.
>>>
>>> Unfortunately I am weeks and weeks behind on pushing fixes back to the tree, so you had to duplicate the work.  If it can be committed (and MFC'd to 8, please) it would save others the trouble.
>>>
>>> -Andrew
>>>
>>> On Aug 3, 2011, at 10:51 AM, Svatopluk Kraus wrote:
>>>
>>> > I have two NIC on same NET (both are up). If a NIC which installs
>>> > network route is going down then an error happens during network route
>>> > replacement (in_scrubprefix: err=17, new prefix add failed).
>>> >
>>> >  I've done a little bit investigation. In rtinit1(), before
>>> > rtrequest1_fib() is called, info.rti_flags is initialized by flags
>>> > (function argument) or-ed with ifa->ifa_flags. Both NIC has a loopback
>>> > route to itself, so IFA_RTSELF is set on ifa(s). As IFA_RTSELF is
>>> > defined by RTF_HOST, rtrequest1_fib() is called with RTF_HOST flag
>>> > even if netmask is not NULL. Consequently, netmask is set to zero in
>>> > rtrequest1_fib(), and request to add network route is changed under
>>> > hands to request to add host route. It is the reason of logged info
>>> > and my problem.
>>> >
>>> >  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.
>>> >
>>> > Index: sys/net/route.c
>>> > ===================================================================
>>> > --- sys/net/route.c (revision 224635)
>>> > +++ sys/net/route.c (working copy)
>>> > @@ -1478,7 +1478,7 @@
>>> >              */
>>> >             bzero((caddr_t)&info, sizeof(info));
>>> >             info.rti_ifa = ifa;
>>> > -           info.rti_flags = flags | ifa->ifa_flags;
>>> > +           info.rti_flags = flags | (ifa->ifa_flags & ~IFA_RTSELF);
>>> >             info.rti_info[RTAX_DST] = dst;
>>> >             /*
>>> >              * doing this for compatibility reason
>>> >
>>> >
>>> >  Is the patch sufficient?
>>> >
>>> >      Svata
>>> > _______________________________________________
>>> > [hidden email] mailing list
>>> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
>>> > To unsubscribe, send any mail to "[hidden email]"
>>>
>>> --------------------------------------------------
>>> Andrew Boyer  [hidden email]
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> [hidden email] mailing list
>>> http://lists.freebsd.org/mailman/listinfo/freebsd-current
>>> To unsubscribe, send any mail to "[hidden email]"
>>
>>
>>
>

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"

patch_raw_ip_new.txt (2K) Download Attachment
raw_ip_test.txt (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kevin Lo
In reply to this post by Svatopluk Kraus
On Mon, 2011-08-08 at 16:57 +0200, Svatopluk Kraus wrote:

> Thanks for committing the fix.
>
> I've continued with work on two NIC on same NET. Now, with
> point-to-point interfaces too and I have more small fixes which I
> submitted today:
>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159602
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159603
>
> I have one more related problem, but I'm not sure how complex the fix should be.
>
> When an interface is marked down a network route is deleted (or
> replaced) and a loopback route persists in routing table. It is OK.
> However, when an interface is marked up again, then a network route is
> installed unconditionally (but error is ignored) and a loopbak route
> is deleted and added immediately and unconditionally too. IMHO, it is
> not correct behaviour. I think that a second half of in_ifinit()
> should be here starting by in_addprefix() call with some small or
> bigger changes.
>
> Maybe, adding network route and ignoring error could be OK, but
> deleting loopback route should be done under IFA_RTSELF flag is set
> condition (with existing route refcount check). V_useloopback should
> be check before re-adding the route and existing route must be check
> to evaluate refcount correctly. The proposed patch is attached.
>
> However, I prefer to call in_addprefix() (which is static now) instead
> of rtinit() and add some more checks from in_ifinit(). Can you (or
> anyone) review the patch?

Hi Svatopluk,

Thanks for working so hard to figure the issue out.
kern/159600 looks good to me also, committed to HEAD.
The rest needs more review, thanks!

>  Thanks once again,
>
>     Svata

        Kevin


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Li, Qing
  Hi,

>
> I've continued with work on two NIC on same NET. Now, with
> point-to-point interfaces too and I have more small fixes which I
> submitted today:
>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
>

     The fix is not entirely correct. The "rtinitflags()" could set RTF_HOST flag when the interface
      type is IFF_LOOPBACK, not necessarily a PPP llink.

>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159602
> http://www.freebsd.org/cgi/query-pr.cgi?pr=159603
>

     I need to run some tests on your patch, but keep in mind the LLE_STATIC is sort overloaded
     to take care of the case where static routes are maintained in the routing table while dynamic
     routes are removed when the interface is taken down.

>
> I have one more related problem, but I'm not sure how complex the fix should be.
>
> When an interface is marked down a network route is deleted (or
> replaced) and a loopback route persists in routing table. It is OK.
> However, when an interface is marked up again, then a network route is
> installed unconditionally (but error is ignored) and a loopbak route
> is deleted and added immediately and unconditionally too. IMHO, it is
> not correct behaviour. I think that a second half of in_ifinit()
> should be here starting by in_addprefix() call with some small or
> bigger changes.
>

      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. See below ...

>
> Maybe, adding network route and ignoring error could be OK, but
> deleting loopback route should be done under IFA_RTSELF flag is set
> condition (with existing route refcount check). V_useloopback should
> be check before re-adding the route and existing route must be check
> to evaluate refcount correctly. The proposed patch is attached.
>

      Did you read my code comment in "in.c", at line 1115 ?

>
> However, I prefer to call in_addprefix() (which is static now) instead
> of rtinit() and add some more checks from in_ifinit(). Can you (or
> anyone) review the patch?
>
 
     There are quite a few cases to cover, including bootp, which takes a different code path
     than DHCP through the routing code. I would appreciate that you test these cases before
     making any code commits. It's taken some time to get the overall routing code stabilized.
     There is still a bug in the Radix code that needs fixing.

    -- Qing



_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Svatopluk Kraus
Hi,

 I try to desribe the whole matter. In the simplest case, I have two
routers - each with two un-numbered interfaces connected in parallel.
Real data flow is managed in hardware. IP layer is used for managing
the hardwares, so I only need to be able to communicate with neighbour
thru one from two links. It's not important which one is used, but if
one link is down then the other must be used (if it is up). Now, it
works on my table and the patches are made public.

 (To be perfectly honest, sometimes I need to send message thru given
interface to know that communication thru this link is possible, but
it's another - IP_SENDIF - story and I'm prepared to re-open a thread
on it. Well, I've used Bruce M Simpson IP_SENDIF patch on current and
it works really fine for me.)

 I've started my work with not point-to-point interfaces and I've
found two problems. The first one - I've mentioned it in the beginning
of this thread and the fix is committed already. The second one -
submitted patch and description is bellow:

 http://www.freebsd.org/cgi/query-pr.cgi?pr=159603

 If both interfaces are on same net and one (which installed network
route) is going down, then the network route is replaced by network
route of the other interface and everything is OK. I can access the
network thru the other interface. However, if the second interface is
going down too, then the network is replaced by network route of the
first interface even if it is down. It is not OK. Futhermore, if the
second interface is going up, then the correct (working) network route
is not installed as a network route with same prefix is installed
already but by interface which is down. So, I can't access the network
and it is bad really.

 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!

 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

 The test case is simple. When the second (parallel) link was being
set then network prefix (already installed by first interface) was not
found (lookup by source address instead of destination one) and
re-adding was failed obviously as the route really was installed.

 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.

 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. 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

 When two unnumbered interfaces are set and one of them is going down,
then refcount on loopback route is decremented, but IFA_RTSELF flags
remains set on interface address. It's inconsistent. After that, if
address on second interface is being deleted, then loopback route is
deleted too. No loopback route exists, but one interface has
IFA_RTSELF flag set.

 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!!

 IMHO, as loopback routes are not touched when interface is going
down, then the routes should be untouched if the interface is going up
too. It's the simple patch, but don't work without previous one.
However, useloopback complicates it. So, I'm proposing the following
patch:

 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.

 Well, it's genesis of the patches and I think it is good case to make
it working.

On Wed, Aug 10, 2011 at 10:48 AM, Li, Qing <[hidden email]> wrote:

>  Hi,
>
>>
>> I've continued with work on two NIC on same NET. Now, with
>> point-to-point interfaces too and I have more small fixes which I
>> submitted today:
>>
>> http://www.freebsd.org/cgi/query-pr.cgi?pr=159600
>>
>
>     The fix is not entirely correct. The "rtinitflags()" could set RTF_HOST flag when the interface
>      type is IFF_LOOPBACK, not necessarily a PPP llink.

In fact, I'm not really happy with rtinitflags() used in places, where
RTF_HOST flag should be tested. However, even if it is not my code, I
understand that the thing with IFF_LOOPBACK could be problem.

>
>>
>> http://www.freebsd.org/cgi/query-pr.cgi?pr=159601
>> http://www.freebsd.org/cgi/query-pr.cgi?pr=159602
>> http://www.freebsd.org/cgi/query-pr.cgi?pr=159603
>>
>
>     I need to run some tests on your patch, but keep in mind the LLE_STATIC is sort overloaded
>     to take care of the case where static routes are maintained in the routing table while dynamic
>     routes are removed when the interface is taken down.

Yes, I think that LLE_STATIC is a little bit misused too, but I try to
make my patch consistent with current implementation.

>
>>
>> I have one more related problem, but I'm not sure how complex the fix should be.
>>
>> When an interface is marked down a network route is deleted (or
>> replaced) and a loopback route persists in routing table. It is OK.
>> However, when an interface is marked up again, then a network route is
>> installed unconditionally (but error is ignored) and a loopbak route
>> is deleted and added immediately and unconditionally too. IMHO, it is
>> not correct behaviour. I think that a second half of in_ifinit()
>> should be here starting by in_addprefix() call with some small or
>> bigger changes.
>>
>
>      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. See below ...

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.

>
>>
>> Maybe, adding network route and ignoring error could be OK, but
>> deleting loopback route should be done under IFA_RTSELF flag is set
>> condition (with existing route refcount check). V_useloopback should
>> be check before re-adding the route and existing route must be check
>> to evaluate refcount correctly. The proposed patch is attached.
>>
>
>      Did you read my code comment in "in.c", at line 1115 ?

Yes, I've read it. However, it's really done only when an interface
address is being deleted and LLE_STATIC is given. If an interface is
going down, then loopback route is untouched (except of mentioned
inconsistence with refcount).

>
>>
>> However, I prefer to call in_addprefix() (which is static now) instead
>> of rtinit() and add some more checks from in_ifinit(). Can you (or
>> anyone) review the patch?
>>
>
>     There are quite a few cases to cover, including bootp, which takes a different code path
>     than DHCP through the routing code. I would appreciate that you test these cases before
>     making any code commits. It's taken some time to get the overall routing code stabilized.
>     There is still a bug in the Radix code that needs fixing.
>
>    -- Qing

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

     Svata
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Li, Qing
  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

 




_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Svatopluk Kraus
Hi,

  thank you very much for your review and related fixes.

On Fri, Aug 12, 2011 at 3:48 AM, Li, Qing <[hidden email]> wrote:

>>  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.

In fact, I don't know the history, so you question is quite in place.
I always try to find more about problems that I'm solving. However,
sometimes it is not easy to find all things about an issue. Sometimes,
I miss the ideas behind to figure out the issue in clear view. That's
why I've started this discussion. So, maybe I'm not perfect, but I'm
trying to learn. ;))

>> 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.

It is clear now, but I don't know that when I was proposing the patch.
I saw "V_useloopback" in in_ifinit(), so I put it to my patch too.

>>> 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.

I understand.

>   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.)

I undestand and once again,  thanks,

     Svata
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Loading...