netdev - Re: [PATCH net-next v5] mpls: support for dead routes

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: <5659468E.5080905@cumulusnetworks.com>
Date:	Fri, 27 Nov 2015 22:15:42 -0800
From:	roopa <roopa@...ulusnetworks.com>
To:	David Miller <davem@...emloft.net>
CC:	ebiederm@...ssion.com, rshearma@...cade.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5] mpls: support for dead routes

On 11/25/15, 8:18 AM, David Miller wrote:
> From: Roopa Prabhu <roopa@...ulusnetworks.com>
> Date: Tue, 24 Nov 2015 15:22:22 -0800
>
>> v4 -v5
>> 	- if kmemdup fails, modify the original route in place. This is a
>> 	corner case and only side effect is that in the remote case
>> 	of kmemdup failure, the changes will not be atomically visible
>> 	to datapath.
> I really don't like this.
>
> Either you need to make the changes appear atomic to the data path,
> and you therefore must fail the operation if kmemdup() fails, or it
> doesn't matter and you should just always change the route in-place.
>
> As far as I can tell it can't possibly matter.  The alive counter is
> never modified by the data path, it is only tested to make a multipath
> decision.  Likewise it's rather harmless to send a frame or two via a
> device currently going down.
>
> But if you're convinced it matters, then is matters, and you can't
> fake things when kmemdup() fails.  And in that case I would recommend
> that you use a two pass algorithm, one pass allocates all of the
> new routes, and the second fills them in, inserts them, and frees
> the old ones.
>
> That is the only way you can unwind and fail cleanly.
>
> And oh yeah, that's right, you can't really fail this and make the
> ifdown not proceed.
>
> So you're stuck, right?
>
> That's why this has to be designed in a way where memory allocations
> are not necessary.  These notifiers aren't really designed to facilitate
> situations that require resource acquisitions that can fail.
>
Get your point. I am not convinced that the atomic update matters much for the transient case.
 I was trying to accommodate comments i got during the review and it seemed ok to cover both
cases by optionally doing the atomic update when we can. But, I get your position on this.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html