[PATCH v2 0/5] ipvs: fix backup sync daemon with IPv6, and minor updates

[PATCH v2 0/5] ipvs: fix backup sync daemon with IPv6, and minor updates

[Quentin Armitage]

This series of patches arise from discovering that:
ipvsadm --start-daemon backup --mcast-group IPv6_address ...
would always fail.

The first patch resolves the problem. The second and third patches are
optimizations that were noticed while investigating the original problem.
The fourth patch adds a lock which appears to have been omitted, and the
final patch adds the recently added sync daemon multicast parameters to
the log messages that are written when the sync daemons start.

v2 fixes a compile error in a debug message identified by kbuild test robot.
Now compiles with CONFIG_IP_VS_DEBUG enabled. Patch 2/5 is modified to correct
the problem, and patch 3/5 is modifed to apply with the modified patch 2/5.

Quentin Armitage (5):
  ipvs: Enable setting IPv6 multicast address for ipvs sync daemon
    backup
  ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync
    daemon
  ipvs: Don't check result < 0 after setting result = 0
  ipvs: Lock socket before setting SK_CAN_REUSE
  ipvs: log additional sync daemon parameters

 net/netfilter/ipvs/ip_vs_sync.c |  104 +++++++++++++++++++-------------------
 1 files changed, 52 insertions(+), 52 deletions(-)

-- 
1.7.7.6

[PATCH v2 1/5] ipvs: Enable setting IPv6 multicast address for ipvs sync daemon backup

[Quentin Armitage]

When using HEAD from https://git.kernel.org/cgit/utils/kernel/ipvsadm/ipvsadm.git/,
the command:
ipvsadm --start-daemon backup --mcast-interface eth0.60 --mcast-group ff01::1:81
fails with the error message:
Argument list too long

whereas both:
ipvsadm --start-daemon master --mcast-interface eth0.60 --mcast-group ff01::1:81
and:
ipvsadm --start-daemon backup --mcast-interface eth0.60 --mcast-group 224.0.0.81
are successful.

The error message "Argument list too long" isn't helpful. The error occurs
because an IPv6 address is given in backup mode.

The error is in make_receive_sock() in net/netfilter/ipvs/ip_vs_sync.c, since
it fails to set the interface on the address or the socket before calling
inet6_bind() (via sock->ops->bind), where the test 'if (!sk->sk_bound_dev_if)'
failed.

Setting sin6_scope_id on the sockaddr before calling inet6_bind() resolves
the issue.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..85b48f1 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1545,7 +1545,7 @@ error:
 /*
  *      Set up receiving multicast socket over UDP
  */
-static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id, int ifindex)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1566,6 +1566,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 0, result);
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->bcfg, id);
+	((struct sockaddr_in6 *)&mcast_addr)->sin6_scope_id = ifindex;
 	result = sock->ops->bind(sock, (struct sockaddr *)&mcast_addr, salen);
 	if (result < 0) {
 		pr_err("Error binding to the multicast addr\n");
@@ -1868,7 +1869,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 		if (state == IP_VS_STATE_MASTER)
 			sock = make_send_sock(ipvs, id);
 		else
-			sock = make_receive_sock(ipvs, id);
+			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
 			result = PTR_ERR(sock);
 			goto outtinfo;
-- 
1.7.7.6

[PATCH v2 2/5] ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync daemon

[Quentin Armitage]

Optimise starting sync daemons by using the result of the first call to
__dev_get_by_name() and pass the result or ifindex to subsequent functions
to avoid them having to call __dev_get_by_name() again.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   59 ++++++++++++--------------------------
 1 files changed, 19 insertions(+), 40 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 85b48f1..d52cc25 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1356,28 +1356,22 @@ static void set_mcast_pmtudisc(struct sock *sk, int val)
 /*
  *      Specifiy default interface for outgoing multicasts
  */
-static int set_mcast_if(struct sock *sk, char *ifname)
+static int set_mcast_if(struct sock *sk, int ifindex)
 {
-	struct net_device *dev;
 	struct inet_sock *inet = inet_sk(sk);
-	struct net *net = sock_net(sk);
-
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
 
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	inet->mc_index = dev->ifindex;
+	inet->mc_index = ifindex;
 	/*  inet->mc_addr  = 0; */
 #ifdef CONFIG_IP_VS_IPV6
 	if (sk->sk_family == AF_INET6) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 
 		/* IPV6_MULTICAST_IF */
-		np->mcast_oif = dev->ifindex;
+		np->mcast_oif = ifindex;
 	}
 #endif
 	release_sock(sk);
@@ -1392,23 +1386,18 @@ static int set_mcast_if(struct sock *sk, char *ifname)
  *      in the in_addr structure passed in as a parameter.
  */
 static int
-join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
+join_mcast_group(struct sock *sk, struct in_addr *addr, int ifindex)
 {
-	struct net *net = sock_net(sk);
 	struct ip_mreqn mreq;
-	struct net_device *dev;
 	int ret;
 
 	memset(&mreq, 0, sizeof(mreq));
 	memcpy(&mreq.imr_multiaddr, addr, sizeof(struct in_addr));
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
-	mreq.imr_ifindex = dev->ifindex;
+	mreq.imr_ifindex = ifindex;
 
 	lock_sock(sk);
 	ret = ip_mc_join_group(sk, &mreq);
@@ -1419,44 +1408,33 @@ join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
 
 #ifdef CONFIG_IP_VS_IPV6
 static int join_mcast_group6(struct sock *sk, struct in6_addr *addr,
-			     char *ifname)
+			     int ifindex)
 {
-	struct net *net = sock_net(sk);
-	struct net_device *dev;
 	int ret;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	ret = ipv6_sock_mc_join(sk, dev->ifindex, addr);
+	ret = ipv6_sock_mc_join(sk, ifindex, addr);
 	release_sock(sk);
 
 	return ret;
 }
 #endif
 
-static int bind_mcastif_addr(struct socket *sock, char *ifname)
+static int bind_mcastif_addr(struct socket *sock, struct net_device *dev)
 {
-	struct net *net = sock_net(sock->sk);
-	struct net_device *dev;
 	__be32 addr;
 	struct sockaddr_in sin;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-
 	addr = inet_select_addr(dev, 0, RT_SCOPE_UNIVERSE);
 	if (!addr)
 		pr_err("You probably need to specify IP address on "
 		       "multicast interface.\n");
 
 	IP_VS_DBG(7, "binding socket with (%s) %pI4\n",
-		  ifname, &addr);
+		  dev->name, &addr);
 
 	/* Now bind the socket with the address of multicast interface */
 	sin.sin_family	     = AF_INET;
@@ -1489,7 +1467,7 @@ static void get_mcast_sockaddr(union ipvs_sockaddr *sa, int *salen,
 /*
  *      Set up sending multicast socket over UDP
  */
-static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id, struct net_device *dev)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1503,7 +1481,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
-	result = set_mcast_if(sock->sk, ipvs->mcfg.mcast_ifn);
+	result = set_mcast_if(sock->sk, dev->ifindex);
 	if (result < 0) {
 		pr_err("Error setting outbound mcast interface\n");
 		goto error;
@@ -1518,7 +1496,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 1, result);
 
 	if (AF_INET == ipvs->mcfg.mcast_af)
-		result = bind_mcastif_addr(sock, ipvs->mcfg.mcast_ifn);
+		result = bind_mcastif_addr(sock, dev);
 	else
 		result = 0;
 	if (result < 0) {
@@ -1559,6 +1537,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id, int ifi
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
+
 	/* it is equivalent to the REUSEADDR option in user-space */
 	sock->sk->sk_reuse = SK_CAN_REUSE;
 	result = sysctl_sync_sock_size(ipvs);
@@ -1577,11 +1556,11 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id, int ifi
 #ifdef CONFIG_IP_VS_IPV6
 	if (ipvs->bcfg.mcast_af == AF_INET6)
 		result = join_mcast_group6(sock->sk, &mcast_addr.in6.sin6_addr,
-					   ipvs->bcfg.mcast_ifn);
+					   ifindex);
 	else
 #endif
 		result = join_mcast_group(sock->sk, &mcast_addr.in.sin_addr,
-					  ipvs->bcfg.mcast_ifn);
+					  ifindex);
 	if (result < 0) {
 		pr_err("Error joining to the multicast group\n");
 		goto error;
@@ -1867,7 +1846,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 	tinfo = NULL;
 	for (id = 0; id < count; id++) {
 		if (state == IP_VS_STATE_MASTER)
-			sock = make_send_sock(ipvs, id);
+			sock = make_send_sock(ipvs, id, dev);
 		else
 			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
-- 
1.7.7.6

[PATCH v2 3/5] ipvs: Don't check result < 0 after setting result = 0

[Quentin Armitage]

Move the block testing result < 0 to avoid the test immediately
after setting result = 0

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index d52cc25..0ca937a 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1495,14 +1495,14 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id, struct net
 	if (result > 0)
 		set_sock_size(sock->sk, 1, result);
 
-	if (AF_INET == ipvs->mcfg.mcast_af)
+	if (AF_INET == ipvs->mcfg.mcast_af) {
 		result = bind_mcastif_addr(sock, dev);
-	else
+		if (result < 0) {
+			pr_err("Error binding address of the mcast interface\n");
+			goto error;
+		}
+	} else
 		result = 0;
-	if (result < 0) {
-		pr_err("Error binding address of the mcast interface\n");
-		goto error;
-	}
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->mcfg, id);
 	result = sock->ops->connect(sock, (struct sockaddr *) &mcast_addr,
-- 
1.7.7.6

[PATCH v2 4/5] ipvs: Lock socket before setting SK_CAN_REUSE

[Quentin Armitage]

When other settings are changed in the socket it is locked, so
lock the socket before setting SK_CAN_REUSE.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 0ca937a..734335a 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1539,7 +1539,9 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id, int ifi
 	}
 
 	/* it is equivalent to the REUSEADDR option in user-space */
+	lock_sock(sock->sk);
 	sock->sk->sk_reuse = SK_CAN_REUSE;
+	release_sock(sock->sk);
 	result = sysctl_sync_sock_size(ipvs);
 	if (result > 0)
 		set_sock_size(sock->sk, 0, result);
-- 
1.7.7.6

[PATCH v2 5/5] ipvs: log additional sync daemon parameters

[Quentin Armitage]

Add new multicast parameters to log messages when sync daemons start.

Commits <e4ff67513096> ("ipvs: add sync_maxlen parameter for the sync
daemon") and <d33288172e72> ("ipvs: add more mcast parameters for the
sync daemon") added additional multicast parameters, but didn't add
them to the log messages when the sync daemons started.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 734335a..bb59b84 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1669,8 +1669,17 @@ static int sync_thread_master(void *data)
 	struct ip_vs_sync_buff *sb;
 
 	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id);
+			"syncid = %d, id = %d, maxlen = %d\n",
+			ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
+			tinfo->id, ipvs->mcfg.sync_maxlen);
+	if (ipvs->mcfg.mcast_af == AF_INET)
+		pr_info("                   : group = %pI4, port = %d, ttl = %d\n",
+			&ipvs->mcfg.mcast_group.in, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
+	else
+		pr_info("                   : group = %pI6c, port = %d, ttl = %d\n",
+			&ipvs->mcfg.mcast_group.in6, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
 
 	for (;;) {
 		sb = next_sync_buff(ipvs, ms);
@@ -1723,8 +1732,17 @@ static int sync_thread_backup(void *data)
 	int len;
 
 	pr_info("sync thread started: state = BACKUP, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid, tinfo->id);
+			"syncid = %d, id = %d, maxlen = %d\n",
+			ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid,
+			tinfo->id, ipvs->bcfg.sync_maxlen);
+	if (ipvs->bcfg.mcast_af == AF_INET)
+		pr_info("                   : group = %pI4, port = %d, ttl = %d\n",
+			&ipvs->bcfg.mcast_group.in, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
+	else
+		pr_info("                   : group = %pI6c, port = %d, ttl = %d\n",
+			&ipvs->bcfg.mcast_group.in6, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
 
 	while (!kthread_should_stop()) {
 		wait_event_interruptible(*sk_sleep(tinfo->sock->sk),
-- 
1.7.7.6

Re: [PATCH v2 0/5] ipvs: fix backup sync daemon with IPv6, and minor updates

[Julian Anastasov]

	Hello,

On Tue, 14 Jun 2016, Quentin Armitage wrote:

> This series of patches arise from discovering that:
> ipvsadm --start-daemon backup --mcast-group IPv6_address ...
> would always fail.
> 
> The first patch resolves the problem. The second and third patches are
> optimizations that were noticed while investigating the original problem.
> The fourth patch adds a lock which appears to have been omitted, and the
> final patch adds the recently added sync daemon multicast parameters to
> the log messages that are written when the sync daemons start.
> 
> v2 fixes a compile error in a debug message identified by kbuild test robot.
> Now compiles with CONFIG_IP_VS_DEBUG enabled. Patch 2/5 is modified to correct
> the problem, and patch 3/5 is modifed to apply with the modified patch 2/5.
> 
> Quentin Armitage (5):
>   ipvs: Enable setting IPv6 multicast address for ipvs sync daemon
>     backup
>   ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync
>     daemon
>   ipvs: Don't check result < 0 after setting result = 0
>   ipvs: Lock socket before setting SK_CAN_REUSE
>   ipvs: log additional sync daemon parameters
> 
>  net/netfilter/ipvs/ip_vs_sync.c |  104 +++++++++++++++++++-------------------
>  1 files changed, 52 insertions(+), 52 deletions(-)
> 
> -- 
> 1.7.7.6

	Thanks for catching this bug. Following are my
comments for the patches:

Patch 1:

	I missed the fact that link-local addresses (ffx2) require
binding to ifindex due to __ipv6_addr_needs_scope_id check,
I tested only with a ff05 address. BTW, ff01 is a node-local
address (loopback), you should not use it for IPVS.

	Instead of directly writing into sin6_scope_id we can use
'sock->sk->sk_bound_dev_if = ifindex;' before bind(), it will
work for v4 and v6. Let me know if such solution works.

	You have to send this patch as a bugfix, it should
apply to the net tree and later will go to stable trees (4.3+),
i.e. 4.4, 4.5, 4.6 and 4.7, I don't see stable 4.3 in
https://www.kernel.org/. You should mention in commit message
that this patch is a fix to specific commit (check
Documentation/SubmittingPatches):

Fixes: d33288172e72 ("ipvs: add more mcast parameters for the sync daemon")

	The other patches will go to the net-next tree in
separate patchset but I see little fuzz if patch 2 is applied
without patch 1, so may be this patchset should wait the first
patch to appear in net-next kernel.

Patch 2: looks OK

Patch 3: looks OK

	It was done this way to not exceed the 80-char limit.
May be you can reduce the message for the same reason.

Patch 4: looks OK

	Before bind() such operations should be safe without locks.

Patch 5:

	No need of <> for the commit IDs.

	The indentation of existing pr_info in both cases
should not be changed.

	Patches 1, 2, 3 have coding style warnings from checkpatch
that can be fixed, you can check them in this way:

scripts/checkpatch.pl --strict /tmp/file.patch

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH v2 0/5] ipvs: fix backup sync daemon with IPv6, and minor updates

[Julian Anastasov]

	Hello,

On Wed, 15 Jun 2016, Quentin Armitage wrote:

> I am updating the patches in line with your comments, but I'm not sure about
> a couple of points.
> 
> Patch 4:
> 
> You state that before bind(), such changes should be safe. However, from the
> function make_send_sock(), when the functions set_mcast_if(),
> set_mcast_loop(), set_mcast_ttl() and set_mcast_pmtudisc() are called before
> connect(), they all lock the socket before modifying it. Patch 4 was
> intended to make the setting of REUSE consistent.
> 
> If the locking is not necessary, would it be better to remove the locks from
> the set_mcast_...() functions referred to above.

	This is a slow path, so it does not matter much.
There is no concurrent access to the socket, the only
risk is some call into the stack that checks with lockdep
for the missing lock. Such example but for another lock
we already hold is ASSERT_RTNL in ip_mc_join_group. But for simple
sk vars lock is not needed. You can safely remove locks before
connect/bind if only sk fields are accessed directly.
We can keep it only in join_mcast_group*(), especially
because they are called after bind().

> Re patch 1 setting 'sock->sk->sk_bound_dev_if = ifindex;', I presume the
> locking should be consistent with what is done in the other functions.

	It is a simple var, so it can work without lock.

> Your comments on the above would be really helpful.
> 
> Patch 5:
> 
> You state 'The indentation of existing pr_info in both cases should not be
> changed". I'm not clear exactly what that means. Does it mean that the
> spaces at the beginning of the pr_info() strings which report group, port
> and ttl should be removed?

	No, here is example from your patch:

        pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
-               "syncid = %d, id = %d\n",
-               ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id);
+                       "syncid = %d, id = %d, maxlen = %d\n",
+                       ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
+                       tinfo->id, ipvs->mcfg.sync_maxlen);

	"syncid = " was at the same column as "sync thread started",
you added another tab, may be to align with the args in new pr_info.
The result is:

	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
			"syncid = %d, id = %d, maxlen = %d\n",
			ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
			tinfo->id, ipvs->mcfg.sync_maxlen);
	<--- 2 TABs --->

	But it should be:

	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
		"syncid = %d, id = %d, maxlen = %d\n",
		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
		tinfo->id, ipvs->mcfg.sync_maxlen);
	< 1 TAB>

	Also, the new pr_info calls exceed 80 columns.
May be you can reduce the many spaces.

Regards

--
Julian Anastasov <ja@ssi.bg>

[PATCH v3 0/4] ipvs: fix backup sync daemon with IPv6, and minor updates

[Quentin Armitage]

This series of patches arise from discovering that:
ipvsadm --start-daemon backup --mcast-group IPv6_address ...
would always fail.

The first patch resolves the problem. The second and third patches are
optimizations that were noticed while investigating the original problem.
The fourth patch adds a lock which appears to have been omitted, and the
final patch adds the recently added sync daemon multicast parameters to
the log messages that are written when the sync daemons start.

v2 fixes a compile error in a debug message identified by kbuild test
robot. Now compiles with CONFIG_IP_VS_DEBUG enabled. Patch 2/5 is modified
to correct the problem, and patch 3/5 is modifed to apply with the
modified patch 2/5.

v3 incorporates changes suggested by Julian Anastasov.
Patch 1 now sets 'sock->sk->sk_bound_dev_if = ifindex' rather than setting
        sin6_scope_id. Also remove the locks since unnecessary.
Patch 3 shortens the logged message in order not to exceed 80-char limit.
Patch 4 Removed, the locks aren't necessary
Patch 5 No longer changes indentation of existing pr_info. Also removes <>
        around commit IDs in commit description.
Patches 1, 2, 3, 5 are updated to resolve coding style warnings, and all
	pass with 0 errors, warnings and checks.
Patch 5 now becomes patch 4.

The changes have all been tested and work as expected.

Quentin Armitage (4):
  ipvs: Enable setting IPv6 multicast address for ipvs
  ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync
    daemon
  ipvs: Don't check result < 0 after setting result = 0
  ipvs: log additional sync daemon parameters

 net/netfilter/ipvs/ip_vs_sync.c |  105 +++++++++++++++++++--------------------
 1 files changed, 52 insertions(+), 53 deletions(-)

-- 
1.7.7.6

[PATCH v3 1/4] ipvs: Enable setting IPv6 multicast address for ipvs

[Quentin Armitage]

When using HEAD from
https://git.kernel.org/cgit/utils/kernel/ipvsadm/ipvsadm.git/,
the command:
ipvsadm --start-daemon backup --mcast-interface eth0.60 \
    --mcast-group ff02::1:81
fails with the error message:
Argument list too long

whereas both:
ipvsadm --start-daemon master --mcast-interface eth0.60 \
    --mcast-group ff02::1:81
and:
ipvsadm --start-daemon backup --mcast-interface eth0.60 \
    --mcast-group 224.0.0.81
are successful.

The error message "Argument list too long" isn't helpful. The error occurs
because an IPv6 address is given in backup mode.

The error is in make_receive_sock() in net/netfilter/ipvs/ip_vs_sync.c,
since it fails to set the interface on the address or the socket before
calling inet6_bind() (via sock->ops->bind), where the test
'if (!sk->sk_bound_dev_if)' failed.

Setting sock->sk->sk_bound_dev_if on the socket before calling
inet6_bind() resolves the issue.

Fixes: d33288172e72 ("ipvs: add more mcast parameters for the sync daemon")

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..1b07578 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1545,7 +1545,8 @@ error:
 /*
  *      Set up receiving multicast socket over UDP
  */
-static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
+					int ifindex)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1566,6 +1567,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 0, result);
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->bcfg, id);
+	sock->sk->sk_bound_dev_if = ifindex;
 	result = sock->ops->bind(sock, (struct sockaddr *)&mcast_addr, salen);
 	if (result < 0) {
 		pr_err("Error binding to the multicast addr\n");
@@ -1868,7 +1870,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 		if (state == IP_VS_STATE_MASTER)
 			sock = make_send_sock(ipvs, id);
 		else
-			sock = make_receive_sock(ipvs, id);
+			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
 			result = PTR_ERR(sock);
 			goto outtinfo;
-- 
1.7.7.6

[PATCH v3 2/4] ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync daemon

[Quentin Armitage]

Optimise starting sync daemons by using the result of the first call to
__dev_get_by_name() and pass the result or ifindex to subsequent functions
to avoid them having to call __dev_get_by_name() again.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   60 +++++++++++++--------------------------
 1 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 1b07578..fbc5ba4 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1356,28 +1356,22 @@ static void set_mcast_pmtudisc(struct sock *sk, int val)
 /*
  *      Specifiy default interface for outgoing multicasts
  */
-static int set_mcast_if(struct sock *sk, char *ifname)
+static int set_mcast_if(struct sock *sk, int ifindex)
 {
-	struct net_device *dev;
 	struct inet_sock *inet = inet_sk(sk);
-	struct net *net = sock_net(sk);
-
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
 
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	inet->mc_index = dev->ifindex;
+	inet->mc_index = ifindex;
 	/*  inet->mc_addr  = 0; */
 #ifdef CONFIG_IP_VS_IPV6
 	if (sk->sk_family == AF_INET6) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 
 		/* IPV6_MULTICAST_IF */
-		np->mcast_oif = dev->ifindex;
+		np->mcast_oif = ifindex;
 	}
 #endif
 	release_sock(sk);
@@ -1392,23 +1386,18 @@ static int set_mcast_if(struct sock *sk, char *ifname)
  *      in the in_addr structure passed in as a parameter.
  */
 static int
-join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
+join_mcast_group(struct sock *sk, struct in_addr *addr, int ifindex)
 {
-	struct net *net = sock_net(sk);
 	struct ip_mreqn mreq;
-	struct net_device *dev;
 	int ret;
 
 	memset(&mreq, 0, sizeof(mreq));
 	memcpy(&mreq.imr_multiaddr, addr, sizeof(struct in_addr));
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
-	mreq.imr_ifindex = dev->ifindex;
+	mreq.imr_ifindex = ifindex;
 
 	lock_sock(sk);
 	ret = ip_mc_join_group(sk, &mreq);
@@ -1419,44 +1408,33 @@ join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
 
 #ifdef CONFIG_IP_VS_IPV6
 static int join_mcast_group6(struct sock *sk, struct in6_addr *addr,
-			     char *ifname)
+			     int ifindex)
 {
-	struct net *net = sock_net(sk);
-	struct net_device *dev;
 	int ret;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	ret = ipv6_sock_mc_join(sk, dev->ifindex, addr);
+	ret = ipv6_sock_mc_join(sk, ifindex, addr);
 	release_sock(sk);
 
 	return ret;
 }
 #endif
 
-static int bind_mcastif_addr(struct socket *sock, char *ifname)
+static int bind_mcastif_addr(struct socket *sock, struct net_device *dev)
 {
-	struct net *net = sock_net(sock->sk);
-	struct net_device *dev;
 	__be32 addr;
 	struct sockaddr_in sin;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-
 	addr = inet_select_addr(dev, 0, RT_SCOPE_UNIVERSE);
 	if (!addr)
 		pr_err("You probably need to specify IP address on "
 		       "multicast interface.\n");
 
 	IP_VS_DBG(7, "binding socket with (%s) %pI4\n",
-		  ifname, &addr);
+		  dev->name, &addr);
 
 	/* Now bind the socket with the address of multicast interface */
 	sin.sin_family	     = AF_INET;
@@ -1489,7 +1467,8 @@ static void get_mcast_sockaddr(union ipvs_sockaddr *sa, int *salen,
 /*
  *      Set up sending multicast socket over UDP
  */
-static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id,
+				     struct net_device *dev)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1503,7 +1482,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
-	result = set_mcast_if(sock->sk, ipvs->mcfg.mcast_ifn);
+	result = set_mcast_if(sock->sk, dev->ifindex);
 	if (result < 0) {
 		pr_err("Error setting outbound mcast interface\n");
 		goto error;
@@ -1518,7 +1497,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 1, result);
 
 	if (AF_INET == ipvs->mcfg.mcast_af)
-		result = bind_mcastif_addr(sock, ipvs->mcfg.mcast_ifn);
+		result = bind_mcastif_addr(sock, dev);
 	else
 		result = 0;
 	if (result < 0) {
@@ -1560,6 +1539,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
+
 	/* it is equivalent to the REUSEADDR option in user-space */
 	sock->sk->sk_reuse = SK_CAN_REUSE;
 	result = sysctl_sync_sock_size(ipvs);
@@ -1578,11 +1558,11 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
 #ifdef CONFIG_IP_VS_IPV6
 	if (ipvs->bcfg.mcast_af == AF_INET6)
 		result = join_mcast_group6(sock->sk, &mcast_addr.in6.sin6_addr,
-					   ipvs->bcfg.mcast_ifn);
+					   ifindex);
 	else
 #endif
 		result = join_mcast_group(sock->sk, &mcast_addr.in.sin_addr,
-					  ipvs->bcfg.mcast_ifn);
+					  ifindex);
 	if (result < 0) {
 		pr_err("Error joining to the multicast group\n");
 		goto error;
@@ -1868,7 +1848,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 	tinfo = NULL;
 	for (id = 0; id < count; id++) {
 		if (state == IP_VS_STATE_MASTER)
-			sock = make_send_sock(ipvs, id);
+			sock = make_send_sock(ipvs, id, dev);
 		else
 			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
-- 
1.7.7.6

[PATCH v3 3/4] ipvs: Don't check result < 0 after setting result = 0

[Quentin Armitage]

Move the block testing result < 0 to avoid the test immediately
after setting result = 0

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index fbc5ba4..2be99b2 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1496,13 +1496,14 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id,
 	if (result > 0)
 		set_sock_size(sock->sk, 1, result);
 
-	if (AF_INET == ipvs->mcfg.mcast_af)
+	if (ipvs->mcfg.mcast_af == AF_INET) {
 		result = bind_mcastif_addr(sock, dev);
-	else
+		if (result < 0) {
+			pr_err("Error binding address of mcast interface\n");
+			goto error;
+		}
+	} else {
 		result = 0;
-	if (result < 0) {
-		pr_err("Error binding address of the mcast interface\n");
-		goto error;
 	}
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->mcfg, id);
-- 
1.7.7.6

[PATCH v3 4/4] ipvs: log additional sync daemon parameters

[Quentin Armitage]

Add new multicast parameters to log messages when sync daemons start.

Commit e4ff67513096 ("ipvs: add sync_maxlen parameter for the sync
daemon") and commit d33288172e72 ("ipvs: add more mcast parameters for
the sync daemon") added additional multicast parameters, but didn't add
them to the log messages when the sync daemons started.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 2be99b2..a52c4be 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1669,9 +1669,17 @@ static int sync_thread_master(void *data)
 	struct sock *sk = tinfo->sock->sk;
 	struct ip_vs_sync_buff *sb;
 
-	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id);
+	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, syncid = %d, id = %d, maxlen = %d\n",
+		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
+		tinfo->id, ipvs->mcfg.sync_maxlen);
+	if (ipvs->mcfg.mcast_af == AF_INET)
+		pr_info("group = %pI4, port = %d, ttl = %d\n",
+			&ipvs->mcfg.mcast_group.in, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
+	else
+		pr_info("group = %pI6c, port = %d, ttl = %d\n",
+			&ipvs->mcfg.mcast_group.in6, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
 
 	for (;;) {
 		sb = next_sync_buff(ipvs, ms);
@@ -1723,9 +1731,17 @@ static int sync_thread_backup(void *data)
 	struct netns_ipvs *ipvs = tinfo->ipvs;
 	int len;
 
-	pr_info("sync thread started: state = BACKUP, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid, tinfo->id);
+	pr_info("sync thread started: state = BACKUP, mcast_ifn = %s, syncid = %d, id = %d, maxlen = %d\n",
+		ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid,
+		tinfo->id, ipvs->bcfg.sync_maxlen);
+	if (ipvs->bcfg.mcast_af == AF_INET)
+		pr_info("group = %pI4, port = %d, ttl = %d\n",
+			&ipvs->bcfg.mcast_group.in, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
+	else
+		pr_info("group = %pI6c, port = %d, ttl = %d\n",
+			&ipvs->bcfg.mcast_group.in6, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
 
 	while (!kthread_should_stop()) {
 		wait_event_interruptible(*sk_sleep(tinfo->sock->sk),
-- 
1.7.7.6

Re: [PATCH v3 0/4] ipvs: fix backup sync daemon with IPv6, and minor updates

[Julian Anastasov]

	Hello,

On Wed, 15 Jun 2016, Quentin Armitage wrote:

> This series of patches arise from discovering that:
> ipvsadm --start-daemon backup --mcast-group IPv6_address ...
> would always fail.
> 
> The first patch resolves the problem. The second and third patches are
> optimizations that were noticed while investigating the original problem.
> The fourth patch adds a lock which appears to have been omitted, and the
> final patch adds the recently added sync daemon multicast parameters to
> the log messages that are written when the sync daemons start.
> 
> v2 fixes a compile error in a debug message identified by kbuild test
> robot. Now compiles with CONFIG_IP_VS_DEBUG enabled. Patch 2/5 is modified
> to correct the problem, and patch 3/5 is modifed to apply with the
> modified patch 2/5.
> 
> v3 incorporates changes suggested by Julian Anastasov.
> Patch 1 now sets 'sock->sk->sk_bound_dev_if = ifindex' rather than setting
>         sin6_scope_id. Also remove the locks since unnecessary.
> Patch 3 shortens the logged message in order not to exceed 80-char limit.
> Patch 4 Removed, the locks aren't necessary
> Patch 5 No longer changes indentation of existing pr_info. Also removes <>
>         around commit IDs in commit description.
> Patches 1, 2, 3, 5 are updated to resolve coding style warnings, and all
> 	pass with 0 errors, warnings and checks.
> Patch 5 now becomes patch 4.
> 
> The changes have all been tested and work as expected.
> 
> Quentin Armitage (4):
>   ipvs: Enable setting IPv6 multicast address for ipvs
>   ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync
>     daemon
>   ipvs: Don't check result < 0 after setting result = 0
>   ipvs: log additional sync daemon parameters
> 
>  net/netfilter/ipvs/ip_vs_sync.c |  105 +++++++++++++++++++--------------------
>  1 files changed, 52 insertions(+), 53 deletions(-)
> 
> -- 
> 1.7.7.6

	You should post first patch separately, not as a part
from the patchset, by specifying the tree:

[PATCH v4 net] ipvs: ...

	The other 3 patches remain in this patchset,
with added "net-next":

[PATCH v4 net-next */3] ipvs: ...

Patch 1:

	It is good to mention that problem happens for link-local
addresses, not for site/org-local or global scope. By this way
we are more precise when creating a bugfix, it avoids confusion.
You can also check again the Subject and the commit message for
improvements. It is up to you but here is an example:

ipvs: fix bind to link-local mcast IPv6 address in backup

	The empty line between Fixes and Signed-off-by should be
removed.

Patch 2-3: look OK

Patch 4:

	Some of the fields are unsigned, so %d should be %u:
sync_maxlen, mcast_port, mcast_af, mcast_ttl

Regards

--
Julian Anastasov <ja@ssi.bg>

[PATCH v4 net] ipvs: fix bind to link-local mcast IPv6 address in backup

[Quentin Armitage]

When using HEAD from
https://git.kernel.org/cgit/utils/kernel/ipvsadm/ipvsadm.git/,
the command:
ipvsadm --start-daemon backup --mcast-interface eth0.60 \
    --mcast-group ff02::1:81
fails with the error message:
Argument list too long

whereas both:
ipvsadm --start-daemon master --mcast-interface eth0.60 \
    --mcast-group ff02::1:81
and:
ipvsadm --start-daemon backup --mcast-interface eth0.60 \
    --mcast-group 224.0.0.81
are successful.

The error message "Argument list too long" isn't helpful. The error occurs
because an IPv6 address is given in backup mode.

The error is in make_receive_sock() in net/netfilter/ipvs/ip_vs_sync.c,
since it fails to set the interface on the address or the socket before
calling inet6_bind() (via sock->ops->bind), where the test
'if (!sk->sk_bound_dev_if)' failed.

Setting sock->sk->sk_bound_dev_if on the socket before calling
inet6_bind() resolves the issue.

Fixes: d33288172e72 ("ipvs: add more mcast parameters for the sync daemon")
Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..1b07578 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1545,7 +1545,8 @@ error:
 /*
  *      Set up receiving multicast socket over UDP
  */
-static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
+					int ifindex)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1566,6 +1567,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 0, result);
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->bcfg, id);
+	sock->sk->sk_bound_dev_if = ifindex;
 	result = sock->ops->bind(sock, (struct sockaddr *)&mcast_addr, salen);
 	if (result < 0) {
 		pr_err("Error binding to the multicast addr\n");
@@ -1868,7 +1870,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 		if (state == IP_VS_STATE_MASTER)
 			sock = make_send_sock(ipvs, id);
 		else
-			sock = make_receive_sock(ipvs, id);
+			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
 			result = PTR_ERR(sock);
 			goto outtinfo;
-- 
1.7.7.6

Re: [PATCH v4 net] ipvs: fix bind to link-local mcast IPv6 address in backup

[Julian Anastasov]

	Hello,

On Thu, 16 Jun 2016, Quentin Armitage wrote:

> When using HEAD from
> https://git.kernel.org/cgit/utils/kernel/ipvsadm/ipvsadm.git/,
> the command:
> ipvsadm --start-daemon backup --mcast-interface eth0.60 \
>     --mcast-group ff02::1:81
> fails with the error message:
> Argument list too long
> 
> whereas both:
> ipvsadm --start-daemon master --mcast-interface eth0.60 \
>     --mcast-group ff02::1:81
> and:
> ipvsadm --start-daemon backup --mcast-interface eth0.60 \
>     --mcast-group 224.0.0.81
> are successful.
> 
> The error message "Argument list too long" isn't helpful. The error occurs
> because an IPv6 address is given in backup mode.
> 
> The error is in make_receive_sock() in net/netfilter/ipvs/ip_vs_sync.c,
> since it fails to set the interface on the address or the socket before
> calling inet6_bind() (via sock->ops->bind), where the test
> 'if (!sk->sk_bound_dev_if)' failed.
> 
> Setting sock->sk->sk_bound_dev_if on the socket before calling
> inet6_bind() resolves the issue.
> 
> Fixes: d33288172e72 ("ipvs: add more mcast parameters for the sync daemon")
> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>

	Looks good to me, thanks!

Acked-by: Julian Anastasov <ja@ssi.bg>

	Simon, please apply to ipvs tree. Patch compiles
also on stable 4.4.13, 4.5.7 and 4.6.2, so no need for
special versions. The ack is also for the other 3 patches
from v4 (for ipvs-next) but they depend on this patch.

> ---
>  net/netfilter/ipvs/ip_vs_sync.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 803001a..1b07578 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -1545,7 +1545,8 @@ error:
>  /*
>   *      Set up receiving multicast socket over UDP
>   */
> -static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
> +static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
> +					int ifindex)
>  {
>  	/* multicast addr */
>  	union ipvs_sockaddr mcast_addr;
> @@ -1566,6 +1567,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id)
>  		set_sock_size(sock->sk, 0, result);
>  
>  	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->bcfg, id);
> +	sock->sk->sk_bound_dev_if = ifindex;
>  	result = sock->ops->bind(sock, (struct sockaddr *)&mcast_addr, salen);
>  	if (result < 0) {
>  		pr_err("Error binding to the multicast addr\n");
> @@ -1868,7 +1870,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
>  		if (state == IP_VS_STATE_MASTER)
>  			sock = make_send_sock(ipvs, id);
>  		else
> -			sock = make_receive_sock(ipvs, id);
> +			sock = make_receive_sock(ipvs, id, dev->ifindex);
>  		if (IS_ERR(sock)) {
>  			result = PTR_ERR(sock);
>  			goto outtinfo;
> -- 
> 1.7.7.6

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH v4 net] ipvs: fix bind to link-local mcast IPv6 address in backup

[Simon Horman]

On Fri, Jun 17, 2016 at 09:42:49AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 16 Jun 2016, Quentin Armitage wrote:
> 
> > When using HEAD from
> > https://git.kernel.org/cgit/utils/kernel/ipvsadm/ipvsadm.git/,
> > the command:
> > ipvsadm --start-daemon backup --mcast-interface eth0.60 \
> >     --mcast-group ff02::1:81
> > fails with the error message:
> > Argument list too long
> > 
> > whereas both:
> > ipvsadm --start-daemon master --mcast-interface eth0.60 \
> >     --mcast-group ff02::1:81
> > and:
> > ipvsadm --start-daemon backup --mcast-interface eth0.60 \
> >     --mcast-group 224.0.0.81
> > are successful.
> > 
> > The error message "Argument list too long" isn't helpful. The error occurs
> > because an IPv6 address is given in backup mode.
> > 
> > The error is in make_receive_sock() in net/netfilter/ipvs/ip_vs_sync.c,
> > since it fails to set the interface on the address or the socket before
> > calling inet6_bind() (via sock->ops->bind), where the test
> > 'if (!sk->sk_bound_dev_if)' failed.
> > 
> > Setting sock->sk->sk_bound_dev_if on the socket before calling
> > inet6_bind() resolves the issue.
> > 
> > Fixes: d33288172e72 ("ipvs: add more mcast parameters for the sync daemon")
> > Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> 
> 	Looks good to me, thanks!
> 
> Acked-by: Julian Anastasov <ja@ssi.bg>
> 
> 	Simon, please apply to ipvs tree. Patch compiles
> also on stable 4.4.13, 4.5.7 and 4.6.2, so no need for
> special versions. The ack is also for the other 3 patches
> from v4 (for ipvs-next) but they depend on this patch.

Thanks, done.

[PATCH net-next v4 1/3] ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync daemon

[Quentin Armitage]

Optimise starting sync daemons by using the result of the first call to
__dev_get_by_name() and pass the result or ifindex to subsequent functions
to avoid them having to call __dev_get_by_name() again.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   60 +++++++++++++--------------------------
 1 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 1b07578..fbc5ba4 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1356,28 +1356,22 @@ static void set_mcast_pmtudisc(struct sock *sk, int val)
 /*
  *      Specifiy default interface for outgoing multicasts
  */
-static int set_mcast_if(struct sock *sk, char *ifname)
+static int set_mcast_if(struct sock *sk, int ifindex)
 {
-	struct net_device *dev;
 	struct inet_sock *inet = inet_sk(sk);
-	struct net *net = sock_net(sk);
-
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
 
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	inet->mc_index = dev->ifindex;
+	inet->mc_index = ifindex;
 	/*  inet->mc_addr  = 0; */
 #ifdef CONFIG_IP_VS_IPV6
 	if (sk->sk_family == AF_INET6) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 
 		/* IPV6_MULTICAST_IF */
-		np->mcast_oif = dev->ifindex;
+		np->mcast_oif = ifindex;
 	}
 #endif
 	release_sock(sk);
@@ -1392,23 +1386,18 @@ static int set_mcast_if(struct sock *sk, char *ifname)
  *      in the in_addr structure passed in as a parameter.
  */
 static int
-join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
+join_mcast_group(struct sock *sk, struct in_addr *addr, int ifindex)
 {
-	struct net *net = sock_net(sk);
 	struct ip_mreqn mreq;
-	struct net_device *dev;
 	int ret;
 
 	memset(&mreq, 0, sizeof(mreq));
 	memcpy(&mreq.imr_multiaddr, addr, sizeof(struct in_addr));
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
-	mreq.imr_ifindex = dev->ifindex;
+	mreq.imr_ifindex = ifindex;
 
 	lock_sock(sk);
 	ret = ip_mc_join_group(sk, &mreq);
@@ -1419,44 +1408,33 @@ join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)
 
 #ifdef CONFIG_IP_VS_IPV6
 static int join_mcast_group6(struct sock *sk, struct in6_addr *addr,
-			     char *ifname)
+			     int ifindex)
 {
-	struct net *net = sock_net(sk);
-	struct net_device *dev;
 	int ret;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-	if (sk->sk_bound_dev_if && dev->ifindex != sk->sk_bound_dev_if)
+	if (sk->sk_bound_dev_if && ifindex != sk->sk_bound_dev_if)
 		return -EINVAL;
 
 	lock_sock(sk);
-	ret = ipv6_sock_mc_join(sk, dev->ifindex, addr);
+	ret = ipv6_sock_mc_join(sk, ifindex, addr);
 	release_sock(sk);
 
 	return ret;
 }
 #endif
 
-static int bind_mcastif_addr(struct socket *sock, char *ifname)
+static int bind_mcastif_addr(struct socket *sock, struct net_device *dev)
 {
-	struct net *net = sock_net(sock->sk);
-	struct net_device *dev;
 	__be32 addr;
 	struct sockaddr_in sin;
 
-	dev = __dev_get_by_name(net, ifname);
-	if (!dev)
-		return -ENODEV;
-
 	addr = inet_select_addr(dev, 0, RT_SCOPE_UNIVERSE);
 	if (!addr)
 		pr_err("You probably need to specify IP address on "
 		       "multicast interface.\n");
 
 	IP_VS_DBG(7, "binding socket with (%s) %pI4\n",
-		  ifname, &addr);
+		  dev->name, &addr);
 
 	/* Now bind the socket with the address of multicast interface */
 	sin.sin_family	     = AF_INET;
@@ -1489,7 +1467,8 @@ static void get_mcast_sockaddr(union ipvs_sockaddr *sa, int *salen,
 /*
  *      Set up sending multicast socket over UDP
  */
-static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
+static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id,
+				     struct net_device *dev)
 {
 	/* multicast addr */
 	union ipvs_sockaddr mcast_addr;
@@ -1503,7 +1482,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
-	result = set_mcast_if(sock->sk, ipvs->mcfg.mcast_ifn);
+	result = set_mcast_if(sock->sk, dev->ifindex);
 	if (result < 0) {
 		pr_err("Error setting outbound mcast interface\n");
 		goto error;
@@ -1518,7 +1497,7 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id)
 		set_sock_size(sock->sk, 1, result);
 
 	if (AF_INET == ipvs->mcfg.mcast_af)
-		result = bind_mcastif_addr(sock, ipvs->mcfg.mcast_ifn);
+		result = bind_mcastif_addr(sock, dev);
 	else
 		result = 0;
 	if (result < 0) {
@@ -1560,6 +1539,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
 		pr_err("Error during creation of socket; terminating\n");
 		return ERR_PTR(result);
 	}
+
 	/* it is equivalent to the REUSEADDR option in user-space */
 	sock->sk->sk_reuse = SK_CAN_REUSE;
 	result = sysctl_sync_sock_size(ipvs);
@@ -1578,11 +1558,11 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id,
 #ifdef CONFIG_IP_VS_IPV6
 	if (ipvs->bcfg.mcast_af == AF_INET6)
 		result = join_mcast_group6(sock->sk, &mcast_addr.in6.sin6_addr,
-					   ipvs->bcfg.mcast_ifn);
+					   ifindex);
 	else
 #endif
 		result = join_mcast_group(sock->sk, &mcast_addr.in.sin_addr,
-					  ipvs->bcfg.mcast_ifn);
+					  ifindex);
 	if (result < 0) {
 		pr_err("Error joining to the multicast group\n");
 		goto error;
@@ -1868,7 +1848,7 @@ int start_sync_thread(struct netns_ipvs *ipvs, struct ipvs_sync_daemon_cfg *c,
 	tinfo = NULL;
 	for (id = 0; id < count; id++) {
 		if (state == IP_VS_STATE_MASTER)
-			sock = make_send_sock(ipvs, id);
+			sock = make_send_sock(ipvs, id, dev);
 		else
 			sock = make_receive_sock(ipvs, id, dev->ifindex);
 		if (IS_ERR(sock)) {
-- 
1.7.7.6

[PATCH net-next v4 2/3] ipvs: Don't check result < 0 after setting result = 0

[Quentin Armitage]

Move the block testing result < 0 to avoid the test immediately
after setting result = 0

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index fbc5ba4..2be99b2 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1496,13 +1496,14 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id,
 	if (result > 0)
 		set_sock_size(sock->sk, 1, result);
 
-	if (AF_INET == ipvs->mcfg.mcast_af)
+	if (ipvs->mcfg.mcast_af == AF_INET) {
 		result = bind_mcastif_addr(sock, dev);
-	else
+		if (result < 0) {
+			pr_err("Error binding address of mcast interface\n");
+			goto error;
+		}
+	} else {
 		result = 0;
-	if (result < 0) {
-		pr_err("Error binding address of the mcast interface\n");
-		goto error;
 	}
 
 	get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->mcfg, id);
-- 
1.7.7.6

[PATCH net-next v4 3/3] ipvs: log additional sync daemon parameters

[Quentin Armitage]

Add new multicast parameters to log messages when sync daemons start.

Commit e4ff67513096 ("ipvs: add sync_maxlen parameter for the sync
daemon") and commit d33288172e72 ("ipvs: add more mcast parameters for
the sync daemon") added additional multicast parameters, but didn't add
them to the log messages when the sync daemons started.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 net/netfilter/ipvs/ip_vs_sync.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 2be99b2..3d0fd99 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1669,9 +1669,17 @@ static int sync_thread_master(void *data)
 	struct sock *sk = tinfo->sock->sk;
 	struct ip_vs_sync_buff *sb;
 
-	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id);
+	pr_info("sync thread started: state = MASTER, mcast_ifn = %s, syncid = %d, id = %d, maxlen = %u\n",
+		ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid,
+		tinfo->id, ipvs->mcfg.sync_maxlen);
+	if (ipvs->mcfg.mcast_af == AF_INET)
+		pr_info("group = %pI4, port = %u, ttl = %u\n",
+			&ipvs->mcfg.mcast_group.in, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
+	else
+		pr_info("group = %pI6c, port = %u, ttl = %u\n",
+			&ipvs->mcfg.mcast_group.in6, ipvs->mcfg.mcast_port,
+			ipvs->mcfg.mcast_ttl);
 
 	for (;;) {
 		sb = next_sync_buff(ipvs, ms);
@@ -1723,9 +1731,17 @@ static int sync_thread_backup(void *data)
 	struct netns_ipvs *ipvs = tinfo->ipvs;
 	int len;
 
-	pr_info("sync thread started: state = BACKUP, mcast_ifn = %s, "
-		"syncid = %d, id = %d\n",
-		ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid, tinfo->id);
+	pr_info("sync thread started: state = BACKUP, mcast_ifn = %s, syncid = %d, id = %d, maxlen = %u\n",
+		ipvs->bcfg.mcast_ifn, ipvs->bcfg.syncid,
+		tinfo->id, ipvs->bcfg.sync_maxlen);
+	if (ipvs->bcfg.mcast_af == AF_INET)
+		pr_info("group = %pI4, port = %u, ttl = %u\n",
+			&ipvs->bcfg.mcast_group.in, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
+	else
+		pr_info("group = %pI6c, port = %u, ttl = %u\n",
+			&ipvs->bcfg.mcast_group.in6, ipvs->bcfg.mcast_port,
+			ipvs->bcfg.mcast_ttl);
 
 	while (!kthread_should_stop()) {
 		wait_event_interruptible(*sk_sleep(tinfo->sock->sk),
-- 
1.7.7.6