Date: Fri, 12 Aug 2011 01:48:49 +0000 From: "Li, Qing" <qing.li@bluecoat.com> To: Svatopluk Kraus <onwahe@gmail.com>, Qing Li <qingli@freebsd.org> Cc: Jeremiah Lott <jlott@averesystems.com>, Kevin Lo <kevlo@kevlo.org>, Andrew Boyer <aboyer@averesystems.com>, "freebsd-current@freebsd.org" <freebsd-current@freebsd.org> Subject: RE: [patch] Problem with two NIC on same NET (in_scrubprefix: err=17, new prefix add failed) Message-ID: <B143A8975061C446AD5E29742C5317230E549A@PWSVL-EXCMBX-01.internal.cacheflow.com> In-Reply-To: <CAFHCsPVTVw4eMJCCfdUFrJ3zLHtD_9jW1zQKByd6=60croctnw@mail.gmail.com> References: <CAFHCsPUpkr-vne%2B9cLnovUXDGeVrOzHrKp1YAub=TjJW_3aVtg@mail.gmail.com> <92B5D566-9816-4134-9358-2306D0F7DAFC@averesystems.com> <1312781293.2521.1.camel@srgsec> <CAFHCsPVM1iGehjt8AAiwhQvW81GqLjjf7SO_-OnWzYBadQ0jmw@mail.gmail.com> <1312960023.2614.12.camel@srgsec> <B143A8975061C446AD5E29742C5317230E16CD@PWSVL-EXCMBX-01.internal.cacheflow.com> <CAFHCsPVTVw4eMJCCfdUFrJ3zLHtD_9jW1zQKByd6=60croctnw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
>=20
> I've started my work with not point-to-point interfaces and I've
> found two problems. The first one -=20
>
<snip>
>=20
> When I've done more investigation, it looks similar to
> http://svnweb.freebsd.org/base?view=3Drevision&revision=3D201543
> 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.=20
Turned out I had a note-to-self to verify if there are other similar=20
problems at the time but busy day job took me away ...
>
> The second one - submitted patch and description is bellow:
>=20
> http://www.freebsd.org/cgi/query-pr.cgi?pr=3D159603
>=20
I agree with your fix.
<snip>
>=20
> 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!
>=20
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.=20
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:
>=20
> http://www.freebsd.org/cgi/query-pr.cgi?pr=3D159600
>=20
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.
=20
>
> 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:
>=20
> http://www.freebsd.org/cgi/query-pr.cgi?pr=3D159601
>=20
I agree with your fix.
<snip>
>=20
> 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!!
>=20
I will fix that.
>=20
> 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.
>=20
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.=20
>=20
> 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.
>=20
You can hack the kernel however you see fit, but when you are=20
ready for a patch commit, please provide sufficient context and
problem description, and test cases whenever possible to make the=20
code review process effective.
<snip>
>=20
> I understand, but I use my own DHCP client. Well, I try to look at it,
> but maybe, someone else can test it.
>=20
Again, my only point is, since these areas are core to the networking=20
kernel, please test as many scenarios as possible, more than just your=20
specific setup. (I made this mistake myself sometimes.)
In any case, thank you very much for your fixes.
-- Qing
=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B143A8975061C446AD5E29742C5317230E549A>