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
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 59 ++++++++++++--------------------------
1 files changed, 19 insertions(+), 40 deletions(-)
@@ -1419,44 +1408,33 @@ join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)#ifdef CONFIG_IP_VS_IPV6staticintjoin_mcast_group6(structsock*sk,structin6_addr*addr,-char*ifname)+intifindex){-structnet*net=sock_net(sk);-structnet_device*dev;intret;-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);returnret;}#endif-staticintbind_mcastif_addr(structsocket*sock,char*ifname)+staticintbind_mcastif_addr(structsocket*sock,structnet_device*dev){-structnet*net=sock_net(sock->sk);-structnet_device*dev;__be32addr;structsockaddr_insin;-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;
@@ -1559,6 +1537,7 @@ static struct socket *make_receive_sock(struct netns_ipvs *ipvs, int id, int ifipr_err("Error during creation of socket; terminating\n");returnERR_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_IPV6if(ipvs->bcfg.mcast_af==AF_INET6)result=join_mcast_group6(sock->sk,&mcast_addr.in6.sin6_addr,-ipvs->bcfg.mcast_ifn);+ifindex);else#endifresult=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");gotoerror;
Move the block testing result < 0 to avoid the test immediately
after setting result = 0
Signed-off-by: Quentin Armitage <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
@@ -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);
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
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 [off-list ref]
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
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 60 +++++++++++++--------------------------
1 files changed, 20 insertions(+), 40 deletions(-)
@@ -1419,44 +1408,33 @@ join_mcast_group(struct sock *sk, struct in_addr *addr, char *ifname)#ifdef CONFIG_IP_VS_IPV6staticintjoin_mcast_group6(structsock*sk,structin6_addr*addr,-char*ifname)+intifindex){-structnet*net=sock_net(sk);-structnet_device*dev;intret;-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);returnret;}#endif-staticintbind_mcastif_addr(structsocket*sock,char*ifname)+staticintbind_mcastif_addr(structsocket*sock,structnet_device*dev){-structnet*net=sock_net(sock->sk);-structnet_device*dev;__be32addr;structsockaddr_insin;-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;
@@ -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");returnERR_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_IPV6if(ipvs->bcfg.mcast_af==AF_INET6)result=join_mcast_group6(sock->sk,&mcast_addr.in6.sin6_addr,-ipvs->bcfg.mcast_ifn);+ifindex);else#endifresult=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");gotoerror;
Move the block testing result < 0 to avoid the test immediately
after setting result = 0
Signed-off-by: Quentin Armitage <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
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 <redacted>
---
net/netfilter/ipvs/ip_vs_sync.c | 28 ++++++++++++++++++++++------
1 files changed, 22 insertions(+), 6 deletions(-)
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 [off-list ref]