[PATCH] net/9p/fd: change port to char *

[PATCH] net/9p/fd: change port to char *

[Joshua Murphy]

The port variable in the option structs is changed to char * to
remove the conversions from u16 to char *.

Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net>
---
 include/net/9p/client.h |  2 +-
 net/9p/trans_fd.c       | 14 ++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 4f785098c..62be2073e 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -114,7 +114,7 @@ struct p9_client {
 			int wfd;
 		} fd;
 		struct {
-			u16 port;
+			char *port;
 			bool privport;
 
 		} tcp;
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 2fea50bf0..2874c7348 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -31,7 +31,7 @@
 
 #include <linux/syscalls.h> /* killme */
 
-#define P9_PORT 564
+#define P9_PORT "564"
 #define MAX_SOCK_BUF (1024*1024)
 #define MAXPOLLWADDR	2
 
@@ -49,7 +49,7 @@ static struct p9_trans_module p9_fd_trans;
 struct p9_fd_opts {
 	int rfd;
 	int wfd;
-	u16 port;
+	char *port;
 	bool privport;
 };
 
@@ -746,8 +746,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
 static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt)
 {
 	if (clnt->trans_mod == &p9_tcp_trans) {
-		if (clnt->trans_opts.tcp.port != P9_PORT)
-			seq_printf(m, ",port=%u", clnt->trans_opts.tcp.port);
+		if (!strcmp(clnt->trans_opts.tcp.port, P9_PORT))
+			seq_printf(m, ",port=%s", clnt->trans_opts.tcp.port);
 	} else if (clnt->trans_mod == &p9_fd_trans) {
 		if (clnt->trans_opts.fd.rfd != ~0)
 			seq_printf(m, ",rfd=%u", clnt->trans_opts.fd.rfd);
@@ -804,7 +804,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts)
 		}
 		switch (token) {
 		case Opt_port:
-			opts->port = option;
+			opts->port = args[0].from;
 			break;
 		case Opt_rfdno:
 			opts->rfd = option;
@@ -981,7 +981,6 @@ 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_storage stor = { 0 };
 	struct p9_fd_opts opts;
@@ -993,9 +992,8 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args)
 	if (!addr)
 		return -EINVAL;
 
-	sprintf(port_str, "%u", opts.port);
 	err = inet_pton_with_scope(current->nsproxy->net_ns, AF_UNSPEC, addr,
-				   port_str, &stor);
+				   opts.port, &stor);
 	if (err < 0)
 		return err;
 
-- 
2.48.1

Re: [PATCH] net/9p/fd: change port to char *

[Daniel Verkamp]

On Fri, Jan 24, 2025 at 1:16 PM Joshua Murphy <joshuamurphy@posteo.net> wrote:
>
> The port variable in the option structs is changed to char * to
> remove the conversions from u16 to char *.
>
> Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net>
> ---
[...]
> @@ -746,8 +746,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
>  static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt)
>  {
>         if (clnt->trans_mod == &p9_tcp_trans) {
> -               if (clnt->trans_opts.tcp.port != P9_PORT)
> -                       seq_printf(m, ",port=%u", clnt->trans_opts.tcp.port);
> +               if (!strcmp(clnt->trans_opts.tcp.port, P9_PORT))
> +                       seq_printf(m, ",port=%s", clnt->trans_opts.tcp.port);

The sense of this check looks reversed; since strcmp() returns 0 when
the strings are equal, this will only print the port number when it
matches the default.

I also wonder if any userspace code relies on the canonicalization of
the port number into decimal; I think specifying (for example)
"port=0x1234" works and would previously show "port=4660" in
/proc/mounts, but now would show the original string in hexadecimal.
This is certainly a corner case, though.

Thanks,
-- Daniel

Re: [PATCH] net/9p/fd: change port to char *

[asmadeus]

Joshua Murphy wrote on Fri, Jan 24, 2025 at 09:14:34PM +0000:
> The port variable in the option structs is changed to char * to
> remove the conversions from u16 to char *.
> 
> Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net>
> ---
>  include/net/9p/client.h |  2 +-
>  net/9p/trans_fd.c       | 14 ++++++--------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index 4f785098c..62be2073e 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -114,7 +114,7 @@ struct p9_client {
>  			int wfd;
>  		} fd;
>  		struct {
> -			u16 port;
> +			char *port;
>  			bool privport;
>  
>  		} tcp;
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 2fea50bf0..2874c7348 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -31,7 +31,7 @@
>  
>  #include <linux/syscalls.h> /* killme */
>  
> -#define P9_PORT 564
> +#define P9_PORT "564"
>  #define MAX_SOCK_BUF (1024*1024)
>  #define MAXPOLLWADDR	2
>  
> @@ -49,7 +49,7 @@ static struct p9_trans_module p9_fd_trans;
>  struct p9_fd_opts {
>  	int rfd;
>  	int wfd;
> -	u16 port;
> +	char *port;
>  	bool privport;
>  };
>  
> @@ -746,8 +746,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
>  static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt)
>  {
>  	if (clnt->trans_mod == &p9_tcp_trans) {
> -		if (clnt->trans_opts.tcp.port != P9_PORT)
> -			seq_printf(m, ",port=%u", clnt->trans_opts.tcp.port);
> +		if (!strcmp(clnt->trans_opts.tcp.port, P9_PORT))

Daniel pointed out at the inverted check (thank you for these reviews!),
but it also needs a null check, because fd/fd_unix variants will not be
setting this field so the strcmp will crash with trans=fd or trans=unix

> +			seq_printf(m, ",port=%s", clnt->trans_opts.tcp.port);
>  	} else if (clnt->trans_mod == &p9_fd_trans) {
>  		if (clnt->trans_opts.fd.rfd != ~0)
>  			seq_printf(m, ",rfd=%u", clnt->trans_opts.fd.rfd);
> @@ -804,7 +804,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts)
>  		}
>  		switch (token) {
>  		case Opt_port:
> -			opts->port = option;
> +			opts->port = args[0].from;

This is a substring from tmp_options that will be freed when parsinig is
finished, so we'll need to kstrdup() it here, which means we'll need to
free it when we're done with options... So, err, at the end of
p9_fd_create which doesn't copy it to the client, and in p9_fd_close for
tcp success (stored in client), and in p9_fd_create_tcp on error
(fd_close not called), and I _think_ that's about it...

I didn't quite estimate the clean up path would be this messy, this
"simple cleanup" is ending up being more complicated than the originial
version so I'm thinking we're better off keeping the int -- what do you
think?

-- 
Dominique Martinet | Asmadeus

Re: [PATCH] net/9p/fd: change port to char *

[Joshua Murphy]

> I didn't quite estimate the clean up path would be this messy, this
> "simple cleanup" is ending up being more complicated than the originial
> version so I'm thinking we're better off keeping the int -- what do you
> think?

Yeah, I’m also thinking that keeping the int would be best.
I don’t think the end result would be more clean than the
current code.

Thanks for the review from you both anyways!

-- 
Joshua Murphy
joshuamurphy@posteo.net