[net-next] ipv6: Enable netlink notification for tentative addresses.

[net-next] ipv6: Enable netlink notification for tentative addresses.

[Ben Greear]

By default, netlink messages are not sent when an IPv6 address
is added if it is in tentative state.  This makes it harder
for user-space applications to know the current state of the
IPv6 addresses.  This patch adds an ipv6 sysctl that will
allow tentative address notifications to be sent.  The sysctl
is off by default.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 f350c69... 3f223d2... M	Documentation/networking/ip-sysctl.txt
:100644 100644 e62683b... 7912172... M	include/linux/ipv6.h
:100644 100644 ab70a3f... ec8b66a... M	net/ipv6/addrconf.c
 Documentation/networking/ip-sysctl.txt |   12 ++++++++++++
 include/linux/ipv6.h                   |    6 ++++++
 net/ipv6/addrconf.c                    |   19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index f350c69..3f223d2 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1005,6 +1005,18 @@ conf/all/forwarding - BOOLEAN
 proxy_ndp - BOOLEAN
 	Do proxy ndp.
 
+nlnotify_on_addr_add - BOOLEAN
+        By default, netlink messages are not sent when an IPv6 address
+	is added if it is in tentative state.  This makes it harder
+	for some user-space applications to function properly.  To
+	ensure that a netlink message is always sent when an IPv6 addr
+	is added, regardless of the state of the address, set this value
+	to 1.  For the old (default) behaviour, set this value to 0.
+
+	If only certain interfaces should have this behaviour, leave the
+	'all' config set to 0 and set the individual interface's value
+	to 1.
+
 conf/interface/*:
 	Change special settings per interface.
 
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index e62683b..7912172 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -163,6 +163,11 @@ struct ipv6_devconf {
 #endif
 	__s32		proxy_ndp;
 	__s32		accept_source_route;
+	__s32		nlnotify_on_addr_add; /* Always notify netlink on addr add, even if it is tentative.
+					       * As currently implemented, this will often cause multiple netlink
+					       * RTM_NEWADDR messages, as a new notification will be sent when
+					       * the address becomes un-tentative.
+					       */
 #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
 	__s32		optimistic_dad;
 #endif
@@ -213,6 +218,7 @@ enum {
 	DEVCONF_DISABLE_IPV6,
 	DEVCONF_ACCEPT_DAD,
 	DEVCONF_FORCE_TLLAO,
+	DEVCONF_NLNOTIFY_ON_ADDR_ADD,
 	DEVCONF_MAX
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab70a3f..ec8b66a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -125,6 +125,7 @@ static void ipv6_regen_rndid(unsigned long data);
 
 static int ipv6_generate_eui64(u8 *eui, struct net_device *dev);
 static int ipv6_count_addresses(struct inet6_dev *idev);
+static void inet6_ifa_notify(int event, struct inet6_ifaddr *ifa);
 
 /*
  *	Configured unicast address hash table
@@ -192,6 +193,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.accept_source_route	= 0,	/* we do not accept RH0 by default. */
 	.disable_ipv6		= 0,
 	.accept_dad		= 1,
+	.nlnotify_on_addr_add	= 0,
 };
 
 static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
@@ -226,6 +228,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.accept_source_route	= 0,	/* we do not accept RH0 by default. */
 	.disable_ipv6		= 0,
 	.accept_dad		= 1,
+	.nlnotify_on_addr_add   = 0,  /* by default, only notify when it becomes un-tentative */
 };
 
 /* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
@@ -704,6 +707,13 @@ out2:
 		ifa = ERR_PTR(err);
 	}
 
+	/* Allow netlink notification of all addresses, regardless of flags. */
+	if (ipv6_devconf.nlnotify_on_addr_add || idev->cnf.nlnotify_on_addr_add) {
+		if (!IS_ERR(ifa)) {
+			inet6_ifa_notify(RTM_NEWADDR, ifa);
+		}
+	}
+	
 	return ifa;
 out:
 	spin_unlock(&addrconf_hash_lock);
@@ -3833,6 +3843,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_DISABLE_IPV6] = cnf->disable_ipv6;
 	array[DEVCONF_ACCEPT_DAD] = cnf->accept_dad;
 	array[DEVCONF_FORCE_TLLAO] = cnf->force_tllao;
+	array[DEVCONF_NLNOTIFY_ON_ADDR_ADD] = cnf->nlnotify_on_addr_add;
 }
 
 static inline size_t inet6_if_nlmsg_size(void)
@@ -4412,6 +4423,14 @@ static struct addrconf_sysctl_table
 			.mode		= 0644,
 			.proc_handler	= proc_dointvec,
 		},
+		{
+			.procname       =       "nlnotify_on_addr_add",
+			.data           =       &ipv6_devconf.nlnotify_on_addr_add,
+			.maxlen         =       sizeof(int),
+			.mode           =       0644,
+			.proc_handler   =       &proc_dointvec,
+
+		},
 #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
 		{
 			.procname       = "optimistic_dad",
-- 
1.6.2.5

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Wed, 25 Aug 2010 11:26:17 -0700

> By default, netlink messages are not sent when an IPv6 address
> is added if it is in tentative state.  This makes it harder
> for user-space applications to know the current state of the
> IPv6 addresses.  This patch adds an ipv6 sysctl that will
> allow tentative address notifications to be sent.  The sysctl
> is off by default.
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>

It's inconsistent to send two NEWADDR events for the same add.

I would advise that we unconditionally do the NEWADDR once,
when the tentative state address is added, and completely
elide the one we current send when it leaves tentative state.

Having a sysctl, and having it off by default, just means you haven't
actually fixes the problem.  Since it's only fixed if someone makes a
non-standard configuration change.

Otherwise what you're saying is that this is a very obscure thing for
very obscure applications, and it very nearly doesn't even matter
as a result.

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[Ben Greear]

On 08/25/2010 09:24 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Wed, 25 Aug 2010 11:26:17 -0700
>
>> By default, netlink messages are not sent when an IPv6 address
>> is added if it is in tentative state.  This makes it harder
>> for user-space applications to know the current state of the
>> IPv6 addresses.  This patch adds an ipv6 sysctl that will
>> allow tentative address notifications to be sent.  The sysctl
>> is off by default.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>
> It's inconsistent to send two NEWADDR events for the same add.

The ipv6 code seems to send a NEWADDR message every time there
is a flag change for the IPv6 addresses.  I suppose this better
lets code that cares know the state of things.

The patch below should always send an even on add, but it will
keep all the other events.  If you really think I should
elide some of the others, I'll make the change, but I think
it might be a bad idea.

If the patch below looks ok as is, let me know and I'll
resend it as a git patch.

Thanks,
Ben

[greearb@ben-dt2 net-next-2.6]$ git diff
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab70a3f..7aa7535 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -125,6 +125,7 @@ static void ipv6_regen_rndid(unsigned long data);

  static int ipv6_generate_eui64(u8 *eui, struct net_device *dev);
  static int ipv6_count_addresses(struct inet6_dev *idev);
+static void inet6_ifa_notify(int event, struct inet6_ifaddr *ifa);

  /*
   *     Configured unicast address hash table
@@ -697,9 +698,10 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, int pfxlen,
  out2:
         rcu_read_unlock_bh();

-       if (likely(err == 0))
+       if (likely(err == 0)) {
                 atomic_notifier_call_chain(&inet6addr_chain, NETDEV_UP, ifa);
-       else {
+               inet6_ifa_notify(RTM_NEWADDR, ifa);
+       } else {
                 kfree(ifa);
                 ifa = ERR_PTR(err);
         }


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[Brian Haley]

On 08/26/2010 02:50 PM, Ben Greear wrote:
> On 08/25/2010 09:24 PM, David Miller wrote:
>> From: Ben Greear<greearb@candelatech.com>
>> Date: Wed, 25 Aug 2010 11:26:17 -0700
>>
>>> By default, netlink messages are not sent when an IPv6 address
>>> is added if it is in tentative state.  This makes it harder
>>> for user-space applications to know the current state of the
>>> IPv6 addresses.  This patch adds an ipv6 sysctl that will
>>> allow tentative address notifications to be sent.  The sysctl
>>> is off by default.
>>>
>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>
>> It's inconsistent to send two NEWADDR events for the same add.
>>
>> I would advise that we unconditionally do the NEWADDR once,
>> when the tentative state address is added, and completely
>> elide the one we current send when it leaves tentative state.

But then we get a message for an address that can't be used because
it hasn't passed DAD, I'm not so sure that is a good thing,
especially if we don't get notified when it passes DAD.

> @@ -697,9 +698,10 @@ ipv6_add_addr(struct inet6_dev *idev, const struct
> in6_addr *addr, int pfxlen,
>  out2:
>         rcu_read_unlock_bh();
> 
> -       if (likely(err == 0))
> +       if (likely(err == 0)) {
>                 atomic_notifier_call_chain(&inet6addr_chain, NETDEV_UP,
> ifa);
> -       else {
> +               inet6_ifa_notify(RTM_NEWADDR, ifa);
> +       } else {
>                 kfree(ifa);
>                 ifa = ERR_PTR(err);
>         }

This will generate two messages in some cases, for example, when lo is
configured, or a SIT tunnel is added, see add_addr() in addrconf.c.

-Brian

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Thu, 26 Aug 2010 15:57:51 -0400

> But then we get a message for an address that can't be used because
> it hasn't passed DAD, I'm not so sure that is a good thing,
> especially if we don't get notified when it passes DAD.

I think that we shouldn't send notifications for an address that can't
even be used.  So essentially I argue against this patch in any form :)

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[Ben Greear]

On 08/26/2010 01:18 PM, David Miller wrote:
> From: Brian Haley<brian.haley@hp.com>
> Date: Thu, 26 Aug 2010 15:57:51 -0400
>
>> But then we get a message for an address that can't be used because
>> it hasn't passed DAD, I'm not so sure that is a good thing,
>> especially if we don't get notified when it passes DAD.
>
> I think that we shouldn't send notifications for an address that can't
> even be used.  So essentially I argue against this patch in any form :)

In our case, we enable IPv6 in user-space, and then we want to get some
immediate indication that indeed the process is working as expected.

In other cases, we want to remove all IPv6 addresses, so if we have not
even been notified that the IP exists, then we cannot know how to delete it.

In my patch, we still get the update when DAD completes, so applications
can take note of the flags if they care about the details.

We've been running this patch for several years, and it has not caused
any obvious problems with other tools, so I think it's safe enough.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[Ben Greear]

On 08/26/2010 12:57 PM, Brian Haley wrote:
> On 08/26/2010 02:50 PM, Ben Greear wrote:

>> @@ -697,9 +698,10 @@ ipv6_add_addr(struct inet6_dev *idev, const struct
>> in6_addr *addr, int pfxlen,
>>   out2:
>>          rcu_read_unlock_bh();
>>
>> -       if (likely(err == 0))
>> +       if (likely(err == 0)) {
>>                  atomic_notifier_call_chain(&inet6addr_chain, NETDEV_UP,
>> ifa);
>> -       else {
>> +               inet6_ifa_notify(RTM_NEWADDR, ifa);
>> +       } else {
>>                  kfree(ifa);
>>                  ifa = ERR_PTR(err);
>>          }
>
> This will generate two messages in some cases, for example, when lo is
> configured, or a SIT tunnel is added, see add_addr() in addrconf.c.

Would that cause any problem though?  It seems a common pattern to
send 'new-foo' netlink messages when some value in foo changes.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Thu, 26 Aug 2010 14:19:30 -0700

> In our case, we enable IPv6 in user-space, and then we want to get
> some immediate indication that indeed the process is working as
> expected.

When DAD completes, you'll get a similar indication.

> In other cases, we want to remove all IPv6 addresses, so if we have
> not even been notified that the IP exists, then we cannot know how
> to delete it.

You can add a netlink message to accomplish that.  This has been
asked for in other contexts as well.

> We've been running this patch for several years, and it has not
> caused any obvious problems with other tools, so I think it's safe
> enough.

And your level of exposure compared to upstream is...? :-)

Anyways, even if it's implemented in an error free way it's still
not necessary the best way to go about this.

Re: [net-next] ipv6: Enable netlink notification for tentative addresses.

[Ben Greear]

On 08/26/2010 02:27 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Thu, 26 Aug 2010 14:19:30 -0700
>
>> In our case, we enable IPv6 in user-space, and then we want to get
>> some immediate indication that indeed the process is working as
>> expected.
>
> When DAD completes, you'll get a similar indication.
>
>> In other cases, we want to remove all IPv6 addresses, so if we have
>> not even been notified that the IP exists, then we cannot know how
>> to delete it.
>
> You can add a netlink message to accomplish that.  This has been
> asked for in other contexts as well.
>
>> We've been running this patch for several years, and it has not
>> caused any obvious problems with other tools, so I think it's safe
>> enough.
>
> And your level of exposure compared to upstream is...? :-)
>
> Anyways, even if it's implemented in an error free way it's still
> not necessary the best way to go about this.

Ok, lets not worry about this patch any more then.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com