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

[asmadeus@codewreck.org]

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

-- 
Dominique Martinet | Asmadeus