linux-kernel - Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d1sdye0x.fsf@vitty.brq.redhat.com>
Date:	Wed, 03 Feb 2016 17:06:22 +0100
From:	Vitaly Kuznetsov <vkuznets@...hat.com>
To:	Haiyang Zhang <haiyangz@...rosoft.com>
Cc:	"davem\@davemloft.net" <davem@...emloft.net>,
	"netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
	KY Srinivasan <kys@...rosoft.com>,
	"olaf\@aepfle.de" <olaf@...fle.de>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	"driverdev-devel\@linuxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>
Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

Haiyang Zhang <haiyangz@...rosoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
>> Sent: Wednesday, February 3, 2016 8:06 AM
>> To: Haiyang Zhang <haiyangz@...rosoft.com>
>> Cc: davem@...emloft.net; netdev@...r.kernel.org; KY Srinivasan
>> <kys@...rosoft.com>; olaf@...fle.de; linux-kernel@...r.kernel.org;
>> driverdev-devel@...uxdriverproject.org
>> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
>> RNDIS_STATUS_NETWORK_CHANGE
>> 
>> Haiyang Zhang <haiyangz@...rosoft.com> writes:
>> 
>> > We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE
>> message to
>> > trigger DHCP renew. User daemons may need multiple seconds to trigger
>> the
>> > link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
>> > this link down period to 10 sec to properly trigger DHCP renew.
>> >
>> 
>> I probably don't follow: why do we need sucha a delay? If (with real
>> hardware) you plug network cable out and in one second you plug it in
>> you'll get DHCP renewed, right?
>> 
>> When I introduced RNDIS_STATUS_NETWORK_CHANGE handling by
>> emulating a
>> pair of up/down events I put 2 second delay to make link_watch happy (as
>> we only handle 1 event per second there) but 10 seconds sounds to much
>> to me.
>
> In the test on Hyper-V, our software on host side  wants to trigger DHCP 
> renew by sending only a RNDIS_STATUS_NETWORK_CHANGE message to 
> a guest without physically unplug the cable. But, this message didn't trigger 
> DHCP renew within 2 seconds. The VM is Centos 7.1 using Networkmanager, 
> which needs 4 seconds after link down to renew IP. Some daemon, like 
> ifplugd, needs 5 sec to renew. That's why we increase the simulated link 
> down time for RNDIS_STATUS_NETWORK_CHANGE message.

Yes, I understand the motivation but sorry if I was unclear with my
question. I meant to say that with physical network adapters it's
possible to trigger same two events by plugging your cable out and then
plugging it back in and it is certailnly doable in less than 10
seconds. NetworkManager or whoever is supposed to handle these events
and we don't really care how fast -- I think that 4 or 5 seconds
mentioned above is just an observation.

>
>> > @@ -1009,8 +1012,11 @@ static void netvsc_link_change(struct
>> work_struct *w)
>> >  			netif_tx_stop_all_queues(net);
>> >  			event->event = RNDIS_STATUS_MEDIA_CONNECT;
>> >  			spin_lock_irqsave(&ndev_ctx->lock, flags);
>> > -			list_add_tail(&event->list, &ndev_ctx-
>> >reconfig_events);
>> > +			list_add(&event->list, &ndev_ctx->reconfig_events);
>> 
>> Why? Adding to tail was here to not screw the order of events...
>
> The RNDIS_STATUS_MEDIA_CONNECT message triggers two "half" events -- 
> link down & up. After "link down", we want the paired "link up" to be the 
> immediate next event. Since the function picks the next event from the list 
> head, so it should be inserted to list head. Otherwise, the possible existing 
> events in the list will be processed in the middle of these two "half events" 
> -- link down & up.

Ah, yes, you're probably right.

-- 
  Vitaly