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]
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

Powered by blists - more mailing lists