[Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[Cong Wang]

It is possible that the interface is already gone after joining
the list of anycast on this interface as we don't hold a refcount
for the device, in this case we are safe to ignore the error.

What's more important, for API compatibility we should not
change this behavior for applications even if it were correct.

Fixes: commit a9ed4a2986e13011 ("ipv6: fix rtnl locking in setsockopt for anycast and multicast")
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ipv6/anycast.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 45b9d81..ff2de7d 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -182,8 +182,6 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	rtnl_unlock();
 
 	sock_kfree_s(sk, pac, sizeof(*pac));
-	if (!dev)
-		return -ENODEV;
 	return 0;
 }
 
-- 
1.8.3.1

Re: [Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[Hannes Frederic Sowa]

Hi Cong,

Just doing normal review, really no bad intentions, just technical
follow-up. ;)

On Fri, Sep 5, 2014, at 23:33, Cong Wang wrote:
> It is possible that the interface is already gone after joining
> the list of anycast on this interface as we don't hold a refcount
> for the device, in this case we are safe to ignore the error.

anycast code actually inserts routes into the routing table and holds a
reference on the interface while that route is active.

> What's more important, for API compatibility we should not
> change this behavior for applications even if it were correct.

IMHO adding new error codes never breaks existing applications because
there is no way they can explore all possible errno variables. Also we
already report ENODEV from multicast setsockopts. So I think it would be
ok to leave it as is, but I have no strong opinion on that and it would
be ok by me if the patch got accepted (maybe update the changelog).

Thanks,
Hannes

Re: [Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[Cong Wang]

On Fri, Sep 5, 2014 at 3:26 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi Cong,
>
> Just doing normal review, really no bad intentions, just technical
> follow-up. ;)

I believe you are not supposed to do it since definitely you
said you don't want to work with me. You said it, not me,
I respect any of your choice. :)

>
> On Fri, Sep 5, 2014, at 23:33, Cong Wang wrote:
>> It is possible that the interface is already gone after joining
>> the list of anycast on this interface as we don't hold a refcount
>> for the device, in this case we are safe to ignore the error.
>
> anycast code actually inserts routes into the routing table and holds a
> reference on the interface while that route is active.

But route could be deleted.

>
>> What's more important, for API compatibility we should not
>> change this behavior for applications even if it were correct.
>
> IMHO adding new error codes never breaks existing applications because
> there is no way they can explore all possible errno variables. Also we
> already report ENODEV from multicast setsockopts. So I think it would be
> ok to leave it as is, but I have no strong opinion on that and it would
> be ok by me if the patch got accepted (maybe update the changelog).
>

Again, too late to change, the code has been there since the beginning
of git history. You should have a strong argument if you want to change it,
otherwise restore old behavior, that is all. Obviously you don't even mention
this when you accept that patch.

Re: [Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[Hannes Frederic Sowa]

On Sat, Sep 6, 2014, at 00:41, Cong Wang wrote:
> On Fri, Sep 5, 2014 at 3:26 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi Cong,
> >
> > Just doing normal review, really no bad intentions, just technical
> > follow-up. ;)
> 
> I believe you are not supposed to do it since definitely you
> said you don't want to work with me. You said it, not me,
> I respect any of your choice. :)

Oh no, you misunderstood me (maybe my bad). My only critique was that
you could have stated that you also saw a problem with the ENODEV return
before the patch was looked at by David.
Sure, let's forget that and work together.

> > On Fri, Sep 5, 2014, at 23:33, Cong Wang wrote:
> >> It is possible that the interface is already gone after joining
> >> the list of anycast on this interface as we don't hold a refcount
> >> for the device, in this case we are safe to ignore the error.
> >
> > anycast code actually inserts routes into the routing table and holds a
> > reference on the interface while that route is active.
> 
> But route could be deleted.

The routes are managed by the inet6_dev and it will drop the routes on
interface shutdown, correct. There is no strong reference from socket to
the routing entry. Only deleting a route should not work as it will
still be referenced by the anycast list in inet6_dev (though it won't
get used anymore).

> >> What's more important, for API compatibility we should not
> >> change this behavior for applications even if it were correct.
> >
> > IMHO adding new error codes never breaks existing applications because
> > there is no way they can explore all possible errno variables. Also we
> > already report ENODEV from multicast setsockopts. So I think it would be
> > ok to leave it as is, but I have no strong opinion on that and it would
> > be ok by me if the patch got accepted (maybe update the changelog).
> >
> 
> Again, too late to change, the code has been there since the beginning
> of git history. You should have a strong argument if you want to change
> it,
> otherwise restore old behavior, that is all. Obviously you don't even
> mention
> this when you accept that patch.

Let's settle this:
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Bye,
Hannes

Re: [Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri,  5 Sep 2014 14:33:00 -0700

> It is possible that the interface is already gone after joining
> the list of anycast on this interface as we don't hold a refcount
> for the device, in this case we are safe to ignore the error.
> 
> What's more important, for API compatibility we should not
> change this behavior for applications even if it were correct.
> 
> Fixes: commit a9ed4a2986e13011 ("ipv6: fix rtnl locking in setsockopt for anycast and multicast")
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks.