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

[PATCH 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.

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 |  107 ++++++++++++++++++++-------------------
 1 files changed, 54 insertions(+), 53 deletions(-)

-- 
1.7.7.6

[PATCH 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 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 |   66 +++++++++++++-------------------------
 1 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 85b48f1..3181fd0 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,45 +1408,31 @@ 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);
-
 	/* Now bind the socket with the address of multicast interface */
 	sin.sin_family	     = AF_INET;
 	sin.sin_addr.s_addr  = addr;
@@ -1489,7 +1464,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 +1478,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;
@@ -1517,8 +1492,12 @@ 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)
-		result = bind_mcastif_addr(sock, ipvs->mcfg.mcast_ifn);
+	if (AF_INET == ipvs->mcfg.mcast_af) {
+		IP_VS_DBG(7, "binding socket with (%s) %pI4\n",
+			  ipvs->mcast_ifn, &addr);
+
+		result = bind_mcastif_addr(sock, dev);
+	}
 	else
 		result = 0;
 	if (result < 0) {
@@ -1559,6 +1538,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 +1557,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 +1847,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 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 |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 3181fd0..29d73d8 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1497,13 +1497,13 @@ static struct socket *make_send_sock(struct netns_ipvs *ipvs, int id, struct net
 			  ipvs->mcast_ifn, &addr);
 
 		result = bind_mcastif_addr(sock, dev);
+		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 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 29d73d8..dfac9ef 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1540,7 +1540,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 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 dfac9ef..e5f6739 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1670,8 +1670,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);
@@ -1724,8 +1733,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 2/5] ipvs: Stop calling __dev_get_by_name() repeatedly when starting sync daemon

[kbuild test robot]

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

Hi,

[auto build test ERROR on ipvs-next/master]
[also build test ERROR on v4.7-rc3 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Quentin-Armitage/ipvs-Enable-setting-IPv6-multicast-address-for-ipvs-sync-daemon-backup/20160614-095939
base:   https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs-next.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

Note: the linux-review/Quentin-Armitage/ipvs-Enable-setting-IPv6-multicast-address-for-ipvs-sync-daemon-backup/20160614-095939 HEAD db86ecb72d6516577334b1b855efa8bf1988321e builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   In file included from net/netfilter/ipvs/ip_vs_sync.c:57:0:
   net/netfilter/ipvs/ip_vs_sync.c: In function 'make_send_sock':
>> net/netfilter/ipvs/ip_vs_sync.c:1497:10: error: 'struct netns_ipvs' has no member named 'mcast_ifn'
         ipvs->mcast_ifn, &addr);
             ^
   include/net/ip_vs.h:243:37: note: in definition of macro 'IP_VS_DBG'
       printk(KERN_DEBUG pr_fmt(msg), ##__VA_ARGS__); \
                                        ^
>> net/netfilter/ipvs/ip_vs_sync.c:1497:24: error: 'addr' undeclared (first use in this function)
         ipvs->mcast_ifn, &addr);
                           ^
   include/net/ip_vs.h:243:37: note: in definition of macro 'IP_VS_DBG'
       printk(KERN_DEBUG pr_fmt(msg), ##__VA_ARGS__); \
                                        ^
   net/netfilter/ipvs/ip_vs_sync.c:1497:24: note: each undeclared identifier is reported only once for each function it appears in
         ipvs->mcast_ifn, &addr);
                           ^
   include/net/ip_vs.h:243:37: note: in definition of macro 'IP_VS_DBG'
       printk(KERN_DEBUG pr_fmt(msg), ##__VA_ARGS__); \
                                        ^

vim +1497 net/netfilter/ipvs/ip_vs_sync.c

  1491		result = sysctl_sync_sock_size(ipvs);
  1492		if (result > 0)
  1493			set_sock_size(sock->sk, 1, result);
  1494	
  1495		if (AF_INET == ipvs->mcfg.mcast_af) {
  1496			IP_VS_DBG(7, "binding socket with (%s) %pI4\n",
> 1497				  ipvs->mcast_ifn, &addr);
  1498	
  1499			result = bind_mcastif_addr(sock, dev);
  1500		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46364 bytes --]

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

[Eric Dumazet]

On Tue, 2016-06-14 at 02:43 +0100, Quentin Armitage wrote:
> 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 29d73d8..dfac9ef 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -1540,7 +1540,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);


Have you tested this patch ?

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

[Eric Dumazet]

On Mon, 2016-06-13 at 20:12 -0700, Eric Dumazet wrote:

> 
> Have you tested this patch ?

Hmm, please ignore this question ;)