netdev - Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F

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:	Tue, 07 Jan 2014 23:54:22 +0100
From:	Thomas Haller <thaller@...hat.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
	stephen@...workplumber.org, dcbw@...hat.com
Subject: Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for
 IFA_F_NOPREFIXROUTE

On Tue, 2014-01-07 at 20:01 +0100, Hannes Frederic Sowa wrote:
> On Tue, Jan 07, 2014 at 07:32:57PM +0100, Thomas Haller wrote:
> > On Tue, 2014-01-07 at 17:28 +0100, Hannes Frederic Sowa wrote:
> > > On Tue, Jan 07, 2014 at 03:39:13PM +0100, Thomas Haller wrote:
> > > > Also, when adding the NOPREFIXROUTE flag to an already existing address,
> > > > check if there there is a prefix that was likly added by the kernel
> > > > and delete it.
> > > 
> > > Hmm, could you give a bit more details why you have done this? I find
> > > that a bit counterintuitive. Maybe it has a reason?
> > > 
> > 
> > You find the behavior or the commit message counterintuitive? Didn't you
> > suggest this behavior in your email from "7 Jan 2014 13:01:11 +0100"?
> 
> I guess I was a bit confused, sorry. I think I confused the deleted and modify
> case. However:
> 
> So we have the following changes on addresses:
> 
> add is simple: just as in the first patch
> 
> modify: is a bit hairy. To be extremly exact, we would have to recreate the
> 	route with proper metrics etc. so delete in any case and reinsert.
> 	I really dislike removing a route someone else might have inserted
> 	manually, and this is a likely scenario.
> 
> 	Somehow I tend to just don't allow NOPREFIXROUTE on modify at all and
> 	just return a proper error value. What do you think? What would be the
> 	best behavior for NM?
> 
> delete: if IFA_F_NOPREFIXROUTE is set, we don't care about removing a prefix
> 	route, it must be set by user space and should get cleaned up by user
> 	space
> 
> > 
> > 
> > For v3 I will reword the commit message. How about the following:
> > ...
> 
> If we want go with the current modify behavior this sounds good.


Hi,


I think, the modify case is not that hairy and the patch does IMO the
sensible thing:

case 1) "change NOPREFIXROUTE -> !NOPREFIXROUTE":
    update or add prefix route (as before);;
case 2) "change !NOPREFIXROUTE -> !NOPREFIXROUTE":
    update or add prefix route (as before);;
case 3) "change NOPREFIXROUTE -> NOPREFIXROUTE":
    ;;
case 4) "change !NOPREFIXROUTE -> NOPREFIXROUTE":
    cleanup prefix route;;

where "cleanup" means the same as done in ipv6_del_addr(), as determined
by check_cleanup_prefix_routes().


Allowing modify with case 2) and 3) is important. But for case 4) (and
possibly 1)), we could also fail with error. I tend to the scheme above
though because it makes it easier for userspace and is likely what it
wants.



The problem of deleting a route created by somebody else is already
present without this patch in ipv6_del_addr. This is indeed a bit shaky,
but I guess it's good enough in practice. Do I understand correctly,
that you think about to use the information from ifp->rt to ensure, that
what we really cleanup the correct route? If that's what you intend, can
you elaborate a bit on how to do that?


ciao,
Thomas


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)