netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
@ 2014-01-02 15:34 Jiri Pirko
  2014-01-02 15:34 ` [patch iproute2 v2 1/2] add support for extended ifa_flags Jiri Pirko
                   ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Jiri Pirko @ 2014-01-02 15:34 UTC (permalink / raw)
  To: netdev; +Cc: stephen, thaller

v1->v2: Removed 0xff masking of ifa_flags

Jiri Pirko (2):
  add support for extended ifa_flags
  add support for IFA_F_MANAGETEMPADDR

 include/linux/if_addr.h |  2 ++
 ip/ipaddress.c          | 50 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 38 insertions(+), 14 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [patch iproute2 v2 1/2] add support for extended ifa_flags
  2014-01-02 15:34 [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
@ 2014-01-02 15:34 ` Jiri Pirko
  2014-01-02 15:34 ` [patch iproute2 v2 2/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
  2014-01-02 17:29 ` [patch iproute2 v2 0/2] " Hannes Frederic Sowa
  2 siblings, 0 replies; 30+ messages in thread
From: Jiri Pirko @ 2014-01-02 15:34 UTC (permalink / raw)
  To: netdev; +Cc: stephen, thaller

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/linux/if_addr.h |  1 +
 ip/ipaddress.c          | 44 ++++++++++++++++++++++++++++++--------------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/include/linux/if_addr.h b/include/linux/if_addr.h
index 58b39f4..cced59f 100644
--- a/include/linux/if_addr.h
+++ b/include/linux/if_addr.h
@@ -28,6 +28,7 @@ enum {
 	IFA_ANYCAST,
 	IFA_CACHEINFO,
 	IFA_MULTICAST,
+	IFA_FLAGS,
 	__IFA_MAX,
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index d02eaaf..1e3f22c 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -541,6 +541,13 @@ static int set_lifetime(unsigned int *lifetime, char *argv)
 	return 0;
 }
 
+static unsigned int get_ifa_flags(struct ifaddrmsg *ifa,
+				  struct rtattr *ifa_flags_attr)
+{
+	return ifa_flags_attr ? rta_getattr_u32(ifa_flags_attr) :
+				ifa->ifa_flags;
+}
+
 int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		   void *arg)
 {
@@ -567,6 +574,8 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 
 	parse_rtattr(rta_tb, IFA_MAX, IFA_RTA(ifa), n->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa)));
 
+	ifa_flags = get_ifa_flags(ifa, rta_tb[IFA_FLAGS]);
+
 	if (!rta_tb[IFA_LOCAL])
 		rta_tb[IFA_LOCAL] = rta_tb[IFA_ADDRESS];
 	if (!rta_tb[IFA_ADDRESS])
@@ -576,7 +585,7 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		return 0;
 	if ((filter.scope^ifa->ifa_scope)&filter.scopemask)
 		return 0;
-	if ((filter.flags^ifa->ifa_flags)&filter.flagmask)
+	if ((filter.flags ^ ifa_flags) & filter.flagmask)
 		return 0;
 	if (filter.label) {
 		SPRINT_BUF(b1);
@@ -670,36 +679,35 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 				    abuf, sizeof(abuf)));
 	}
 	fprintf(fp, "scope %s ", rtnl_rtscope_n2a(ifa->ifa_scope, b1, sizeof(b1)));
-	ifa_flags = ifa->ifa_flags;
-	if (ifa->ifa_flags&IFA_F_SECONDARY) {
+	if (ifa_flags & IFA_F_SECONDARY) {
 		ifa_flags &= ~IFA_F_SECONDARY;
 		if (ifa->ifa_family == AF_INET6)
 			fprintf(fp, "temporary ");
 		else
 			fprintf(fp, "secondary ");
 	}
-	if (ifa->ifa_flags&IFA_F_TENTATIVE) {
+	if (ifa_flags & IFA_F_TENTATIVE) {
 		ifa_flags &= ~IFA_F_TENTATIVE;
 		fprintf(fp, "tentative ");
 	}
-	if (ifa->ifa_flags&IFA_F_DEPRECATED) {
+	if (ifa_flags & IFA_F_DEPRECATED) {
 		ifa_flags &= ~IFA_F_DEPRECATED;
 		deprecated = 1;
 		fprintf(fp, "deprecated ");
 	}
-	if (ifa->ifa_flags&IFA_F_HOMEADDRESS) {
+	if (ifa_flags & IFA_F_HOMEADDRESS) {
 		ifa_flags &= ~IFA_F_HOMEADDRESS;
 		fprintf(fp, "home ");
 	}
-	if (ifa->ifa_flags&IFA_F_NODAD) {
+	if (ifa_flags & IFA_F_NODAD) {
 		ifa_flags &= ~IFA_F_NODAD;
 		fprintf(fp, "nodad ");
 	}
-	if (!(ifa->ifa_flags&IFA_F_PERMANENT)) {
+	if (!(ifa_flags & IFA_F_PERMANENT)) {
 		fprintf(fp, "dynamic ");
 	} else
 		ifa_flags &= ~IFA_F_PERMANENT;
-	if (ifa->ifa_flags&IFA_F_DADFAILED) {
+	if (ifa_flags & IFA_F_DADFAILED) {
 		ifa_flags &= ~IFA_F_DADFAILED;
 		fprintf(fp, "dadfailed ");
 	}
@@ -926,6 +934,8 @@ static void ipaddr_filter(struct nlmsg_chain *linfo, struct nlmsg_chain *ainfo)
 		for (a = ainfo->head; a; a = a->next) {
 			struct nlmsghdr *n = &a->h;
 			struct ifaddrmsg *ifa = NLMSG_DATA(n);
+			struct rtattr *tb[IFA_MAX + 1];
+			unsigned int ifa_flags;
 
 			if (ifa->ifa_index != ifi->ifi_index)
 				continue;
@@ -934,11 +944,13 @@ static void ipaddr_filter(struct nlmsg_chain *linfo, struct nlmsg_chain *ainfo)
 				continue;
 			if ((filter.scope^ifa->ifa_scope)&filter.scopemask)
 				continue;
-			if ((filter.flags^ifa->ifa_flags)&filter.flagmask)
+
+			parse_rtattr(tb, IFA_MAX, IFA_RTA(ifa), IFA_PAYLOAD(n));
+			ifa_flags = get_ifa_flags(ifa, tb[IFA_FLAGS]);
+
+			if ((filter.flags ^ ifa_flags) & filter.flagmask)
 				continue;
 			if (filter.pfx.family || filter.label) {
-				struct rtattr *tb[IFA_MAX+1];
-				parse_rtattr(tb, IFA_MAX, IFA_RTA(ifa), IFA_PAYLOAD(n));
 				if (!tb[IFA_LOCAL])
 					tb[IFA_LOCAL] = tb[IFA_ADDRESS];
 
@@ -1252,6 +1264,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 	__u32 preferred_lft = INFINITY_LIFE_TIME;
 	__u32 valid_lft = INFINITY_LIFE_TIME;
 	struct ifa_cacheinfo cinfo;
+	unsigned int ifa_flags = 0;
 
 	memset(&req, 0, sizeof(req));
 
@@ -1329,9 +1342,9 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 			if (set_lifetime(&preferred_lft, *argv))
 				invarg("preferred_lft value", *argv);
 		} else if (strcmp(*argv, "home") == 0) {
-			req.ifa.ifa_flags |= IFA_F_HOMEADDRESS;
+			ifa_flags |= IFA_F_HOMEADDRESS;
 		} else if (strcmp(*argv, "nodad") == 0) {
-			req.ifa.ifa_flags |= IFA_F_NODAD;
+			ifa_flags |= IFA_F_NODAD;
 		} else {
 			if (strcmp(*argv, "local") == 0) {
 				NEXT_ARG();
@@ -1349,6 +1362,9 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 		}
 		argc--; argv++;
 	}
+	req.ifa.ifa_flags = ifa_flags;
+	addattr32(&req.n, sizeof(req), IFA_FLAGS, ifa_flags);
+
 	if (d == NULL) {
 		fprintf(stderr, "Not enough information: \"dev\" argument is required.\n");
 		return -1;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [patch iproute2 v2 2/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-02 15:34 [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
  2014-01-02 15:34 ` [patch iproute2 v2 1/2] add support for extended ifa_flags Jiri Pirko
@ 2014-01-02 15:34 ` Jiri Pirko
  2014-01-02 15:50   ` [PATCH 1/1] fixup! " Thomas Haller
  2014-01-02 17:29 ` [patch iproute2 v2 0/2] " Hannes Frederic Sowa
  2 siblings, 1 reply; 30+ messages in thread
From: Jiri Pirko @ 2014-01-02 15:34 UTC (permalink / raw)
  To: netdev; +Cc: stephen, thaller

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/linux/if_addr.h | 1 +
 ip/ipaddress.c          | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/if_addr.h b/include/linux/if_addr.h
index cced59f..e1e95ce 100644
--- a/include/linux/if_addr.h
+++ b/include/linux/if_addr.h
@@ -45,6 +45,7 @@ enum {
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40
 #define IFA_F_PERMANENT		0x80
+#define IFA_F_MANAGETEMPADDR	0x0100
 
 struct ifa_cacheinfo {
 	__u32	ifa_prefered;
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 1e3f22c..5dbb017 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -703,6 +703,10 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		ifa_flags &= ~IFA_F_NODAD;
 		fprintf(fp, "nodad ");
 	}
+	if (ifa_flags & IFA_F_MANAGETEMPADDR) {
+		ifa_flags &= ~IFA_F_MANAGETEMPADDR;
+		fprintf(fp, "mngtmpaddr ");
+	}
 	if (!(ifa_flags & IFA_F_PERMANENT)) {
 		fprintf(fp, "dynamic ");
 	} else
@@ -1345,6 +1349,8 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 			ifa_flags |= IFA_F_HOMEADDRESS;
 		} else if (strcmp(*argv, "nodad") == 0) {
 			ifa_flags |= IFA_F_NODAD;
+		} else if (strcmp(*argv, "mngtmpaddr") == 0) {
+			ifa_flags |= IFA_F_MANAGETEMPADDR;
 		} else {
 			if (strcmp(*argv, "local") == 0) {
 				NEXT_ARG();
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 1/1] fixup! add support for IFA_F_MANAGETEMPADDR
  2014-01-02 15:34 ` [patch iproute2 v2 2/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
@ 2014-01-02 15:50   ` Thomas Haller
  2014-01-04 10:44     ` Jiri Pirko
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-02 15:50 UTC (permalink / raw)
  To: netdev; +Cc: stephen, Jiri Pirko, Thomas Haller

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 I think, also the following changes are needed(??)

 ip/ipaddress.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5dbb017..b0d54fe 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -82,7 +82,7 @@ static void usage(void)
 	fprintf(stderr, "           tentative | deprecated | dadfailed | temporary |\n");
 	fprintf(stderr, "           CONFFLAG-LIST ]\n");
 	fprintf(stderr, "CONFFLAG-LIST := [ CONFFLAG-LIST ] CONFFLAG\n");
-	fprintf(stderr, "CONFFLAG  := [ home | nodad ]\n");
+	fprintf(stderr, "CONFFLAG  := [ home | nodad | mngtmpaddr ]\n");
 	fprintf(stderr, "LIFETIME := [ valid_lft LFT ] [ preferred_lft LFT ]\n");
 	fprintf(stderr, "LFT := forever | SECONDS\n");
 
@@ -1130,6 +1130,9 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 		} else if (strcmp(*argv, "nodad") == 0) {
 			filter.flags |= IFA_F_NODAD;
 			filter.flagmask |= IFA_F_NODAD;
+		} else if (strcmp(*argv, "mngtmpaddr") == 0) {
+			filter.flags |= IFA_F_MANAGETEMPADDR;
+			filter.flagmask |= IFA_F_MANAGETEMPADDR;
 		} else if (strcmp(*argv, "dadfailed") == 0) {
 			filter.flags |= IFA_F_DADFAILED;
 			filter.flagmask |= IFA_F_DADFAILED;
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-02 15:34 [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
  2014-01-02 15:34 ` [patch iproute2 v2 1/2] add support for extended ifa_flags Jiri Pirko
  2014-01-02 15:34 ` [patch iproute2 v2 2/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
@ 2014-01-02 17:29 ` Hannes Frederic Sowa
  2014-01-04 10:43   ` Jiri Pirko
  2 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-02 17:29 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, stephen, thaller

On Thu, Jan 02, 2014 at 04:34:37PM +0100, Jiri Pirko wrote:
> v1->v2: Removed 0xff masking of ifa_flags
> 
> Jiri Pirko (2):
>   add support for extended ifa_flags
>   add support for IFA_F_MANAGETEMPADDR
> 
>  include/linux/if_addr.h |  2 ++
>  ip/ipaddress.c          | 50 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 38 insertions(+), 14 deletions(-)

I still wonder how source address selection should work for
IFA_F_MANAGETEMPADDR if use_tempaddr != 2 mode is not available for
those addresses.

Up until now applications can bind to those addresses and traffic can be
received for them, but there is now way how a user can specify to favor them
in case of use_tempaddr == 0.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-02 17:29 ` [patch iproute2 v2 0/2] " Hannes Frederic Sowa
@ 2014-01-04 10:43   ` Jiri Pirko
  2014-01-04 10:55     ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Jiri Pirko @ 2014-01-04 10:43 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: netdev, stephen, thaller

Thu, Jan 02, 2014 at 06:29:49PM CET, hannes@stressinduktion.org wrote:
>On Thu, Jan 02, 2014 at 04:34:37PM +0100, Jiri Pirko wrote:
>> v1->v2: Removed 0xff masking of ifa_flags
>> 
>> Jiri Pirko (2):
>>   add support for extended ifa_flags
>>   add support for IFA_F_MANAGETEMPADDR
>> 
>>  include/linux/if_addr.h |  2 ++
>>  ip/ipaddress.c          | 50 +++++++++++++++++++++++++++++++++++--------------
>>  2 files changed, 38 insertions(+), 14 deletions(-)
>
>I still wonder how source address selection should work for
>IFA_F_MANAGETEMPADDR if use_tempaddr != 2 mode is not available for
>those addresses.
>
>Up until now applications can bind to those addresses and traffic can be
>received for them, but there is now way how a user can specify to favor them
>in case of use_tempaddr == 0.

I'm not sure I understand you. Can you please elaborate more? Not sure
how this is related to iproute2.

Anyway, the kernel behaviour wrt use_tempaddr settings remains unchanged
with the addition of IFA_F_MANAGETEMPADDR. It only allows to create temp
addresses for other addresses than the ones created by kernel (by RA).

Jiri
	
>
>Greetings,
>
>  Hannes
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] fixup! add support for IFA_F_MANAGETEMPADDR
  2014-01-02 15:50   ` [PATCH 1/1] fixup! " Thomas Haller
@ 2014-01-04 10:44     ` Jiri Pirko
  0 siblings, 0 replies; 30+ messages in thread
From: Jiri Pirko @ 2014-01-04 10:44 UTC (permalink / raw)
  To: Thomas Haller; +Cc: netdev, stephen

Thu, Jan 02, 2014 at 04:50:15PM CET, thaller@redhat.com wrote:
>Signed-off-by: Thomas Haller <thaller@redhat.com>
>---
> I think, also the following changes are needed(??)

Right. I will incorporate this into the patch and send v3. Thanks Thomas	

>
> ip/ipaddress.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>index 5dbb017..b0d54fe 100644
>--- a/ip/ipaddress.c
>+++ b/ip/ipaddress.c
>@@ -82,7 +82,7 @@ static void usage(void)
> 	fprintf(stderr, "           tentative | deprecated | dadfailed | temporary |\n");
> 	fprintf(stderr, "           CONFFLAG-LIST ]\n");
> 	fprintf(stderr, "CONFFLAG-LIST := [ CONFFLAG-LIST ] CONFFLAG\n");
>-	fprintf(stderr, "CONFFLAG  := [ home | nodad ]\n");
>+	fprintf(stderr, "CONFFLAG  := [ home | nodad | mngtmpaddr ]\n");
> 	fprintf(stderr, "LIFETIME := [ valid_lft LFT ] [ preferred_lft LFT ]\n");
> 	fprintf(stderr, "LFT := forever | SECONDS\n");
> 
>@@ -1130,6 +1130,9 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
> 		} else if (strcmp(*argv, "nodad") == 0) {
> 			filter.flags |= IFA_F_NODAD;
> 			filter.flagmask |= IFA_F_NODAD;
>+		} else if (strcmp(*argv, "mngtmpaddr") == 0) {
>+			filter.flags |= IFA_F_MANAGETEMPADDR;
>+			filter.flagmask |= IFA_F_MANAGETEMPADDR;
> 		} else if (strcmp(*argv, "dadfailed") == 0) {
> 			filter.flags |= IFA_F_DADFAILED;
> 			filter.flagmask |= IFA_F_DADFAILED;
>-- 
>1.8.4.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 10:43   ` Jiri Pirko
@ 2014-01-04 10:55     ` Hannes Frederic Sowa
  2014-01-04 11:05       ` Jiri Pirko
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-04 10:55 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, stephen, thaller

On Sat, Jan 04, 2014 at 11:43:31AM +0100, Jiri Pirko wrote:
> Thu, Jan 02, 2014 at 06:29:49PM CET, hannes@stressinduktion.org wrote:
> >On Thu, Jan 02, 2014 at 04:34:37PM +0100, Jiri Pirko wrote:
> >> v1->v2: Removed 0xff masking of ifa_flags
> >> 
> >> Jiri Pirko (2):
> >>   add support for extended ifa_flags
> >>   add support for IFA_F_MANAGETEMPADDR
> >> 
> >>  include/linux/if_addr.h |  2 ++
> >>  ip/ipaddress.c          | 50 +++++++++++++++++++++++++++++++++++--------------
> >>  2 files changed, 38 insertions(+), 14 deletions(-)
> >
> >I still wonder how source address selection should work for
> >IFA_F_MANAGETEMPADDR if use_tempaddr != 2 mode is not available for
> >those addresses.
> >
> >Up until now applications can bind to those addresses and traffic can be
> >received for them, but there is now way how a user can specify to favor them
> >in case of use_tempaddr == 0.
> 
> I'm not sure I understand you. Can you please elaborate more? Not sure
> how this is related to iproute2.

Sorry, it is not related to this patch set at all but more to
IFA_F_MANAGETEMPADDR as a whole (maybe it could be a follow-up feature).

> Anyway, the kernel behaviour wrt use_tempaddr settings remains unchanged
> with the addition of IFA_F_MANAGETEMPADDR. It only allows to create temp
> addresses for other addresses than the ones created by kernel (by RA).

I assume that systems with NetworkManager won't activate use_tempaddr. If
you look at ipv6_get_saddr_eval we only prefer privacy addresses to
normal ones, if use_tempaddr == 2, which also implies that kernel does
generate privacy addresses.

So currently privacy addresses are correctly installed, but we cannot control
if we want prefer them to global addresses for outgoing connections where the
socket is not bound to a specific address.

Also, I saw that NetworkManager switched to install autoconf addresses
as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
prefixlen?

I guess NetworkManager wants a way to add /64 addresses without installing the
on-link prefix route?

Hope that makes sense?

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 10:55     ` Hannes Frederic Sowa
@ 2014-01-04 11:05       ` Jiri Pirko
  2014-01-04 11:15         ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Jiri Pirko @ 2014-01-04 11:05 UTC (permalink / raw)
  To: netdev, stephen, thaller

Sat, Jan 04, 2014 at 11:55:15AM CET, hannes@stressinduktion.org wrote:
>On Sat, Jan 04, 2014 at 11:43:31AM +0100, Jiri Pirko wrote:
>> Thu, Jan 02, 2014 at 06:29:49PM CET, hannes@stressinduktion.org wrote:
>> >On Thu, Jan 02, 2014 at 04:34:37PM +0100, Jiri Pirko wrote:
>> >> v1->v2: Removed 0xff masking of ifa_flags
>> >> 
>> >> Jiri Pirko (2):
>> >>   add support for extended ifa_flags
>> >>   add support for IFA_F_MANAGETEMPADDR
>> >> 
>> >>  include/linux/if_addr.h |  2 ++
>> >>  ip/ipaddress.c          | 50 +++++++++++++++++++++++++++++++++++--------------
>> >>  2 files changed, 38 insertions(+), 14 deletions(-)
>> >
>> >I still wonder how source address selection should work for
>> >IFA_F_MANAGETEMPADDR if use_tempaddr != 2 mode is not available for
>> >those addresses.
>> >
>> >Up until now applications can bind to those addresses and traffic can be
>> >received for them, but there is now way how a user can specify to favor them
>> >in case of use_tempaddr == 0.
>> 
>> I'm not sure I understand you. Can you please elaborate more? Not sure
>> how this is related to iproute2.
>
>Sorry, it is not related to this patch set at all but more to
>IFA_F_MANAGETEMPADDR as a whole (maybe it could be a follow-up feature).
>
>> Anyway, the kernel behaviour wrt use_tempaddr settings remains unchanged
>> with the addition of IFA_F_MANAGETEMPADDR. It only allows to create temp
>> addresses for other addresses than the ones created by kernel (by RA).
>
>I assume that systems with NetworkManager won't activate use_tempaddr. If
>you look at ipv6_get_saddr_eval we only prefer privacy addresses to
>normal ones, if use_tempaddr == 2, which also implies that kernel does
>generate privacy addresses.

Sure. NM should set use_tempaddr accordingly. You are right that kernel
generate temporary adresses, but only for the prefixes received via
neighbor discovery (see addrconf_prefix_rcv). The ones that are set by
hand are not handled. That is the reason we introduced IFA_F_MANAGETEMPADDR.

>
>So currently privacy addresses are correctly installed, but we cannot control
>if we want prefer them to global addresses for outgoing connections where the
>socket is not bound to a specific address.
>
>Also, I saw that NetworkManager switched to install autoconf addresses
>as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
>prefixlen?

/64 is required

>
>I guess NetworkManager wants a way to add /64 addresses without installing the
>on-link prefix route?
>
>Hope that makes sense?
>
>Greetings,
>
>  Hannes
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 11:05       ` Jiri Pirko
@ 2014-01-04 11:15         ` Hannes Frederic Sowa
  2014-01-04 11:21           ` Thomas Haller
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-04 11:15 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, stephen, thaller

On Sat, Jan 04, 2014 at 12:05:57PM +0100, Jiri Pirko wrote:
> Sure. NM should set use_tempaddr accordingly. You are right that kernel
> generate temporary adresses, but only for the prefixes received via
> neighbor discovery (see addrconf_prefix_rcv). The ones that are set by
> hand are not handled. That is the reason we introduced IFA_F_MANAGETEMPADDR.

Ah, sorry. So NM sets use_tempaddr == 2 but disables accept_ra? That's fine,
sorry to bother!

> >So currently privacy addresses are correctly installed, but we cannot control
> >if we want prefer them to global addresses for outgoing connections where the
> >socket is not bound to a specific address.
> >
> >Also, I saw that NetworkManager switched to install autoconf addresses
> >as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
> >prefixlen?
> 
> /64 is required

Ok, currently NM seems to "violate" that as it installs autoconf addresses
with 128 prefixlen, so IFA_F_MANAGETEMPADDR should not work on them.
(currently observed on Fedora 20).

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 11:15         ` Hannes Frederic Sowa
@ 2014-01-04 11:21           ` Thomas Haller
  2014-01-04 11:35             ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-04 11:21 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

Hi,

On Sat, 2014-01-04 at 12:15 +0100, Hannes Frederic Sowa wrote:
> On Sat, Jan 04, 2014 at 12:05:57PM +0100, Jiri Pirko wrote:
> > Sure. NM should set use_tempaddr accordingly. You are right that kernel
> > generate temporary adresses, but only for the prefixes received via
> > neighbor discovery (see addrconf_prefix_rcv). The ones that are set by
> > hand are not handled. That is the reason we introduced IFA_F_MANAGETEMPADDR.
> 
> Ah, sorry. So NM sets use_tempaddr == 2 but disables accept_ra? That's fine,
> sorry to bother!

yes, that is the plan. use_tempaddr is to configure the preference for
address selection, and without accept_ra, the kernel will not add
autoconf addresses himself -- only NM adds them with
IFA_F_MANAGETEMPADDR. I think this will work out fine.

> 
> > >So currently privacy addresses are correctly installed, but we cannot control
> > >if we want prefer them to global addresses for outgoing connections where the
> > >socket is not bound to a specific address.
> > >
> > >Also, I saw that NetworkManager switched to install autoconf addresses
> > >as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
> > >prefixlen?
> > 
> > /64 is required
> 
> Ok, currently NM seems to "violate" that as it installs autoconf addresses
> with 128 prefixlen, so IFA_F_MANAGETEMPADDR should not work on them.
> (currently observed on Fedora 20).

True, I noticed that too. I think that is a bug in NM to add the
addresses as /128. Probably, we will fix that soon.


Thomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 11:21           ` Thomas Haller
@ 2014-01-04 11:35             ` Hannes Frederic Sowa
  2014-01-06 15:41               ` Thomas Haller
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-04 11:35 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen

On Sat, Jan 04, 2014 at 12:21:51PM +0100, Thomas Haller wrote:
> > > >Also, I saw that NetworkManager switched to install autoconf addresses
> > > >as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
> > > >prefixlen?
> > > 
> > > /64 is required
> > 
> > Ok, currently NM seems to "violate" that as it installs autoconf addresses
> > with 128 prefixlen, so IFA_F_MANAGETEMPADDR should not work on them.
> > (currently observed on Fedora 20).
> 
> True, I noticed that too. I think that is a bug in NM to add the
> addresses as /128. Probably, we will fix that soon.

The change could be valid. Otherwise currently NM could not correctly handle
prefix information in RAs in some cases:

It is possible to let the client generate an autonomously address in a
prefix which is actually not on-link (L=0). Kernel would automatically
create prefix route by mistake, if NM tries to install such an address
with /64 prefix. This does not happen if the prefix address uses 128
prefixlen.

Would be great to have feedback on this, as this could be easily solved by an
additional ifa_flag.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-04 11:35             ` Hannes Frederic Sowa
@ 2014-01-06 15:41               ` Thomas Haller
  2014-01-06 16:01                 ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-06 15:41 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On Sat, 2014-01-04 at 12:35 +0100, Hannes Frederic Sowa wrote:
> On Sat, Jan 04, 2014 at 12:21:51PM +0100, Thomas Haller wrote:
> > > > >Also, I saw that NetworkManager switched to install autoconf addresses
> > > > >as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
> > > > >prefixlen?
> > > > 
> > > > /64 is required
> > > 
> > > Ok, currently NM seems to "violate" that as it installs autoconf addresses
> > > with 128 prefixlen, so IFA_F_MANAGETEMPADDR should not work on them.
> > > (currently observed on Fedora 20).
> > 
> > True, I noticed that too. I think that is a bug in NM to add the
> > addresses as /128. Probably, we will fix that soon.
> 
> The change could be valid. Otherwise currently NM could not correctly handle
> prefix information in RAs in some cases:
> 
> It is possible to let the client generate an autonomously address in a
> prefix which is actually not on-link (L=0). Kernel would automatically
> create prefix route by mistake, if NM tries to install such an address
> with /64 prefix. This does not happen if the prefix address uses 128
> prefixlen.
> 
> Would be great to have feedback on this, as this could be easily solved by an
> additional ifa_flag.
> 
> Greetings,
> 
>   Hannes
> 


Hi Hannes,


good point. I think, the user-space application (NetworkManager) should
add the autoconf addresses as /64. But the kernel should not generate
any routes in that case. It's up to the application to add them
(depending on the on-link flag).


What do you think about something like:



diff --git i/net/ipv6/addrconf.c w/net/ipv6/addrconf.c
index 6c16345..5a4c382 100644
--- i/net/ipv6/addrconf.c
+++ w/net/ipv6/addrconf.c
@@ -2433,8 +2433,11 @@ static int inet6_addr_add(struct net *net, int ifindex,
 			    valid_lft, prefered_lft);
 
 	if (!IS_ERR(ifp)) {
-		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
-				      expires, flags);
+		if (ifa_flags & IFA_F_MANAGETEMPADDR == 0) {
+			addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
+					      expires, flags);
+		}
+
 		/*
 		 * Note that section 3.1 of RFC 4429 indicates
 		 * that the Optimistic flag should not be set for


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR
  2014-01-06 15:41               ` Thomas Haller
@ 2014-01-06 16:01                 ` Hannes Frederic Sowa
  2014-01-06 17:29                   ` [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Thomas Haller
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-06 16:01 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

On Mon, Jan 06, 2014 at 04:41:49PM +0100, Thomas Haller wrote:
> On Sat, 2014-01-04 at 12:35 +0100, Hannes Frederic Sowa wrote:
> > On Sat, Jan 04, 2014 at 12:21:51PM +0100, Thomas Haller wrote:
> > > > > >Also, I saw that NetworkManager switched to install autoconf addresses
> > > > > >as /128, doesn't this break with IFA_F_MANAGETEMPADDR, as you expect a /64
> > > > > >prefixlen?
> > > > > 
> > > > > /64 is required
> > > > 
> > > > Ok, currently NM seems to "violate" that as it installs autoconf addresses
> > > > with 128 prefixlen, so IFA_F_MANAGETEMPADDR should not work on them.
> > > > (currently observed on Fedora 20).
> > > 
> > > True, I noticed that too. I think that is a bug in NM to add the
> > > addresses as /128. Probably, we will fix that soon.
> > 
> > The change could be valid. Otherwise currently NM could not correctly handle
> > prefix information in RAs in some cases:
> > 
> > It is possible to let the client generate an autonomously address in a
> > prefix which is actually not on-link (L=0). Kernel would automatically
> > create prefix route by mistake, if NM tries to install such an address
> > with /64 prefix. This does not happen if the prefix address uses 128
> > prefixlen.
> > 
> > Would be great to have feedback on this, as this could be easily solved by an
> > additional ifa_flag.
> > 
> > Greetings,
> > 
> >   Hannes
> > 
> 
> 
> Hi Hannes,
> 
> 
> good point. I think, the user-space application (NetworkManager) should
> add the autoconf addresses as /64. But the kernel should not generate
> any routes in that case. It's up to the application to add them
> (depending on the on-link flag).
> 
> 
> What do you think about something like:

I would introduce a new flag for that and also make it accessible via
iproute, maybe later. Otherwise IFA_F_MANAGETEMPADDR is overloaded and
doesn't do justice to its name. And since Jiri made new room in ifa_flags,
it shoud be no problem. ;)

Btw. while just reviewing anycast changes, I noticed a funny thing:
Kernel does allocate an anycast address which is the same as the autoconf
address, if NM installs /128 address and you have ipv6 forwarding enabled
(fedora 20 install + ipv6 forwarding because of libvirt). You can see the
addresses pop up in /proc/net/anycast6. I currenlty don't know if that
is problematic.  Everything seems to work here for me. ;)

In the end, I don't think we should install anycast for /128, but I am not
sure, yet.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-06 16:01                 ` Hannes Frederic Sowa
@ 2014-01-06 17:29                   ` Thomas Haller
  2014-01-06 17:38                     ` Jiri Pirko
                                       ` (2 more replies)
  0 siblings, 3 replies; 30+ messages in thread
From: Thomas Haller @ 2014-01-06 17:29 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw, Thomas Haller

When adding/modifying an IPv6 address, the userspace application needs
a way to suppress adding a prefix route. This is for example relevant
together with IFA_F_MANAGERTEMPADDR, where userspace creates autoconf
generated addresses, but depending on on-link, no route should for the
prefix should be added.

This flag will not be set as ifa_flags of the address, it is only
considered as parameter while adding/modifying an address.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Hi, how about this?

 The flag is only a parameter for the netlink command.
 This might be unexpected, because when adding an address,
 you won't see the flag in `ip -6 addr`.
 Still, I think, it is better to do it this way, because having
 an address with the NOPREFIXROUTE flag, does not mean, that
 there is no route for this prefix. It only means, that at the
 moment of setting the address, no route was added.

 The alternative would be, not to add a prefix route, when
 IFA_F_MANAGERTEMPADDR is set.

 Thomas

 include/uapi/linux/if_addr.h |  1 +
 net/ipv6/addrconf.c          | 19 +++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
index cfed10b..dea10a8 100644
--- a/include/uapi/linux/if_addr.h
+++ b/include/uapi/linux/if_addr.h
@@ -49,6 +49,7 @@ enum {
 #define IFA_F_TENTATIVE		0x40
 #define IFA_F_PERMANENT		0x80
 #define IFA_F_MANAGETEMPADDR	0x100
+#define IFA_F_NOPREFIXROUTE	0x200
 
 struct ifa_cacheinfo {
 	__u32	ifa_prefered;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6c16345..51bd757 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2429,12 +2429,16 @@ static int inet6_addr_add(struct net *net, int ifindex,
 		prefered_lft = timeout;
 	}
 
-	ifp = ipv6_add_addr(idev, pfx, peer_pfx, plen, scope, ifa_flags,
+	ifp = ipv6_add_addr(idev, pfx, peer_pfx, plen, scope,
+			    ifa_flags & ~IFA_F_NOPREFIXROUTE,
 			    valid_lft, prefered_lft);
 
 	if (!IS_ERR(ifp)) {
-		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
-				      expires, flags);
+		if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
+			addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
+					      expires, flags);
+		}
+
 		/*
 		 * Note that section 3.1 of RFC 4429 indicates
 		 * that the Optimistic flag should not be set for
@@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 	if (!(ifp->flags&IFA_F_TENTATIVE))
 		ipv6_ifa_notify(0, ifp);
 
-	addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
-			      expires, flags);
+	if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
+		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
+				      expires, flags);
+	}
 
 	if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
 		if (was_managetempaddr && !(ifp->flags & IFA_F_MANAGETEMPADDR))
@@ -3717,7 +3723,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
 	ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) : ifm->ifa_flags;
 
 	/* We ignore other flags so far. */
-	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR;
+	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
+		     IFA_F_NOPREFIXROUTE;
 
 	ifa = ipv6_get_ifaddr(net, pfx, dev, 1);
 	if (ifa == NULL) {
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-06 17:29                   ` [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Thomas Haller
@ 2014-01-06 17:38                     ` Jiri Pirko
  2014-01-07  9:39                       ` Hannes Frederic Sowa
  2014-01-07 12:01                     ` Hannes Frederic Sowa
  2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
  2 siblings, 1 reply; 30+ messages in thread
From: Jiri Pirko @ 2014-01-06 17:38 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Hannes Frederic Sowa, netdev, stephen, dcbw

Mon, Jan 06, 2014 at 06:29:35PM CET, thaller@redhat.com wrote:
>When adding/modifying an IPv6 address, the userspace application needs
>a way to suppress adding a prefix route. This is for example relevant
>together with IFA_F_MANAGERTEMPADDR, where userspace creates autoconf
>generated addresses, but depending on on-link, no route should for the
>prefix should be added.
>
>This flag will not be set as ifa_flags of the address, it is only
>considered as parameter while adding/modifying an address.
>
>Signed-off-by: Thomas Haller <thaller@redhat.com>
>---
> Hi, how about this?
>
> The flag is only a parameter for the netlink command.
> This might be unexpected, because when adding an address,
> you won't see the flag in `ip -6 addr`.
> Still, I think, it is better to do it this way, because having
> an address with the NOPREFIXROUTE flag, does not mean, that
> there is no route for this prefix. It only means, that at the
> moment of setting the address, no route was added.
>
> The alternative would be, not to add a prefix route, when
> IFA_F_MANAGERTEMPADDR is set.
>
> Thomas
>
> include/uapi/linux/if_addr.h |  1 +
> net/ipv6/addrconf.c          | 19 +++++++++++++------
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
>diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
>index cfed10b..dea10a8 100644
>--- a/include/uapi/linux/if_addr.h
>+++ b/include/uapi/linux/if_addr.h
>@@ -49,6 +49,7 @@ enum {
> #define IFA_F_TENTATIVE		0x40
> #define IFA_F_PERMANENT		0x80
> #define IFA_F_MANAGETEMPADDR	0x100
>+#define IFA_F_NOPREFIXROUTE	0x200
> 
> struct ifa_cacheinfo {
> 	__u32	ifa_prefered;
>diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>index 6c16345..51bd757 100644
>--- a/net/ipv6/addrconf.c
>+++ b/net/ipv6/addrconf.c
>@@ -2429,12 +2429,16 @@ static int inet6_addr_add(struct net *net, int ifindex,
> 		prefered_lft = timeout;
> 	}
> 
>-	ifp = ipv6_add_addr(idev, pfx, peer_pfx, plen, scope, ifa_flags,
>+	ifp = ipv6_add_addr(idev, pfx, peer_pfx, plen, scope,
>+			    ifa_flags & ~IFA_F_NOPREFIXROUTE,
> 			    valid_lft, prefered_lft);
> 
> 	if (!IS_ERR(ifp)) {
>-		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
>-				      expires, flags);
>+		if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {

if (!(ifa_flags & IFA_F_NOPREFIXROUTE)) {

>+			addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
>+					      expires, flags);
>+		}
>+
> 		/*
> 		 * Note that section 3.1 of RFC 4429 indicates
> 		 * that the Optimistic flag should not be set for
>@@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
> 	if (!(ifp->flags&IFA_F_TENTATIVE))
> 		ipv6_ifa_notify(0, ifp);
> 
>-	addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
>-			      expires, flags);
>+	if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
>+		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
>+				      expires, flags);
>+	}
> 
> 	if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
> 		if (was_managetempaddr && !(ifp->flags & IFA_F_MANAGETEMPADDR))
>@@ -3717,7 +3723,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
> 	ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) : ifm->ifa_flags;
> 
> 	/* We ignore other flags so far. */
>-	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR;
>+	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
>+		     IFA_F_NOPREFIXROUTE;
> 
> 	ifa = ipv6_get_ifaddr(net, pfx, dev, 1);
> 	if (ifa == NULL) {
>-- 
>1.8.4.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-06 17:38                     ` Jiri Pirko
@ 2014-01-07  9:39                       ` Hannes Frederic Sowa
  0 siblings, 0 replies; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07  9:39 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Thomas Haller, netdev, stephen, dcbw

On Mon, Jan 06, 2014 at 06:38:37PM +0100, Jiri Pirko wrote:
> Mon, Jan 06, 2014 at 06:29:35PM CET, thaller@redhat.com wrote:
> >When adding/modifying an IPv6 address, the userspace application needs
> >a way to suppress adding a prefix route. This is for example relevant
> >together with IFA_F_MANAGERTEMPADDR, where userspace creates autoconf
> >generated addresses, but depending on on-link, no route should for the
> >prefix should be added.
> >
> >This flag will not be set as ifa_flags of the address, it is only
> >considered as parameter while adding/modifying an address.
> >
> >Signed-off-by: Thomas Haller <thaller@redhat.com>
> >---
> > Hi, how about this?
> >
> > The flag is only a parameter for the netlink command.
> > This might be unexpected, because when adding an address,
> > you won't see the flag in `ip -6 addr`.
> > Still, I think, it is better to do it this way, because having
> > an address with the NOPREFIXROUTE flag, does not mean, that
> > there is no route for this prefix. It only means, that at the
> > moment of setting the address, no route was added.
> >
> > The alternative would be, not to add a prefix route, when
> > IFA_F_MANAGERTEMPADDR is set.

Looks good, maybe you could apply Jiri's feedback.

Otherwise we could skip trying to find a corresponding prefix route in
ipv6_del_addr. You would need to add the IFA_F_NOPREFIXROUTE test just before
the addrconf_get_prefix_route and ip6_del_rt.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-06 17:29                   ` [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Thomas Haller
  2014-01-06 17:38                     ` Jiri Pirko
@ 2014-01-07 12:01                     ` Hannes Frederic Sowa
  2014-01-07 12:14                       ` Thomas Haller
  2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
  2 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 12:01 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

On Mon, Jan 06, 2014 at 06:29:35PM +0100, Thomas Haller wrote:
> @@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
>  	if (!(ifp->flags&IFA_F_TENTATIVE))
>  		ipv6_ifa_notify(0, ifp);
>  
> -	addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> -			      expires, flags);
> +	if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
> +		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> +				      expires, flags);
> +	}

Actually, if we switch from IFA_F_NOPREFIXROUTE to !IFA_F_NOPREFIXROUTE we
have to remove the prefix route, no?

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-07 12:01                     ` Hannes Frederic Sowa
@ 2014-01-07 12:14                       ` Thomas Haller
  2014-01-07 12:22                         ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 12:14 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Tue, 2014-01-07 at 13:01 +0100, Hannes Frederic Sowa wrote:
> On Mon, Jan 06, 2014 at 06:29:35PM +0100, Thomas Haller wrote:
> > @@ -3662,8 +3666,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
> >  	if (!(ifp->flags&IFA_F_TENTATIVE))
> >  		ipv6_ifa_notify(0, ifp);
> >  
> > -	addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> > -			      expires, flags);
> > +	if (ifa_flags & IFA_F_NOPREFIXROUTE == 0) {
> > +		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
> > +				      expires, flags);
> > +	}
> 
> Actually, if we switch from IFA_F_NOPREFIXROUTE to !IFA_F_NOPREFIXROUTE we
> have to remove the prefix route, no?
> 
> Greetings,
> 
>   Hannes
> 

hi Hannes,


I am about to resent the patch, so that IFA_F_NOPREFIXROUTE is saved in
the flags of the address. Later, when deleting such address, this is
used to indicate ~not~ to delete any prefix route... just as you suggest
in your earlier email (if I understood you right).

About this suggestion now, I tend to "no". Yes, it could be sensible, on
the other hand, if user space already controls the routes (as indicated
by it's use of IFA_F_NOPREFIXROUTES), I would just leave it to the user
to clean up the wrong prefix route.

What do you think?


Thomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-07 12:14                       ` Thomas Haller
@ 2014-01-07 12:22                         ` Hannes Frederic Sowa
  0 siblings, 0 replies; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 12:22 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

Hi Thomas!

On Tue, Jan 07, 2014 at 01:14:34PM +0100, Thomas Haller wrote:
> I am about to resent the patch, so that IFA_F_NOPREFIXROUTE is saved in
> the flags of the address. Later, when deleting such address, this is
> used to indicate ~not~ to delete any prefix route... just as you suggest
> in your earlier email (if I understood you right).

Ok, good.

> About this suggestion now, I tend to "no". Yes, it could be sensible, on
> the other hand, if user space already controls the routes (as indicated
> by it's use of IFA_F_NOPREFIXROUTES), I would just leave it to the user
> to clean up the wrong prefix route.
>
> What do you think?

It is a matter of usability. As prefix routes are added to main routing
table and are visible via simple ip -6 r l (and not ip -6 r l table
something), I am fine with that, too.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-06 17:29                   ` [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Thomas Haller
  2014-01-06 17:38                     ` Jiri Pirko
  2014-01-07 12:01                     ` Hannes Frederic Sowa
@ 2014-01-07 14:39                     ` Thomas Haller
  2014-01-07 14:39                       ` [PATCH v2 1/2] " Thomas Haller
                                         ` (2 more replies)
  2 siblings, 3 replies; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 14:39 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw, Thomas Haller

Now, the IFA_F_NOPREFIXROUTE flag is saved in ifp->flags.
The second patch reworks the deletion of addresses/cleanup of prefix
routes and considers IFA_F_NOPREFIXROUTE.

Thomas Haller (2):
  ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of
    IP6 routes
  ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE

 include/uapi/linux/if_addr.h |   1 +
 net/ipv6/addrconf.c          | 206 ++++++++++++++++++++++++++-----------------
 2 files changed, 125 insertions(+), 82 deletions(-)

-- 
1.8.4.2

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
@ 2014-01-07 14:39                       ` Thomas Haller
  2014-01-07 14:39                       ` [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE Thomas Haller
  2014-01-07 16:03                       ` [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Hannes Frederic Sowa
  2 siblings, 0 replies; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 14:39 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw, Thomas Haller

When adding/modifying an IPv6 address, the userspace application needs
a way to suppress adding a prefix route. This is for example relevant
together with IFA_F_MANAGERTEMPADDR, where userspace creates autoconf
generated addresses, but depending on on-link, no route for the
prefix should be added.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/uapi/linux/if_addr.h |  1 +
 net/ipv6/addrconf.c          | 18 ++++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
index cfed10b..dea10a8 100644
--- a/include/uapi/linux/if_addr.h
+++ b/include/uapi/linux/if_addr.h
@@ -49,6 +49,7 @@ enum {
 #define IFA_F_TENTATIVE		0x40
 #define IFA_F_PERMANENT		0x80
 #define IFA_F_MANAGETEMPADDR	0x100
+#define IFA_F_NOPREFIXROUTE	0x200
 
 struct ifa_cacheinfo {
 	__u32	ifa_prefered;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 31f75ea..1bc575f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2433,8 +2433,11 @@ static int inet6_addr_add(struct net *net, int ifindex,
 			    valid_lft, prefered_lft);
 
 	if (!IS_ERR(ifp)) {
-		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
-				      expires, flags);
+		if (!(ifa_flags & IFA_F_NOPREFIXROUTE)) {
+			addrconf_prefix_route(&ifp->addr, ifp->prefix_len, dev,
+					      expires, flags);
+		}
+
 		/*
 		 * Note that section 3.1 of RFC 4429 indicates
 		 * that the Optimistic flag should not be set for
@@ -3658,7 +3661,7 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 	spin_lock_bh(&ifp->lock);
 	was_managetempaddr = ifp->flags & IFA_F_MANAGETEMPADDR;
 	ifp->flags &= ~(IFA_F_DEPRECATED | IFA_F_PERMANENT | IFA_F_NODAD |
-			IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR);
+			IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR | IFA_F_NOPREFIXROUTE);
 	ifp->flags |= ifa_flags;
 	ifp->tstamp = jiffies;
 	ifp->valid_lft = valid_lft;
@@ -3668,8 +3671,10 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 	if (!(ifp->flags&IFA_F_TENTATIVE))
 		ipv6_ifa_notify(0, ifp);
 
-	addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
-			      expires, flags);
+	if (!(ifa_flags & IFA_F_NOPREFIXROUTE)) {
+		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
+				      expires, flags);
+	}
 
 	if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
 		if (was_managetempaddr && !(ifp->flags & IFA_F_MANAGETEMPADDR))
@@ -3723,7 +3728,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh)
 	ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) : ifm->ifa_flags;
 
 	/* We ignore other flags so far. */
-	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR;
+	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
+		     IFA_F_NOPREFIXROUTE;
 
 	ifa = ipv6_get_ifaddr(net, pfx, dev, 1);
 	if (ifa == NULL) {
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
  2014-01-07 14:39                       ` [PATCH v2 1/2] " Thomas Haller
@ 2014-01-07 14:39                       ` Thomas Haller
  2014-01-07 16:28                         ` Hannes Frederic Sowa
  2014-01-07 16:03                       ` [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Hannes Frederic Sowa
  2 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 14:39 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw, Thomas Haller

Refactor the deletion/update of route prefixes when removing an
address. Now, consider IFA_F_NOPREFIXROUTE and if there is an address
present with this flag, to not cleanup the prefix. Instead, assume
that userspace is taking care of this prefix.

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.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 net/ipv6/addrconf.c | 188 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 76 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 1bc575f..1293a27 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -900,15 +900,106 @@ out:
 	goto out2;
 }
 
+/*
+ * Check, whether the prefix route without ifp is still valid.
+ * The function returns:
+ *   -1 route valid, update lifetimes (returns expires time).
+ *    0 route invalid, delete
+ *    1 route valid, don't update lifetimes.
+ *
+ * 1) we don't purge prefix here if address was not permanent.
+ *    prefix is managed by its own lifetime.
+ * 2) we also don't purge, if the address was IFA_F_NOPREFIXROUTE.
+ * 3) if there're no addresses, delete prefix.
+ * 4) if there're still other permanent address(es),
+ *    corresponding prefix is still permanent.
+ * 5) if there are still other addresses with IFA_F_NOPREFIXROUTE,
+ *    don't purge the prefix, assume user space is
+ *    managing it.
+ * 6) otherwise, update prefix lifetime to the
+ *    longest valid lifetime among the corresponding
+ *    addresses on the device.
+ *    Note: subsequent RA will update lifetime.
+ *
+ * --yoshfuji
+ **/
+static int
+check_cleanup_prefix_routes(struct inet6_ifaddr *ifp, u32 ifa_flags, unsigned long *expires)
+{
+	struct inet6_ifaddr *ifa;
+	struct inet6_dev *idev = ifp->idev;
+	unsigned long lifetime;
+	int onlink = 0;
+
+	*expires = jiffies;
+
+
+	if (!(ifa_flags & IFA_F_PERMANENT))
+		return 1;
+	if (ifa_flags & IFA_F_NOPREFIXROUTE)
+		return 1;
+
+	list_for_each_entry(ifa, &idev->addr_list, if_list) {
+		if (ifa == ifp)
+			continue;
+		if (!ipv6_prefix_equal(&ifa->addr, &ifp->addr,
+				       ifp->prefix_len))
+			continue;
+		if (ifa->flags & IFA_F_PERMANENT)
+			return 1;
+		if (ifa->flags & IFA_F_NOPREFIXROUTE)
+			return 1; /* user space is managing this prefix. */
+
+		onlink = -1;
+
+		spin_lock(&ifa->lock);
+
+		lifetime = addrconf_timeout_fixup(ifa->valid_lft, HZ);
+		/*
+		 * Note: Because this address is
+		 * not permanent, lifetime <
+		 * LONG_MAX / HZ here.
+		 */
+		if (time_before(*expires, ifa->tstamp + lifetime * HZ))
+			*expires = ifa->tstamp + lifetime * HZ;
+		spin_unlock(&ifa->lock);
+	}
+
+	return onlink;
+}
+
+static void
+cleanup_prefix_route(struct inet6_ifaddr *ifp, unsigned long expires, int onlink)
+{
+	struct rt6_info *rt;
+
+	if (onlink >= 1)
+		return;
+
+	rt = addrconf_get_prefix_route(&ifp->addr,
+				       ifp->prefix_len,
+				       ifp->idev->dev,
+				       0, RTF_GATEWAY | RTF_DEFAULT);
+
+	if (rt) {
+		if (onlink == 0) {
+			ip6_del_rt(rt);
+			rt = NULL;
+		} else if (!(rt->rt6i_flags & RTF_EXPIRES)) {
+			rt6_set_expires(rt, expires);
+		}
+	}
+	ip6_rt_put(rt);
+}
+
+
 /* This function wants to get referenced ifp and releases it before return */
 
 static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 {
-	struct inet6_ifaddr *ifa, *ifn;
-	struct inet6_dev *idev = ifp->idev;
 	int state;
-	int deleted = 0, onlink = 0;
-	unsigned long expires = jiffies;
+	int onlink;
+	unsigned long expires;
 
 	spin_lock_bh(&ifp->state_lock);
 	state = ifp->state;
@@ -922,7 +1013,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 	hlist_del_init_rcu(&ifp->addr_lst);
 	spin_unlock_bh(&addrconf_hash_lock);
 
-	write_lock_bh(&idev->lock);
+	write_lock_bh(&ifp->idev->lock);
 
 	if (ifp->flags&IFA_F_TEMPORARY) {
 		list_del(&ifp->tmp_list);
@@ -933,45 +1024,11 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 		__in6_ifa_put(ifp);
 	}
 
-	list_for_each_entry_safe(ifa, ifn, &idev->addr_list, if_list) {
-		if (ifa == ifp) {
-			list_del_init(&ifp->if_list);
-			__in6_ifa_put(ifp);
+	onlink = check_cleanup_prefix_routes(ifp, ifp->flags, &expires);
+	list_del_init(&ifp->if_list);
+	__in6_ifa_put(ifp);
 
-			if (!(ifp->flags & IFA_F_PERMANENT) || onlink > 0)
-				break;
-			deleted = 1;
-			continue;
-		} else if (ifp->flags & IFA_F_PERMANENT) {
-			if (ipv6_prefix_equal(&ifa->addr, &ifp->addr,
-					      ifp->prefix_len)) {
-				if (ifa->flags & IFA_F_PERMANENT) {
-					onlink = 1;
-					if (deleted)
-						break;
-				} else {
-					unsigned long lifetime;
-
-					if (!onlink)
-						onlink = -1;
-
-					spin_lock(&ifa->lock);
-
-					lifetime = addrconf_timeout_fixup(ifa->valid_lft, HZ);
-					/*
-					 * Note: Because this address is
-					 * not permanent, lifetime <
-					 * LONG_MAX / HZ here.
-					 */
-					if (time_before(expires,
-							ifa->tstamp + lifetime * HZ))
-						expires = ifa->tstamp + lifetime * HZ;
-					spin_unlock(&ifa->lock);
-				}
-			}
-		}
-	}
-	write_unlock_bh(&idev->lock);
+	write_unlock_bh(&ifp->idev->lock);
 
 	addrconf_del_dad_timer(ifp);
 
@@ -979,39 +1036,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 
 	inet6addr_notifier_call_chain(NETDEV_DOWN, ifp);
 
-	/*
-	 * Purge or update corresponding prefix
-	 *
-	 * 1) we don't purge prefix here if address was not permanent.
-	 *    prefix is managed by its own lifetime.
-	 * 2) if there're no addresses, delete prefix.
-	 * 3) if there're still other permanent address(es),
-	 *    corresponding prefix is still permanent.
-	 * 4) otherwise, update prefix lifetime to the
-	 *    longest valid lifetime among the corresponding
-	 *    addresses on the device.
-	 *    Note: subsequent RA will update lifetime.
-	 *
-	 * --yoshfuji
-	 */
-	if ((ifp->flags & IFA_F_PERMANENT) && onlink < 1) {
-		struct rt6_info *rt;
-
-		rt = addrconf_get_prefix_route(&ifp->addr,
-					       ifp->prefix_len,
-					       ifp->idev->dev,
-					       0, RTF_GATEWAY | RTF_DEFAULT);
-
-		if (rt) {
-			if (onlink == 0) {
-				ip6_del_rt(rt);
-				rt = NULL;
-			} else if (!(rt->rt6i_flags & RTF_EXPIRES)) {
-				rt6_set_expires(rt, expires);
-			}
-		}
-		ip6_rt_put(rt);
-	}
+	cleanup_prefix_route(ifp, expires, onlink);
 
 	/* clean up prefsrc entries */
 	rt6_remove_prefsrc(ifp);
@@ -3632,6 +3657,7 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 	clock_t expires;
 	unsigned long timeout;
 	bool was_managetempaddr;
+	bool was_noprefixroute;
 
 	if (!valid_lft || (prefered_lft > valid_lft))
 		return -EINVAL;
@@ -3660,6 +3686,7 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 
 	spin_lock_bh(&ifp->lock);
 	was_managetempaddr = ifp->flags & IFA_F_MANAGETEMPADDR;
+	was_noprefixroute = ifp->flags & IFA_F_NOPREFIXROUTE;
 	ifp->flags &= ~(IFA_F_DEPRECATED | IFA_F_PERMANENT | IFA_F_NODAD |
 			IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR | IFA_F_NOPREFIXROUTE);
 	ifp->flags |= ifa_flags;
@@ -3674,6 +3701,15 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
 	if (!(ifa_flags & IFA_F_NOPREFIXROUTE)) {
 		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, ifp->idev->dev,
 				      expires, flags);
+	} else if (was_noprefixroute) {
+		int onlink;
+		unsigned long expires;
+
+		write_lock_bh(&ifp->idev->lock);
+		onlink = check_cleanup_prefix_routes(ifp, ifp->flags & ~IFA_F_NOPREFIXROUTE, &expires);
+		write_unlock_bh(&ifp->idev->lock);
+
+		cleanup_prefix_route(ifp, expires, onlink);
 	}
 
 	if (was_managetempaddr || ifp->flags & IFA_F_MANAGETEMPADDR) {
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
  2014-01-07 14:39                       ` [PATCH v2 1/2] " Thomas Haller
  2014-01-07 14:39                       ` [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE Thomas Haller
@ 2014-01-07 16:03                       ` Hannes Frederic Sowa
  2014-01-07 21:42                         ` Thomas Haller
  2 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 16:03 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

On Tue, Jan 07, 2014 at 03:39:11PM +0100, Thomas Haller wrote:
> Now, the IFA_F_NOPREFIXROUTE flag is saved in ifp->flags.
> The second patch reworks the deletion of addresses/cleanup of prefix
> routes and considers IFA_F_NOPREFIXROUTE.
> 
> Thomas Haller (2):
>   ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of
>     IP6 routes
>   ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE

Maybe you could look into if a small patch implementing noprefixroute would be
doable. That would help testing these changes a lot. ;)

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 14:39                       ` [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE Thomas Haller
@ 2014-01-07 16:28                         ` Hannes Frederic Sowa
  2014-01-07 18:32                           ` Thomas Haller
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 16:28 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

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?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 16:28                         ` Hannes Frederic Sowa
@ 2014-01-07 18:32                           ` Thomas Haller
  2014-01-07 19:01                             ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 18:32 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

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

Hi,

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"?


For v3 I will reword the commit message. How about the following:

    ipv6 addrconf: don't cleanup prefix route for IFA_F_NOPREFIXROUTE
    
    Refactor the deletion/update of prefix routes when removing an
    address. Now, consider IFA_F_NOPREFIXROUTE and if there is an address
    present with this flag, to not cleanup the route. Instead, assume
    that userspace is taking care of this prefix.
    
    Also perform the same cleanup, when userspace changes an existing address
    to add NOPREFIXROUTE to an address that didn't have this flag. We do this
    because when the address was added, a prefix route was created for it.
    Since the user now wants to handle this route by himself, we remove it again.
    
    As before, a prefix route only gets removed, if there is no address
    that might need it. Or, if there are only non-permanent addresses,
    update the lifetime of the route.


ciao,
Thomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 18:32                           ` Thomas Haller
@ 2014-01-07 19:01                             ` Hannes Frederic Sowa
  2014-01-07 22:54                               ` Thomas Haller
  0 siblings, 1 reply; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 19:01 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

Hi,

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:
> 
>     ipv6 addrconf: don't cleanup prefix route for IFA_F_NOPREFIXROUTE
>     
>     Refactor the deletion/update of prefix routes when removing an
>     address. Now, consider IFA_F_NOPREFIXROUTE and if there is an address
>     present with this flag, to not cleanup the route. Instead, assume
>     that userspace is taking care of this prefix.
>     
>     Also perform the same cleanup, when userspace changes an existing address
>     to add NOPREFIXROUTE to an address that didn't have this flag. We do this
>     because when the address was added, a prefix route was created for it.
>     Since the user now wants to handle this route by himself, we remove it again.
>     
>     As before, a prefix route only gets removed, if there is no address
>     that might need it. Or, if there are only non-permanent addresses,
>     update the lifetime of the route.

If we want go with the current modify behavior this sounds good.

Thanks,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes
  2014-01-07 16:03                       ` [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Hannes Frederic Sowa
@ 2014-01-07 21:42                         ` Thomas Haller
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 21:42 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Tue, 2014-01-07 at 17:03 +0100, Hannes Frederic Sowa wrote:
> On Tue, Jan 07, 2014 at 03:39:11PM +0100, Thomas Haller wrote:
> > Now, the IFA_F_NOPREFIXROUTE flag is saved in ifp->flags.
> > The second patch reworks the deletion of addresses/cleanup of prefix
> > routes and considers IFA_F_NOPREFIXROUTE.
> > 
> > Thomas Haller (2):
> >   ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of
> >     IP6 routes
> >   ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
> 
> Maybe you could look into if a small patch implementing noprefixroute would be
> doable. That would help testing these changes a lot. ;)
> 
> Greetings,
> 
>   Hannes
> 

Just for reference:

I sent an email "[patch iproute2 v3 3/3] add support for IFA_F_NOPREFIXROUTE"
with a patch for iproute2.


Thomas



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 19:01                             ` Hannes Frederic Sowa
@ 2014-01-07 22:54                               ` Thomas Haller
  2014-01-07 23:09                                 ` Hannes Frederic Sowa
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Haller @ 2014-01-07 22:54 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Jiri Pirko, netdev, stephen, dcbw

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE
  2014-01-07 22:54                               ` Thomas Haller
@ 2014-01-07 23:09                                 ` Hannes Frederic Sowa
  0 siblings, 0 replies; 30+ messages in thread
From: Hannes Frederic Sowa @ 2014-01-07 23:09 UTC (permalink / raw)
  To: Thomas Haller; +Cc: Jiri Pirko, netdev, stephen, dcbw

On Tue, Jan 07, 2014 at 11:54:22PM +0100, Thomas Haller wrote:
> 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?

The ifp->rt thing, I thought of, does not work. It only holds the RTF_LOCAL
route (over loopback) which has nothing to do with the prefix route. We don't
have a link from ifp to the prefix route.

Currently I am fine with the semantics you described above but won't
have time to review them today. I'll do that tomorrow.

Thanks,

  Hannes

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2014-01-07 23:09 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-02 15:34 [patch iproute2 v2 0/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
2014-01-02 15:34 ` [patch iproute2 v2 1/2] add support for extended ifa_flags Jiri Pirko
2014-01-02 15:34 ` [patch iproute2 v2 2/2] add support for IFA_F_MANAGETEMPADDR Jiri Pirko
2014-01-02 15:50   ` [PATCH 1/1] fixup! " Thomas Haller
2014-01-04 10:44     ` Jiri Pirko
2014-01-02 17:29 ` [patch iproute2 v2 0/2] " Hannes Frederic Sowa
2014-01-04 10:43   ` Jiri Pirko
2014-01-04 10:55     ` Hannes Frederic Sowa
2014-01-04 11:05       ` Jiri Pirko
2014-01-04 11:15         ` Hannes Frederic Sowa
2014-01-04 11:21           ` Thomas Haller
2014-01-04 11:35             ` Hannes Frederic Sowa
2014-01-06 15:41               ` Thomas Haller
2014-01-06 16:01                 ` Hannes Frederic Sowa
2014-01-06 17:29                   ` [PATCH 1/1] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Thomas Haller
2014-01-06 17:38                     ` Jiri Pirko
2014-01-07  9:39                       ` Hannes Frederic Sowa
2014-01-07 12:01                     ` Hannes Frederic Sowa
2014-01-07 12:14                       ` Thomas Haller
2014-01-07 12:22                         ` Hannes Frederic Sowa
2014-01-07 14:39                     ` [PATCH v2 0/2] " Thomas Haller
2014-01-07 14:39                       ` [PATCH v2 1/2] " Thomas Haller
2014-01-07 14:39                       ` [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE Thomas Haller
2014-01-07 16:28                         ` Hannes Frederic Sowa
2014-01-07 18:32                           ` Thomas Haller
2014-01-07 19:01                             ` Hannes Frederic Sowa
2014-01-07 22:54                               ` Thomas Haller
2014-01-07 23:09                                 ` Hannes Frederic Sowa
2014-01-07 16:03                       ` [PATCH v2 0/2] ipv6 addrconf: add IFA_F_NOPREFIXROUTE flag to suppress creation of IP6 routes Hannes Frederic Sowa
2014-01-07 21:42                         ` Thomas Haller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).