| Message ID | 87oalu1p16.fsf_-_@x220.int.ebiederm.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric W. Biederman > Sent: 09 May 2015 03:08 > > This is long overdue, and is part of cleaning up how we allocate kernel > sockets that don't reference count struct net. ... > diff --git a/net/socket.c b/net/socket.c > index b5f1f43ed8f4..9963a0b53a64 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -1210,9 +1210,9 @@ int sock_create(int family, int type, int protocol, struct socket **res) > } > EXPORT_SYMBOL(sock_create); > > -int sock_create_kern(int family, int type, int protocol, struct socket **res) > +int sock_create_kern(struct net *net, int family, int type, int protocol, struct socket **res) > { > - return __sock_create(&init_net, family, type, protocol, res, 1); > + return __sock_create(net, family, type, protocol, res, 1); > } > EXPORT_SYMBOL(sock_create_kern); Wouldn't it involve far less churn to add a new function that uses a non-default namespace? Changing the function prototype will a PITA for anyone doing back-ports of fixes. (And more so for anyone trying to get a driver to build against kernels that might have this change back-ported.) David -- 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 May 12, 2015 3:24:11 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: >From: Eric W. Biederman >> Sent: 09 May 2015 03:08 >> >> This is long overdue, and is part of cleaning up how we allocate >kernel >> sockets that don't reference count struct net. >... >> diff --git a/net/socket.c b/net/socket.c >> index b5f1f43ed8f4..9963a0b53a64 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -1210,9 +1210,9 @@ int sock_create(int family, int type, int >protocol, struct socket **res) >> } >> EXPORT_SYMBOL(sock_create); >> >> -int sock_create_kern(int family, int type, int protocol, struct >socket **res) >> +int sock_create_kern(struct net *net, int family, int type, int >protocol, struct socket **res) >> { >> - return __sock_create(&init_net, family, type, protocol, res, 1); >> + return __sock_create(net, family, type, protocol, res, 1); >> } >> EXPORT_SYMBOL(sock_create_kern); > >Wouldn't it involve far less churn to add a new function that uses a >non-default >namespace? The goal is comprehensible and maintainable kernel code. Which network namespace your socket is in, is an important property and something you probably care about if you are creating kernel sockets. Having a second function is more maintenance and results in harder to understand code. A major fraction of the callers even before this change wanted to be outside the initial network namespace. This change should have been made years ago, but unfortunately it was not. >Changing the function prototype will a PITA for anyone doing back-ports >of fixes. >(And more so for anyone trying to get a driver to build against kernels >that might have this change back-ported.) Yep dealing with backports sucks, my sympathies. 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
From: Eric W. Biederman > Sent: 12 May 2015 09:55 > > On May 12, 2015 3:24:11 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: ... > >Wouldn't it involve far less churn to add a new function that uses a > >non-default namespace? > > The goal is comprehensible and maintainable kernel code. > > Which network namespace your socket is in, is an important property and something you probably care > about if you are creating kernel sockets. That rather depends on whether you've anywhere to get a namespace from. eg something like ceph/messenger.c > Having a second function is more maintenance and results in harder to understand code. A major > fraction of the callers even before this change wanted to be outside the initial network namespace. A static inline in the header file wouldn't cause any maintenance issues. David
Le 12/05/2015 13:48, David Laight a écrit : > From: Eric W. Biederman >> Sent: 12 May 2015 09:55 >> >> On May 12, 2015 3:24:11 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: > ... >>> Wouldn't it involve far less churn to add a new function that uses a >>> non-default namespace? >> >> The goal is comprehensible and maintainable kernel code. >> >> Which network namespace your socket is in, is an important property and something you probably care >> about if you are creating kernel sockets. > > That rather depends on whether you've anywhere to get a namespace from. > eg something like ceph/messenger.c sk_net(con->sock->sk)? This parameter is essential, hiding it will just hides bugs. Having this parameter forces the developer to ask himself what is the best value. -- 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
From: Nicolas Dichtel [mailto:nicolas.dichtel@6wind.com] > Sent: 12 May 2015 13:29 > Le 12/05/2015 13:48, David Laight a écrit : > > From: Eric W. Biederman > >> Sent: 12 May 2015 09:55 > >> > >> On May 12, 2015 3:24:11 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: > > ... > >>> Wouldn't it involve far less churn to add a new function that uses a > >>> non-default namespace? > >> > >> The goal is comprehensible and maintainable kernel code. > >> > >> Which network namespace your socket is in, is an important property and something you probably care > >> about if you are creating kernel sockets. > > > > That rather depends on whether you've anywhere to get a namespace from. > > eg something like ceph/messenger.c > sk_net(con->sock->sk)? What if you don't already have a socket? Just an IP(v6) address ? > This parameter is essential, hiding it will just hides bugs. > Having this parameter forces the developer to ask himself what is the best > value. What if the answer is 'NFI'? Which requires the answer be pushed back to the 'application' configuration? Most users will have no idea either. David
Le 12/05/2015 15:16, David Laight a écrit : > From: Nicolas Dichtel [mailto:nicolas.dichtel@6wind.com] >> Sent: 12 May 2015 13:29 >> Le 12/05/2015 13:48, David Laight a écrit : >>> From: Eric W. Biederman >>>> Sent: 12 May 2015 09:55 >>>> >>>> On May 12, 2015 3:24:11 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: >>> ... >>>>> Wouldn't it involve far less churn to add a new function that uses a >>>>> non-default namespace? >>>> >>>> The goal is comprehensible and maintainable kernel code. >>>> >>>> Which network namespace your socket is in, is an important property and something you probably care >>>> about if you are creating kernel sockets. >>> >>> That rather depends on whether you've anywhere to get a namespace from. >>> eg something like ceph/messenger.c >> sk_net(con->sock->sk)? > > What if you don't already have a socket? > Just an IP(v6) address ? So you have an IPv6 address and you don't know on which netns this address belongs? I can't imagine how it can work. > >> This parameter is essential, hiding it will just hides bugs. >> Having this parameter forces the developer to ask himself what is the best >> value. > > What if the answer is 'NFI'? > Which requires the answer be pushed back to the 'application' configuration? Your application is bound to a netns. > Most users will have no idea either. If you support netns, you *have to know*. Regards, Nicolas -- 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
From: David Laight <David.Laight@ACULAB.COM> Date: Tue, 12 May 2015 08:24:11 +0000 > Changing the function prototype will a PITA for anyone doing back-ports of fixes. > (And more so for anyone trying to get a driver to build against kernels > that might have this change back-ported.) As someone who actually does such backports every week or two for up to 5 different -stable releases, I really don't find it to be a big deal, and I've therefore developed a very low level of sympathy for this argument so please don't make it as a reason to not make an interface change. Thanks. -- 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 May 12, 2015 8:16:23 AM CDT, David Laight <David.Laight@ACULAB.COM> wrote: >From: Nicolas Dichtel [mailto:nicolas.dichtel@6wind.com] >> Sent: 12 May 2015 13:29 >> Le 12/05/2015 13:48, David Laight a écrit : >> > From: Eric W. Biederman >> >> Sent: 12 May 2015 09:55 >> >> >> >> On May 12, 2015 3:24:11 AM CDT, David Laight ><David.Laight@ACULAB.COM> wrote: >> > ... >> >>> Wouldn't it involve far less churn to add a new function that >uses a >> >>> non-default namespace? >> >> >> >> The goal is comprehensible and maintainable kernel code. >> >> >> >> Which network namespace your socket is in, is an important >property and something you probably care >> >> about if you are creating kernel sockets. >> > >> > That rather depends on whether you've anywhere to get a namespace >from. >> > eg something like ceph/messenger.c >> sk_net(con->sock->sk)? > >What if you don't already have a socket? >Just an IP(v6) address ? > >> This parameter is essential, hiding it will just hides bugs. >> Having this parameter forces the developer to ask himself what is the >best >> value. > >What if the answer is 'NFI'? >Which requires the answer be pushed back to the 'application' >configuration? >Most users will have no idea either. current->nsproxy->netns. Capture it at mount time. Call get_net then and put_net after you have cleaned up during unmount. Not hard, and the parameter is doing what it is supposed to be doing getting you to ask the question, and realize there is an unhandled issue. 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/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c index cee20354ac37..c097909c589c 100644 --- a/drivers/block/drbd/drbd_receiver.c +++ b/drivers/block/drbd/drbd_receiver.c @@ -598,7 +598,7 @@ static struct socket *drbd_try_connect(struct drbd_connection *connection) memcpy(&peer_in6, &connection->peer_addr, peer_addr_len); what = "sock_create_kern"; - err = sock_create_kern(((struct sockaddr *)&src_in6)->sa_family, + err = sock_create_kern(&init_net, ((struct sockaddr *)&src_in6)->sa_family, SOCK_STREAM, IPPROTO_TCP, &sock); if (err < 0) { sock = NULL; @@ -693,7 +693,7 @@ static int prepare_listen_socket(struct drbd_connection *connection, struct acce memcpy(&my_addr, &connection->my_addr, my_addr_len); what = "sock_create_kern"; - err = sock_create_kern(((struct sockaddr *)&my_addr)->sa_family, + err = sock_create_kern(&init_net, ((struct sockaddr *)&my_addr)->sa_family, SOCK_STREAM, IPPROTO_TCP, &s_listen); if (err) { s_listen = NULL; diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c index 3a57a1b0fb51..b50642870a43 100644 --- a/fs/afs/rxrpc.c +++ b/fs/afs/rxrpc.c @@ -85,7 +85,7 @@ int afs_open_socket(void) return -ENOMEM; } - ret = sock_create_kern(AF_RXRPC, SOCK_DGRAM, PF_INET, &socket); + ret = sock_create_kern(&init_net, AF_RXRPC, SOCK_DGRAM, PF_INET, &socket); if (ret < 0) { destroy_workqueue(afs_async_calls); _leave(" = %d [socket]", ret); diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c index d08e079ea5d3..754fd6c0b747 100644 --- a/fs/dlm/lowcomms.c +++ b/fs/dlm/lowcomms.c @@ -921,8 +921,8 @@ static int tcp_accept_from_sock(struct connection *con) mutex_unlock(&connections_lock); memset(&peeraddr, 0, sizeof(peeraddr)); - result = sock_create_kern(dlm_local_addr[0]->ss_family, SOCK_STREAM, - IPPROTO_TCP, &newsock); + result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family, + SOCK_STREAM, IPPROTO_TCP, &newsock); if (result < 0) return -ENOMEM; @@ -1173,8 +1173,8 @@ static void tcp_connect_to_sock(struct connection *con) goto out; /* Create a socket to communicate with */ - result = sock_create_kern(dlm_local_addr[0]->ss_family, SOCK_STREAM, - IPPROTO_TCP, &sock); + result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family, + SOCK_STREAM, IPPROTO_TCP, &sock); if (result < 0) goto out_err; @@ -1258,8 +1258,8 @@ static struct socket *tcp_create_listen_sock(struct connection *con, addr_len = sizeof(struct sockaddr_in6); /* Create a socket to communicate with */ - result = sock_create_kern(dlm_local_addr[0]->ss_family, SOCK_STREAM, - IPPROTO_TCP, &sock); + result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family, + SOCK_STREAM, IPPROTO_TCP, &sock); if (result < 0) { log_print("Can't create listening comms socket"); goto create_out; @@ -1365,8 +1365,8 @@ static int sctp_listen_for_all(void) log_print("Using SCTP for communications"); - result = sock_create_kern(dlm_local_addr[0]->ss_family, SOCK_SEQPACKET, - IPPROTO_SCTP, &sock); + result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family, + SOCK_SEQPACKET, IPPROTO_SCTP, &sock); if (result < 0) { log_print("Can't create comms socket, check SCTP is loaded"); goto out; diff --git a/include/linux/net.h b/include/linux/net.h index 8a5e81d2bdf7..04aa06852771 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -207,7 +207,7 @@ void sock_unregister(int family); int __sock_create(struct net *net, int family, int type, int proto, struct socket **res, int kern); int sock_create(int family, int type, int proto, struct socket **res); -int sock_create_kern(int family, int type, int proto, struct socket **res); +int sock_create_kern(struct net *net, int family, int type, int proto, struct socket **res); int sock_create_lite(int family, int type, int proto, struct socket **res); void sock_release(struct socket *sock); int sock_sendmsg(struct socket *sock, struct msghdr *msg); diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index 4fea24275b17..29709fbfd1f5 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -200,7 +200,7 @@ static int rfcomm_l2sock_create(struct socket **sock) BT_DBG(""); - err = sock_create_kern(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP, sock); + err = sock_create_kern(&init_net, PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP, sock); if (!err) { struct sock *sk = (*sock)->sk; sk->sk_data_ready = rfcomm_l2data_ready; diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 967080a9f043..073262fea6dd 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -480,8 +480,8 @@ static int ceph_tcp_connect(struct ceph_connection *con) int ret; BUG_ON(con->sock); - ret = sock_create_kern(con->peer_addr.in_addr.ss_family, SOCK_STREAM, - IPPROTO_TCP, &sock); + ret = sock_create_kern(&init_net, con->peer_addr.in_addr.ss_family, + SOCK_STREAM, IPPROTO_TCP, &sock); if (ret) return ret; sock->sk->sk_allocation = GFP_NOFS; diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 8b47a4d79d04..09f4d024dfe5 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1430,7 +1430,7 @@ int inet_ctl_sock_create(struct sock **sk, unsigned short family, struct net *net) { struct socket *sock; - int rc = sock_create_kern(family, type, protocol, &sock); + int rc = sock_create_kern(&init_net, family, type, protocol, &sock); if (rc == 0) { *sk = sock->sk; diff --git a/net/ipv4/udp_tunnel.c b/net/ipv4/udp_tunnel.c index 6bb98cc193c9..4e2837476967 100644 --- a/net/ipv4/udp_tunnel.c +++ b/net/ipv4/udp_tunnel.c @@ -15,7 +15,7 @@ int udp_sock_create4(struct net *net, struct udp_port_cfg *cfg, struct socket *sock = NULL; struct sockaddr_in udp_addr; - err = sock_create_kern(AF_INET, SOCK_DGRAM, 0, &sock); + err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &sock); if (err < 0) goto error; diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c index bba8903e871f..478576b61214 100644 --- a/net/ipv6/ip6_udp_tunnel.c +++ b/net/ipv6/ip6_udp_tunnel.c @@ -19,7 +19,7 @@ int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg, int err; struct socket *sock = NULL; - err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, &sock); + err = sock_create_kern(&init_net, AF_INET6, SOCK_DGRAM, 0, &sock); if (err < 0) goto error; diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index a29a504492af..ae513a2fe7f3 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1399,7 +1399,7 @@ static int l2tp_tunnel_sock_create(struct net *net, if (cfg->local_ip6 && cfg->peer_ip6) { struct sockaddr_l2tpip6 ip6_addr = {0}; - err = sock_create_kern(AF_INET6, SOCK_DGRAM, + err = sock_create_kern(&init_net, AF_INET6, SOCK_DGRAM, IPPROTO_L2TP, &sock); if (err < 0) goto out; @@ -1429,7 +1429,7 @@ static int l2tp_tunnel_sock_create(struct net *net, { struct sockaddr_l2tpip ip_addr = {0}; - err = sock_create_kern(AF_INET, SOCK_DGRAM, + err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, IPPROTO_L2TP, &sock); if (err < 0) goto out; diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c index 19b9cce6c210..2e9a5b5d1239 100644 --- a/net/netfilter/ipvs/ip_vs_sync.c +++ b/net/netfilter/ipvs/ip_vs_sync.c @@ -1458,7 +1458,7 @@ static struct socket *make_send_sock(struct net *net, int id) int result; /* First create a socket move it to right name space later */ - result = sock_create_kern(PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock); + result = sock_create_kern(&init_net, PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock); if (result < 0) { pr_err("Error during creation of socket; terminating\n"); return ERR_PTR(result); @@ -1518,7 +1518,7 @@ static struct socket *make_receive_sock(struct net *net, int id) int result; /* First create a socket */ - result = sock_create_kern(PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock); + result = sock_create_kern(&init_net, PF_INET, SOCK_DGRAM, IPPROTO_UDP, &sock); if (result < 0) { pr_err("Error during creation of socket; terminating\n"); return ERR_PTR(result); diff --git a/net/rxrpc/ar-local.c b/net/rxrpc/ar-local.c index ca904ed5400a..78483b4602bf 100644 --- a/net/rxrpc/ar-local.c +++ b/net/rxrpc/ar-local.c @@ -73,8 +73,8 @@ static int rxrpc_create_local(struct rxrpc_local *local) _enter("%p{%d}", local, local->srx.transport_type); /* create a socket to represent the local endpoint */ - ret = sock_create_kern(PF_INET, local->srx.transport_type, IPPROTO_UDP, - &local->socket); + ret = sock_create_kern(&init_net, PF_INET, local->srx.transport_type, + IPPROTO_UDP, &local->socket); if (ret < 0) { _leave(" = %d [socket]", ret); return ret; diff --git a/net/socket.c b/net/socket.c index b5f1f43ed8f4..9963a0b53a64 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1210,9 +1210,9 @@ int sock_create(int family, int type, int protocol, struct socket **res) } EXPORT_SYMBOL(sock_create); -int sock_create_kern(int family, int type, int protocol, struct socket **res) +int sock_create_kern(struct net *net, int family, int type, int protocol, struct socket **res) { - return __sock_create(&init_net, family, type, protocol, res, 1); + return __sock_create(net, family, type, protocol, res, 1); } EXPORT_SYMBOL(sock_create_kern);
This is long overdue, and is part of cleaning up how we allocate kernel sockets that don't reference count struct net. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- drivers/block/drbd/drbd_receiver.c | 4 ++-- fs/afs/rxrpc.c | 2 +- fs/dlm/lowcomms.c | 16 ++++++++-------- include/linux/net.h | 2 +- net/bluetooth/rfcomm/core.c | 2 +- net/ceph/messenger.c | 4 ++-- net/ipv4/af_inet.c | 2 +- net/ipv4/udp_tunnel.c | 2 +- net/ipv6/ip6_udp_tunnel.c | 2 +- net/l2tp/l2tp_core.c | 4 ++-- net/netfilter/ipvs/ip_vs_sync.c | 4 ++-- net/rxrpc/ar-local.c | 4 ++-- net/socket.c | 4 ++-- 13 files changed, 26 insertions(+), 26 deletions(-)