Re: CVS commit: src/sys

[Roy Marples <roy%NetBSD.org@localhost>]

On 13/09/2016 02:28, Ryota Ozaki wrote:
> On Mon, Sep 12, 2016 at 6:28 PM, Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost> wrote:
>> Hi,
>>
>> On 2016/09/12 17:28, Roy Marples wrote:
>>> On 09/09/2016 20:08, Martin Husemann wrote:
>>>> On Thu, Sep 08, 2016 at 02:19:15PM +0900, Kengo NAKAHARA wrote:
>>>>>
>>>>> On 2016/09/08 0:41, Roy Marples wrote:
>>>>>> Module Name:       src
>>>>>> Committed By:      roy
>>>>>> Date:              Wed Sep  7 15:41:44 UTC 2016
>>>>>>
>>>>>> Modified Files:
>>>>>>    src/sys/netinet: ip_input.c
>>>>>>    src/sys/netinet6: ip6_input.c
>>>>>>
>>>>>> Log Message:
>>>>>> Disallow input to detached addresses because they are not yet valid.
>>>>>>
>>>>>>
>>>>>> To generate a diff of this commit:
>>>>>> cvs rdiff -u -r1.340 -r1.341 src/sys/netinet/ip_input.c
>>>>>> cvs rdiff -u -r1.167 -r1.168 src/sys/netinet6/ip6_input.c
>>>>>>
>>>>>> Please note that diffs are not public domain; they are subject to the
>>>>>> copyright notices on the relevant files.
>>>>>
>>>>> ATF net/if_pppoe/t_pppoe:pap fails after this commit. I revert this
>>>>> commit locally, and then the ATF become successful.
>>>>
>>>> I see the failure too, but manually configuring two rump servers in a very
>>>> similar setup than the test still works for me - something must be wrong
>>>> in the test, but I don't see it right now.
>>>
>>> martin@ did more testing and discovered that the PPP connection works
>>> fine, so something is wrong in the test.
>>>
>>> I believe that the function wait_for_session_established needs an
>>> additional pause to wait for the detached flag to clear.
>>> If the interface is marked IFF_UP at this point, it should be possible
>>> to add ifconfig -w 5 -W -5 and the test should then succeed. I'm getting
>>> symbol errors still trying to run this test so cannot fix it myself
>>> right now.
>>
>> I add ifconfig -w 15 -W 15 in wait_for_session_established(), however
>> net/if_pppoe/t_pppoe:pap still fails.
>>
>> Hmm, does my below modification match your pointing out, doesn't it?
> 
> It seems that DAD doesn't run on ifconfig pppoe0 up
> and pppoe0 remains as detached.
> 
> The following processing is happening:
> - ifconfig up calls ioctl(SIOCSIFFLAGS)
>   that calls ifioctl_common normally
> - ifioctl_common schedules DAD eventually
>   but only if IFF_UP & IFF_RUNNING
>   (see in_if_link_up)
> - However, some interfaces including pppoe set
>   IFF_RUNNING *after* calling ifioctl_common
> - Such interfaces don't run DAD
>   and remain as detached
> 
> I don't know how to fix it properly.

OK, I've had a quick look over the code.
It looks like IFF_RUNNING is used to indicate whether a PPP session is
currently running or not as opposed to hardware resources being allocated.

It strikes me that the easiest fix is to set IFF_RUNNING before calling
ifioctl_common if going_up and clear it if there is an error.

We should also hook the interface link state code into ppp as well
because we should know when the link is ready for use.

Any reason why this cannot be done?

Roy