[PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[Joshua Murphy]

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(-)

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);
-- 
2.48.1

Re: [PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[Daniel Verkamp]

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.

Re: [PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[Joshua Murphy]

> 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
joshuamurphy@posteo.net

Re: [PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[asmadeus]

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,
-- 
Dominique Martinet | Asmadeus

Re: [PATCH v2] net/9p/fd: support ipv6 for trans=tcp

[Joshua Murphy]

> 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.

-- 
Joshua Murphy
joshuamurphy@posteo.net