| Message ID | 20250117214943.6368-2-joshuamurphy@posteo.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net/9p/fd: support ipv6 for trans=tcp | expand |
On Fri, Jan 17, 2025 at 1:52 PM Joshua Murphy <joshuamurphy@posteo.net> wrote: > > Allows specifying an IPv6 address when mounting a remote 9p file system. > > Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net> > --- [...] > - cl.sin_family = AF_INET; > - cl.sin_addr.s_addr = htonl(INADDR_ANY); > + stor.ss_family = sock->ops->family; > + inet_addr_is_any((struct sockaddr *) &stor); It looks like inet_addr_is_any() checks the current sin_addr/sin6_addr of a sockaddr, but does not change it, so this doesn't seem quite right (its only effect is to return a bool, which is discarded here). [...] > - if (addr == NULL || valid_ipaddr4(addr) < 0) > - return -EINVAL; > + sprintf(port_str, "%u", opts.port); > + err = inet_pton_with_scope(current->nsproxy->net_ns, AF_UNSPEC, addr, > + port_str, &stor); > + if (addr == NULL || err < 0) { inet_pton_with_scope() appears to unconditionally dereference its `src` parameter (`addr` in the call above), so the `addr == NULL` check is too late.
> It looks like inet_addr_is_any() checks the current sin_addr/sin6_addr > of a sockaddr, but does not change it, so this doesn't seem quite > right (its only effect is to return a bool, which is discarded here). Huh, I’ll definitely fix that. For some reason I must’ve thought it changed the addresses rather than checking them. > inet_pton_with_scope() appears to unconditionally dereference its > `src` parameter (`addr` in the call above), so the `addr == NULL` > check is too late. I’ll put it in a separate check before the inet one. Thanks,
Joshua Murphy wrote on Fri, Jan 17, 2025 at 09:44:46PM +0000: > Allows specifying an IPv6 address when mounting a remote 9p file system. Thanks for the v2! Daniel beat me to the important bits, just a couple more. > - if (addr == NULL || valid_ipaddr4(addr) < 0) valid_ipaddr4 is no longer used so please also remove the function definition just above as well > - return -EINVAL; > + sprintf(port_str, "%u", opts.port); Oh, I hadn't realized this takes a string.. The previous string -> u16 conversion into opts.port happens in the same file (parse_opts), and trans_opts is only used to print options in /proc/mount later so can be made a char * as well, but that is code cleanup that can (and probably should) be done in a separate patch. Let's keep the sprintf here and I'll try to find time for that later unless you want to do it. Cheers,
> Let's keep the sprintf here and I'll try to find time for that later > unless you want to do it. Doesn’t seem too crazy, so I’ll do it once this one is good.
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 196060dc6..eaf72067e 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> @@ -973,45 +974,48 @@ static inline int valid_ipaddr4(const char *buf) static int p9_bind_privport(struct socket *sock) { - struct sockaddr_in cl; + struct sockaddr_storage stor = { 0 }; int port, err = -EINVAL; - memset(&cl, 0, sizeof(cl)); - cl.sin_family = AF_INET; - cl.sin_addr.s_addr = htonl(INADDR_ANY); + stor.ss_family = sock->ops->family; + inet_addr_is_any((struct sockaddr *) &stor); 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 (stor.ss_family == AF_INET) + ((struct sockaddr_in *) &stor)->sin_port = htons((ushort)port); + else + ((struct sockaddr_in6 *) &stor)->sin6_port = htons((ushort)port); + err = kernel_bind(sock, (struct sockaddr *) &stor, sizeof(stor)); if (err != -EADDRINUSE) break; } return err; } - static int p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) { int err; + char port_str[6]; struct socket *csocket; - struct sockaddr_in sin_server; + struct sockaddr_storage stor = { 0 }; struct p9_fd_opts opts; err = parse_opts(args, &opts); if (err < 0) return err; - if (addr == NULL || valid_ipaddr4(addr) < 0) - return -EINVAL; + sprintf(port_str, "%u", opts.port); + err = inet_pton_with_scope(current->nsproxy->net_ns, AF_UNSPEC, addr, + port_str, &stor); + if (addr == NULL || err < 0) { + return err; + } 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, stor.ss_family, SOCK_STREAM, IPPROTO_TCP, &csocket, 1); if (err) { pr_err("%s (%d): problem creating socket\n", @@ -1030,8 +1034,8 @@ 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); + (struct sockaddr *) &stor, + 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> --- v1 -> v2: Simplified support using inet_pton_with_scope, initialized sockaddr_storage variables to zero, and removed unneeded argument from p9_bind_privport. net/9p/trans_fd.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-)