diff mbox

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

Message ID 4C76B770.4030800@candelatech.com
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Ben Greear Aug. 26, 2010, 6:50 p.m. UTC 
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

Comments

Brian Haley Aug. 26, 2010, 7:57 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 26, 2010, 8:18 p.m. UTC | #2 
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 :)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Aug. 26, 2010, 9:19 p.m. UTC | #3 
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 Aug. 26, 2010, 9:22 p.m. UTC | #4 
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
David Miller Aug. 26, 2010, 9:27 p.m. UTC | #5 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Aug. 27, 2010, 4:24 a.m. UTC | #6 
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
diff mbox

Patch

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);
         }