linux-kernel - Re: [PATCH v3] net: sysctl for RA default route MTU

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] [day] [month] [year] [list]

Message-Id: <211371428519836@webcorp02f.yandex-team.ru>
Date:	Wed, 08 Apr 2015 22:03:56 +0300
From:	Roman Gushchin <klamm@...dex-team.ru>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] net: sysctl for RA default route MTU

07.04.2015, 18:58, "Hannes Frederic Sowa" <hannes@...essinduktion.org>:
>  On Do, 2015-04-02 at 21:08 +0300, Roman Gushchin wrote:
>>>>    The next question I have is about the behavior of the new setting
>>>>    in the presence of an RA MTU option.  It seems like the sysctl
>>>>    doesn't override that RA MTU option, but rather just clamps it.
>>>>
>>>>    And then if it's in range, this controls only whether the default
>>>>    route has it's MTU adjusted.
>>>>
>>>>    That doesn't make any sense to me if we then go and do the
>>>>    rt6_mtu_change() call unconditionally.  The route metric update
>>>>    and the rt6_mtu_change() go hand in hand.
>>>   Agreed but that gets interesting:
>>>
>>>   I guess during testing the cnf.mtu6 value was equal to the newly
>>>   announced mtu value, so the rt6_mtu_change call does not happen. We
>>>   update cnf.mtu6 so a second RA packet would actually bring the system
>>>   into the desired state but we have a moment where the default route
>>>   carries a too big MTU. That's not good.
>>   Agreed.
>>>   Easiest solution is to reorder those calls but that also leaves us with
>>>   a time frame where we carry the incorrect MTU on the default route.
>>>   Otherwise we must conditionally filter out the default routes.
>>>   Roman, any ideas?
>>   I think, such approach will work on practise, but looks not very beatiful.
>>
>>   May be, a better idea is to serarate per-route and per-device MTU,
>>   so an updating of per-device MTU will not affect per-route MTU.
>>   Actual MTU can always been calculated as min(route_mtu, device_mtu),
>>   but we wouldn't need to update mtu on each route on receiving RA MTU option,
>>   for instance.
>>
>>   Do you see any problems with such approach?
>  If I understood you correct this actually seems to be quite an intrusive
>  change? :/ Can you show me some code how to do this?

Too intrusive, really)

>  I would also dislike adding a filtering capability to the route mtu
>  updates. Currently I don't have a god idea, sorry.

Hmm, I thought a bit more about this issue... And It seems to me now, that there is no issue at all.
If RA MTU is larger than ra_default_route_mtu, rt6_mtu_change() will not update it,
because dst_mtu(&rt->dst) != idev->cnf.mtu6 :
	if (rt->dst.dev == arg->dev &&
	    !dst_metric_locked(&rt->dst, RTAX_MTU) &&
	    (dst_mtu(&rt->dst) >= arg->mtu ||
	     (dst_mtu(&rt->dst) < arg->mtu &&
	      dst_mtu(&rt->dst) == idev->cnf.mtu6))) {
		dst_metric_set(&rt->dst, RTAX_MTU, arg->mtu);
	}
So, it's ok.

Otherwise, if RA MTU is lower than ra_default_route_mtu, rt6_mtu_change() will lower default route mtu, and it's ok too. There is a short period of time, when a newly created default route has too large MTU, but it's not scary. And it's exactly as it works now if new RA advertise MTU smaller than previous.

Do I miss something?

Thanks!

Regards,
Roman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/