Message ID | 1303226705-29178-1-git-send-email-hans@schillstrom.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, Apr 19, 2011 at 05:25:03PM +0200, Hans Schillstrom wrote: > This is the first patch in a series of three. > The cleanup doesn't work when not exit in a clean way by using ipvsadm. > Killing of a namespace causes a hanging ipvs, this series will cure that. > > If the sync daemons run in a namespace while it crashes > or get killed, there is no way to stop them except for a reboot. > > Kernel threads should not increment the use count of a socket. > By calling sk_change_net() after creating a socket this is avoided. > sock_release cant be used, instead sk_release_kernel() should be used. > > Thanks to Eric W Biederman. > > This patch is based on net-next-2.6 ver 2.6.39-rc2 Thanks Hans and Eric. Is it only this 1st patch that is intended for 2.6.39? The entire series feels a bit long to be applied this late in the rc series. In any case, I'll hold off for comment from Eric and Julian before pushing any of these patches anywhere. > Signed-off-by: Hans Schillstrom <hans@schillstrom.com> > --- > net/netfilter/ipvs/ip_vs_sync.c | 28 +++++++++++++++++++--------- > 1 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c > index 3e7961e..3f87555 100644 > --- a/net/netfilter/ipvs/ip_vs_sync.c > +++ b/net/netfilter/ipvs/ip_vs_sync.c > @@ -1309,7 +1309,12 @@ static struct socket *make_send_sock(struct net *net) > pr_err("Error during creation of socket; terminating\n"); > return ERR_PTR(result); > } > - > + /* > + * Kernel sockets that are a part of a namespace, should not > + * hold a reference to a namespace in order to allow to stop it. > + * After sk_change_net should be released using sk_release_kernel. > + */ > + sk_change_net(sock->sk, net); > result = set_mcast_if(sock->sk, ipvs->master_mcast_ifn); > if (result < 0) { > pr_err("Error setting outbound mcast interface\n"); > @@ -1334,8 +1339,8 @@ static struct socket *make_send_sock(struct net *net) > > return sock; > > - error: > - sock_release(sock); > +error: > + sk_release_kernel(sock->sk); > return ERR_PTR(result); > } > > @@ -1355,7 +1360,12 @@ static struct socket *make_receive_sock(struct net *net) > pr_err("Error during creation of socket; terminating\n"); > return ERR_PTR(result); > } > - > + /* > + * Kernel sockets that are a part of a namespace, should not > + * hold a reference to a namespace in order to allow to stop it. > + * After sk_change_net should be released using sk_release_kernel. > + */ > + sk_change_net(sock->sk, net); > /* it is equivalent to the REUSEADDR option in user-space */ > sock->sk->sk_reuse = 1; > > @@ -1377,8 +1387,8 @@ static struct socket *make_receive_sock(struct net *net) > > return sock; > > - error: > - sock_release(sock); > +error: > + sk_release_kernel(sock->sk); > return ERR_PTR(result); > } > > @@ -1473,7 +1483,7 @@ static int sync_thread_master(void *data) > ip_vs_sync_buff_release(sb); > > /* release the sending multicast socket */ > - sock_release(tinfo->sock); > + sk_release_kernel(tinfo->sock->sk); > kfree(tinfo); > > return 0; > @@ -1513,7 +1523,7 @@ static int sync_thread_backup(void *data) > } > > /* release the sending multicast socket */ > - sock_release(tinfo->sock); > + sk_release_kernel(tinfo->sock->sk); > kfree(tinfo->buf); > kfree(tinfo); > > @@ -1601,7 +1611,7 @@ outtinfo: > outbuf: > kfree(buf); > outsocket: > - sock_release(sock); > + sk_release_kernel(sock->sk); > out: > return result; > } > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe lvs-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Schillstrom <hans@schillstrom.com> writes: > This is the first patch in a series of three. > The cleanup doesn't work when not exit in a clean way by using ipvsadm. > Killing of a namespace causes a hanging ipvs, this series will cure that. > > If the sync daemons run in a namespace while it crashes > or get killed, there is no way to stop them except for a reboot. > > Kernel threads should not increment the use count of a socket. > By calling sk_change_net() after creating a socket this is avoided. > sock_release cant be used, instead sk_release_kernel() should be used. > > Thanks to Eric W Biederman. > > This patch is based on net-next-2.6 ver 2.6.39-rc2 I am not comfortable with this patch on it's own. What kills the daemons on network namespace exit? I would be a little happier if the code also used sock_create_kern instead of __sock_create. Now that it uses sk_change_net. Just to make the code more idiomatic. But not actually guaranteeing that the namespace exit kills the threads at this point seems pretty badly broken, as this idiom is not safe unless we have a hard guarantee that the threads are dead. Eric > Signed-off-by: Hans Schillstrom <hans@schillstrom.com> > --- > net/netfilter/ipvs/ip_vs_sync.c | 28 +++++++++++++++++++--------- > 1 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c > index 3e7961e..3f87555 100644 > --- a/net/netfilter/ipvs/ip_vs_sync.c > +++ b/net/netfilter/ipvs/ip_vs_sync.c > @@ -1309,7 +1309,12 @@ static struct socket *make_send_sock(struct net *net) > pr_err("Error during creation of socket; terminating\n"); > return ERR_PTR(result); > } > - > + /* > + * Kernel sockets that are a part of a namespace, should not > + * hold a reference to a namespace in order to allow to stop it. > + * After sk_change_net should be released using sk_release_kernel. > + */ > + sk_change_net(sock->sk, net); > result = set_mcast_if(sock->sk, ipvs->master_mcast_ifn); > if (result < 0) { > pr_err("Error setting outbound mcast interface\n"); > @@ -1334,8 +1339,8 @@ static struct socket *make_send_sock(struct net *net) > > return sock; > > - error: > - sock_release(sock); > +error: > + sk_release_kernel(sock->sk); > return ERR_PTR(result); > } > > @@ -1355,7 +1360,12 @@ static struct socket *make_receive_sock(struct net *net) > pr_err("Error during creation of socket; terminating\n"); > return ERR_PTR(result); > } > - > + /* > + * Kernel sockets that are a part of a namespace, should not > + * hold a reference to a namespace in order to allow to stop it. > + * After sk_change_net should be released using sk_release_kernel. > + */ > + sk_change_net(sock->sk, net); > /* it is equivalent to the REUSEADDR option in user-space */ > sock->sk->sk_reuse = 1; > > @@ -1377,8 +1387,8 @@ static struct socket *make_receive_sock(struct net *net) > > return sock; > > - error: > - sock_release(sock); > +error: > + sk_release_kernel(sock->sk); > return ERR_PTR(result); > } > > @@ -1473,7 +1483,7 @@ static int sync_thread_master(void *data) > ip_vs_sync_buff_release(sb); > > /* release the sending multicast socket */ > - sock_release(tinfo->sock); > + sk_release_kernel(tinfo->sock->sk); > kfree(tinfo); > > return 0; > @@ -1513,7 +1523,7 @@ static int sync_thread_backup(void *data) > } > > /* release the sending multicast socket */ > - sock_release(tinfo->sock); > + sk_release_kernel(tinfo->sock->sk); > kfree(tinfo->buf); > kfree(tinfo); > > @@ -1601,7 +1611,7 @@ outtinfo: > outbuf: > kfree(buf); > outsocket: > - sock_release(sock); > + sk_release_kernel(sock->sk); > out: > return result; > } > -- > 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Simon Horman <horms@verge.net.au> writes: > On Tue, Apr 19, 2011 at 05:25:03PM +0200, Hans Schillstrom wrote: >> This is the first patch in a series of three. >> The cleanup doesn't work when not exit in a clean way by using ipvsadm. >> Killing of a namespace causes a hanging ipvs, this series will cure that. >> >> If the sync daemons run in a namespace while it crashes >> or get killed, there is no way to stop them except for a reboot. >> >> Kernel threads should not increment the use count of a socket. >> By calling sk_change_net() after creating a socket this is avoided. >> sock_release cant be used, instead sk_release_kernel() should be used. >> >> Thanks to Eric W Biederman. >> >> This patch is based on net-next-2.6 ver 2.6.39-rc2 > > Thanks Hans and Eric. > > Is it only this 1st patch that is intended for 2.6.39? > The entire series feels a bit long to be applied > this late in the rc series. Hans was tracking two bugs that I gave him some advice on. The first was the threads keeping namespaces from exiting at the proper time, which the first patch appears to almost fix. I didn't see the code in that to ensure the threads were gone. The second was a something that sounded like packets flying around during network namespace subsys cleanup. Simply not having network devices and sockets open in the namespace should be enough to guarantee you don't have packets flying around. My feeling is that the patches are in the right neighbourhood but not quite there yet. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c index 3e7961e..3f87555 100644 --- a/net/netfilter/ipvs/ip_vs_sync.c +++ b/net/netfilter/ipvs/ip_vs_sync.c @@ -1309,7 +1309,12 @@ static struct socket *make_send_sock(struct net *net) pr_err("Error during creation of socket; terminating\n"); return ERR_PTR(result); } - + /* + * Kernel sockets that are a part of a namespace, should not + * hold a reference to a namespace in order to allow to stop it. + * After sk_change_net should be released using sk_release_kernel. + */ + sk_change_net(sock->sk, net); result = set_mcast_if(sock->sk, ipvs->master_mcast_ifn); if (result < 0) { pr_err("Error setting outbound mcast interface\n"); @@ -1334,8 +1339,8 @@ static struct socket *make_send_sock(struct net *net) return sock; - error: - sock_release(sock); +error: + sk_release_kernel(sock->sk); return ERR_PTR(result); } @@ -1355,7 +1360,12 @@ static struct socket *make_receive_sock(struct net *net) pr_err("Error during creation of socket; terminating\n"); return ERR_PTR(result); } - + /* + * Kernel sockets that are a part of a namespace, should not + * hold a reference to a namespace in order to allow to stop it. + * After sk_change_net should be released using sk_release_kernel. + */ + sk_change_net(sock->sk, net); /* it is equivalent to the REUSEADDR option in user-space */ sock->sk->sk_reuse = 1; @@ -1377,8 +1387,8 @@ static struct socket *make_receive_sock(struct net *net) return sock; - error: - sock_release(sock); +error: + sk_release_kernel(sock->sk); return ERR_PTR(result); } @@ -1473,7 +1483,7 @@ static int sync_thread_master(void *data) ip_vs_sync_buff_release(sb); /* release the sending multicast socket */ - sock_release(tinfo->sock); + sk_release_kernel(tinfo->sock->sk); kfree(tinfo); return 0; @@ -1513,7 +1523,7 @@ static int sync_thread_backup(void *data) } /* release the sending multicast socket */ - sock_release(tinfo->sock); + sk_release_kernel(tinfo->sock->sk); kfree(tinfo->buf); kfree(tinfo); @@ -1601,7 +1611,7 @@ outtinfo: outbuf: kfree(buf); outsocket: - sock_release(sock); + sk_release_kernel(sock->sk); out: return result; }
This is the first patch in a series of three. The cleanup doesn't work when not exit in a clean way by using ipvsadm. Killing of a namespace causes a hanging ipvs, this series will cure that. If the sync daemons run in a namespace while it crashes or get killed, there is no way to stop them except for a reboot. Kernel threads should not increment the use count of a socket. By calling sk_change_net() after creating a socket this is avoided. sock_release cant be used, instead sk_release_kernel() should be used. Thanks to Eric W Biederman. This patch is based on net-next-2.6 ver 2.6.39-rc2 Signed-off-by: Hans Schillstrom <hans@schillstrom.com> --- net/netfilter/ipvs/ip_vs_sync.c | 28 +++++++++++++++++++--------- 1 files changed, 19 insertions(+), 9 deletions(-) -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html