Message ID | 20121025092105.5119b8bf@nehalam.linuxnetplumber.net |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Stephen Hemminger <shemminger@vyatta.com> writes: > I noticed that the L2TP sockets are not being moved to the correct name > space. > > Something like this is probably needed. This is almost right. There needs to be a line in l2tp_tunnel_create that verifies the network namespace of the socket derived from a file descriptor and the passed in network namespace match. For the l2tp_tunnel_sock_create case where we have a socket that is not exported to userspace using sk_change_net seems appropriate to avoid reference counting problems. And it may be worth moving that work into sk_create_kern. But we need a network namespace hook that will lookup all l2tp tunnel sockets when a network namespace is being destroyed and remove them. I think we can hit this bug with rmmod as well. Bleh. Eric > --- a/net/l2tp/l2tp_core.c 2012-10-25 09:11:15.691271882 -0700 > +++ b/net/l2tp/l2tp_core.c 2012-10-25 09:18:58.746621418 -0700 > @@ -1357,7 +1357,10 @@ static void l2tp_tunnel_free(struct l2tp > * userspace. This is used for static tunnels where there is no > * managing L2TP daemon. > */ > -static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct socket **sockp) > +static int l2tp_tunnel_sock_create(struct net *net, > + u32 tunnel_id, u32 peer_tunnel_id, > + struct l2tp_tunnel_cfg *cfg, > + struct socket **sockp) > { > int err = -EINVAL; > struct sockaddr_in udp_addr; > @@ -1372,11 +1375,12 @@ static int l2tp_tunnel_sock_create(u32 t > case L2TP_ENCAPTYPE_UDP: > #if IS_ENABLED(CONFIG_IPV6) > if (cfg->local_ip6 && cfg->peer_ip6) { > - err = sock_create(AF_INET6, SOCK_DGRAM, 0, sockp); > + err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, sockp); > if (err < 0) > goto out; > > sock = *sockp; > + sk_change(sock->sk, net); > > memset(&udp6_addr, 0, sizeof(udp6_addr)); > udp6_addr.sin6_family = AF_INET6; > @@ -1400,11 +1404,12 @@ static int l2tp_tunnel_sock_create(u32 t > } else > #endif > { > - err = sock_create(AF_INET, SOCK_DGRAM, 0, sockp); > + err = sock_create_kern(AF_INET, SOCK_DGRAM, 0, sockp); > if (err < 0) > goto out; > > sock = *sockp; > + sk_change(sock->sk, net); > > memset(&udp_addr, 0, sizeof(udp_addr)); > udp_addr.sin_family = AF_INET; > @@ -1433,7 +1438,7 @@ static int l2tp_tunnel_sock_create(u32 t > case L2TP_ENCAPTYPE_IP: > #if IS_ENABLED(CONFIG_IPV6) > if (cfg->local_ip6 && cfg->peer_ip6) { > - err = sock_create(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP, > + err = sock_create_kern(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP, > sockp); > if (err < 0) > goto out; > @@ -1462,12 +1467,13 @@ static int l2tp_tunnel_sock_create(u32 t > } else > #endif > { > - err = sock_create(AF_INET, SOCK_DGRAM, IPPROTO_L2TP, > + err = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_L2TP, > sockp); > if (err < 0) > goto out; > > sock = *sockp; > + sk_change(sock->sk, net); > > memset(&ip_addr, 0, sizeof(ip_addr)); > ip_addr.l2tp_family = AF_INET; > @@ -1517,7 +1523,8 @@ int l2tp_tunnel_create(struct net *net, > * kernel socket. > */ > if (fd < 0) { > - err = l2tp_tunnel_sock_create(tunnel_id, peer_tunnel_id, cfg, &sock); > + err = l2tp_tunnel_sock_create(net, tunnel_id, peer_tunnel_id, > + cfg, &sock); > if (err < 0) > goto err; > } else { > -- > 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 -- 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
On Sat, 27 Oct 2012 22:43:13 -0700 ebiederm@xmission.com (Eric W. Biederman) wrote: > Stephen Hemminger <shemminger@vyatta.com> writes: > > > I noticed that the L2TP sockets are not being moved to the correct name > > space. > > > > Something like this is probably needed. > > This is almost right. > > There needs to be a line in l2tp_tunnel_create that verifies > the network namespace of the socket derived from a file descriptor > and the passed in network namespace match. > > For the l2tp_tunnel_sock_create case where we have a socket that is not > exported to userspace using sk_change_net seems appropriate to avoid > reference counting problems. And it may be worth moving that work into > sk_create_kern. But we need a network namespace hook that will lookup > all l2tp tunnel sockets when a network namespace is being destroyed and > remove them. I think we can hit this bug with rmmod as well. Since I don't use netns or L2TP for real, someone else needs to take up the crusade here. -- 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
Stephen Hemminger <shemminger@vyatta.com> writes: > On Sat, 27 Oct 2012 22:43:13 -0700 > ebiederm@xmission.com (Eric W. Biederman) wrote: > >> Stephen Hemminger <shemminger@vyatta.com> writes: >> >> > I noticed that the L2TP sockets are not being moved to the correct name >> > space. >> > >> > Something like this is probably needed. >> >> This is almost right. >> >> There needs to be a line in l2tp_tunnel_create that verifies >> the network namespace of the socket derived from a file descriptor >> and the passed in network namespace match. >> >> For the l2tp_tunnel_sock_create case where we have a socket that is not >> exported to userspace using sk_change_net seems appropriate to avoid >> reference counting problems. And it may be worth moving that work into >> sk_create_kern. But we need a network namespace hook that will lookup >> all l2tp tunnel sockets when a network namespace is being destroyed and >> remove them. I think we can hit this bug with rmmod as well. > > Since I don't use netns or L2TP for real, someone else needs to take > up the crusade here. Let's see if James Chapman is interested. I don't use L2TP for real either. James are you at all interested in the network namespace bugs that have been found in the l2tp code? 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
On 30/10/12 00:21, Eric W. Biederman wrote: > Stephen Hemminger <shemminger@vyatta.com> writes: > >> On Sat, 27 Oct 2012 22:43:13 -0700 >> ebiederm@xmission.com (Eric W. Biederman) wrote: >> >>> Stephen Hemminger <shemminger@vyatta.com> writes: >>> >>>> I noticed that the L2TP sockets are not being moved to the correct name >>>> space. >>>> >>>> Something like this is probably needed. >>> >>> This is almost right. >>> >>> There needs to be a line in l2tp_tunnel_create that verifies >>> the network namespace of the socket derived from a file descriptor >>> and the passed in network namespace match. >>> >>> For the l2tp_tunnel_sock_create case where we have a socket that is not >>> exported to userspace using sk_change_net seems appropriate to avoid >>> reference counting problems. And it may be worth moving that work into >>> sk_create_kern. But we need a network namespace hook that will lookup >>> all l2tp tunnel sockets when a network namespace is being destroyed and >>> remove them. I think we can hit this bug with rmmod as well. >> >> Since I don't use netns or L2TP for real, someone else needs to take >> up the crusade here. > > Let's see if James Chapman is interested. I don't use L2TP for real either. > > James are you at all interested in the network namespace bugs that have > been found in the l2tp code? Very much so, Eric. Thanks for keeping me in the loop. Unfortunately, I am busy on other things at the moment. It's in my queue. I'll get to it as soon as I can. > > Eric
--- a/net/l2tp/l2tp_core.c 2012-10-25 09:11:15.691271882 -0700 +++ b/net/l2tp/l2tp_core.c 2012-10-25 09:18:58.746621418 -0700 @@ -1357,7 +1357,10 @@ static void l2tp_tunnel_free(struct l2tp * userspace. This is used for static tunnels where there is no * managing L2TP daemon. */ -static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct socket **sockp) +static int l2tp_tunnel_sock_create(struct net *net, + u32 tunnel_id, u32 peer_tunnel_id, + struct l2tp_tunnel_cfg *cfg, + struct socket **sockp) { int err = -EINVAL; struct sockaddr_in udp_addr; @@ -1372,11 +1375,12 @@ static int l2tp_tunnel_sock_create(u32 t case L2TP_ENCAPTYPE_UDP: #if IS_ENABLED(CONFIG_IPV6) if (cfg->local_ip6 && cfg->peer_ip6) { - err = sock_create(AF_INET6, SOCK_DGRAM, 0, sockp); + err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, sockp); if (err < 0) goto out; sock = *sockp; + sk_change(sock->sk, net); memset(&udp6_addr, 0, sizeof(udp6_addr)); udp6_addr.sin6_family = AF_INET6; @@ -1400,11 +1404,12 @@ static int l2tp_tunnel_sock_create(u32 t } else #endif { - err = sock_create(AF_INET, SOCK_DGRAM, 0, sockp); + err = sock_create_kern(AF_INET, SOCK_DGRAM, 0, sockp); if (err < 0) goto out; sock = *sockp; + sk_change(sock->sk, net); memset(&udp_addr, 0, sizeof(udp_addr)); udp_addr.sin_family = AF_INET; @@ -1433,7 +1438,7 @@ static int l2tp_tunnel_sock_create(u32 t case L2TP_ENCAPTYPE_IP: #if IS_ENABLED(CONFIG_IPV6) if (cfg->local_ip6 && cfg->peer_ip6) { - err = sock_create(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP, + err = sock_create_kern(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP, sockp); if (err < 0) goto out; @@ -1462,12 +1467,13 @@ static int l2tp_tunnel_sock_create(u32 t } else #endif { - err = sock_create(AF_INET, SOCK_DGRAM, IPPROTO_L2TP, + err = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_L2TP, sockp); if (err < 0) goto out; sock = *sockp; + sk_change(sock->sk, net); memset(&ip_addr, 0, sizeof(ip_addr)); ip_addr.l2tp_family = AF_INET; @@ -1517,7 +1523,8 @@ int l2tp_tunnel_create(struct net *net, * kernel socket. */ if (fd < 0) { - err = l2tp_tunnel_sock_create(tunnel_id, peer_tunnel_id, cfg, &sock); + err = l2tp_tunnel_sock_create(net, tunnel_id, peer_tunnel_id, + cfg, &sock); if (err < 0) goto err; } else {