| Message ID | 20250116160701.8024-3-joshuamurphy@posteo.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net/9p/fd: support ipv6 for trans=tcp | expand |
Joshua Murphy wrote on Thu, Jan 16, 2025 at 04:07:03PM +0000: > Allows specifying an IPv6 address when mounting a remote 9p file system. That was fast! Thank you! A few comments below; I'll try to find time to test shortly once addressed. > Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net> > --- > net/9p/trans_fd.c | 50 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 16 deletions(-) > > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index 196060dc6..effd0261d 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -11,6 +11,7 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/in.h> > +#include <linux/in6.h> > #include <linux/module.h> > #include <linux/net.h> > #include <linux/ipv6.h> > @@ -971,17 +972,24 @@ static inline int valid_ipaddr4(const char *buf) > return 0; > } > > -static int p9_bind_privport(struct socket *sock) > +static int p9_bind_privport(struct socket *sock, int family) > { > - struct sockaddr_in cl; > + struct sockaddr_storage stor; > + struct sockaddr *cl = (struct sockaddr *) &stor; > int port, err = -EINVAL; > > memset(&cl, 0, sizeof(cl)); That memset is for sockaddr size, should it be for sockaddr_storage? > - cl.sin_family = AF_INET; > - cl.sin_addr.s_addr = htonl(INADDR_ANY); > + cl->sa_family = family; > + if (cl->sa_family == AF_INET) > + ((struct sockaddr_in *) cl)->sin_addr.s_addr = htonl(INADDR_ANY); > + else > + ((struct sockaddr_in6 *) cl)->sin6_addr = in6addr_any; > for (port = p9_ipport_resv_max; port >= p9_ipport_resv_min; port--) { > - cl.sin_port = htons((ushort)port); > - err = kernel_bind(sock, (struct sockaddr *)&cl, sizeof(cl)); > + if (cl->sa_family == AF_INET) > + ((struct sockaddr_in *)cl)->sin_port = htons((ushort)port); > + else > + ((struct sockaddr_in6 *)cl)->sin6_port = htons((ushort)port); > + err = kernel_bind(sock, cl, sizeof(cl)); > if (err != -EADDRINUSE) > break; > } > @@ -994,24 +1002,34 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) > { > int err; > struct socket *csocket; > - struct sockaddr_in sin_server; > + struct sockaddr_storage stor; > + struct sockaddr *sin_server = (struct sockaddr *) &stor; not a new problem but I don't see this being initialized to zero either, I'm not sure if there are cases where that can cause problems but it might be just as well to init `stor = { 0 };` or equivalent just in case? > struct p9_fd_opts opts; > > err = parse_opts(args, &opts); > if (err < 0) > return err; > > - if (addr == NULL || valid_ipaddr4(addr) < 0) > + if (addr == NULL) > + return -EINVAL; > + > + if (valid_ipaddr4(addr) == 0) { Hmmm, I wonder if we could just move rpc_pton somewhere and reuse that... It's not that complex but it's certainly pure duplication... But then I found `int_pton_with_scope`, that sounds like an even better match as we probaly want inet6_pton more than in6_pton for local scope identifiers: Could you try to get rid of valid_ipaddr4 and call inet_pton_with_scope with AF_UNSPEC directly? That should do Just What We Want (e.g. try v4 first and fallback to v6), up till filling sa_family and port directly in the right field or failing if neither was valid. (yes, sorry, give a hand and I'll grab the whole arm!) > + sin_server->sa_family = AF_INET; > + ((struct sockaddr_in *) sin_server)->sin_addr.s_addr = in_aton(addr); > + ((struct sockaddr_in *) sin_server)->sin_port = htons(opts.port); > + } > + else if (in6_pton(addr, -1, ((struct sockaddr_in6 *) sin_server)->sin6_addr.s6_addr, -1, NULL)) { > + sin_server->sa_family = AF_INET6; > + ((struct sockaddr_in6 *) sin_server)->sin6_port = htons(opts.port); > + } > + else > return -EINVAL; > > csocket = NULL; > > client->trans_opts.tcp.port = opts.port; > client->trans_opts.tcp.privport = opts.privport; > - sin_server.sin_family = AF_INET; > - sin_server.sin_addr.s_addr = in_aton(addr); > - sin_server.sin_port = htons(opts.port); > - err = __sock_create(current->nsproxy->net_ns, PF_INET, > + err = __sock_create(current->nsproxy->net_ns, sin_server->sa_family, > SOCK_STREAM, IPPROTO_TCP, &csocket, 1); > if (err) { > pr_err("%s (%d): problem creating socket\n", > @@ -1020,7 +1038,7 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) > } > > if (opts.privport) { > - err = p9_bind_privport(csocket); > + err = p9_bind_privport(csocket, sin_server->sa_family); > if (err < 0) { > pr_err("%s (%d): problem binding to privport\n", > __func__, task_pid_nr(current)); > @@ -1029,9 +1047,9 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) > } > } > > - err = READ_ONCE(csocket->ops)->connect(csocket, > - (struct sockaddr *)&sin_server, > - sizeof(struct sockaddr_in), 0); > + err = READ_ONCE(csocket->ops)->connect(csocket, sin_server, > + sizeof(stor), 0); > + > if (err < 0) { > pr_err("%s (%d): problem connecting socket to %s\n", > __func__, task_pid_nr(current), addr);
> That memset is for sockaddr size, should it be for sockaddr_storage? Yeah, it probably should be. Do you think it would make more sense to only initialize the sockaddr_storage as later suggested instead of using the memset? > not a new problem but I don't see this being initialized to zero either, > I'm not sure if there are cases where that can cause problems but it > might be just as well to init `stor = { 0 };` or equivalent just in > case? Makes sense, will do. > Could you try to get rid of valid_ipaddr4 and call inet_pton_with_scope > with AF_UNSPEC directly? That would definitely be the more correct solution, I think. Originally, I was trying not to change the code too much, but I guess if I’m changing it I might as well go all the way :D.
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 196060dc6..effd0261d 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -11,6 +11,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/in.h> +#include <linux/in6.h> #include <linux/module.h> #include <linux/net.h> #include <linux/ipv6.h> @@ -971,17 +972,24 @@ static inline int valid_ipaddr4(const char *buf) return 0; } -static int p9_bind_privport(struct socket *sock) +static int p9_bind_privport(struct socket *sock, int family) { - struct sockaddr_in cl; + struct sockaddr_storage stor; + struct sockaddr *cl = (struct sockaddr *) &stor; int port, err = -EINVAL; memset(&cl, 0, sizeof(cl)); - cl.sin_family = AF_INET; - cl.sin_addr.s_addr = htonl(INADDR_ANY); + cl->sa_family = family; + if (cl->sa_family == AF_INET) + ((struct sockaddr_in *) cl)->sin_addr.s_addr = htonl(INADDR_ANY); + else + ((struct sockaddr_in6 *) cl)->sin6_addr = in6addr_any; for (port = p9_ipport_resv_max; port >= p9_ipport_resv_min; port--) { - cl.sin_port = htons((ushort)port); - err = kernel_bind(sock, (struct sockaddr *)&cl, sizeof(cl)); + if (cl->sa_family == AF_INET) + ((struct sockaddr_in *)cl)->sin_port = htons((ushort)port); + else + ((struct sockaddr_in6 *)cl)->sin6_port = htons((ushort)port); + err = kernel_bind(sock, cl, sizeof(cl)); if (err != -EADDRINUSE) break; } @@ -994,24 +1002,34 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) { int err; struct socket *csocket; - struct sockaddr_in sin_server; + struct sockaddr_storage stor; + struct sockaddr *sin_server = (struct sockaddr *) &stor; struct p9_fd_opts opts; err = parse_opts(args, &opts); if (err < 0) return err; - if (addr == NULL || valid_ipaddr4(addr) < 0) + if (addr == NULL) + return -EINVAL; + + if (valid_ipaddr4(addr) == 0) { + sin_server->sa_family = AF_INET; + ((struct sockaddr_in *) sin_server)->sin_addr.s_addr = in_aton(addr); + ((struct sockaddr_in *) sin_server)->sin_port = htons(opts.port); + } + else if (in6_pton(addr, -1, ((struct sockaddr_in6 *) sin_server)->sin6_addr.s6_addr, -1, NULL)) { + sin_server->sa_family = AF_INET6; + ((struct sockaddr_in6 *) sin_server)->sin6_port = htons(opts.port); + } + else return -EINVAL; csocket = NULL; client->trans_opts.tcp.port = opts.port; client->trans_opts.tcp.privport = opts.privport; - sin_server.sin_family = AF_INET; - sin_server.sin_addr.s_addr = in_aton(addr); - sin_server.sin_port = htons(opts.port); - err = __sock_create(current->nsproxy->net_ns, PF_INET, + err = __sock_create(current->nsproxy->net_ns, sin_server->sa_family, SOCK_STREAM, IPPROTO_TCP, &csocket, 1); if (err) { pr_err("%s (%d): problem creating socket\n", @@ -1020,7 +1038,7 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) } if (opts.privport) { - err = p9_bind_privport(csocket); + err = p9_bind_privport(csocket, sin_server->sa_family); if (err < 0) { pr_err("%s (%d): problem binding to privport\n", __func__, task_pid_nr(current)); @@ -1029,9 +1047,9 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) } } - err = READ_ONCE(csocket->ops)->connect(csocket, - (struct sockaddr *)&sin_server, - sizeof(struct sockaddr_in), 0); + err = READ_ONCE(csocket->ops)->connect(csocket, sin_server, + sizeof(stor), 0); + if (err < 0) { pr_err("%s (%d): problem connecting socket to %s\n", __func__, task_pid_nr(current), addr);
Allows specifying an IPv6 address when mounting a remote 9p file system. Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net> --- net/9p/trans_fd.c | 50 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 16 deletions(-)