Hi Kuan-Wei,
On 2026-07-06T21:54:42, Kuan-Wei Chiu <[email protected]> wrote:
> net: 9p: Add 9P2000 protocol support
>
> Introduce the core 9P network protocol client implementation.
>
> 9P is a network filesystem protocol originally developed for the
> Plan 9 operating system. Add the baseline 9P2000.L protocol handling,
> including message serialization, client registration, and basic
> transport operations.
>
> Signed-off-by: Kuan-Wei Chiu <[email protected]>
>
> include/9p.h | 82 +++++++++++++++++
> net/9p/Kconfig | 5 ++
> net/9p/Makefile | 5 ++
> net/9p/client.c | 272
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> net/Kconfig | 2 +
> net/Makefile | 1 +
> 6 files changed, 367 insertions(+)
> net: 9p: Add 9P2000 protocol support
The subject says 9P2000 but the client only negotiates and implements
9P2000.L, which is a different dialect. Please can you say 9P2000.L
consistently throughout the series?
> diff --git a/include/9p.h b/include/9p.h
> @@ -0,0 +1,82 @@
> +struct p9_client {
> + const struct p9_trans_ops *ops;
> + void *priv;
> + u8 *tx;
> + u8 *rx;
> + u32 next_fid;
> +};
Please can you add comments to the structs and exported functions in
this header, in the normal U-Boot style (see include/fs.h)? Each
function should document its parameters and return value, and each
struct member should have a comment.
> diff --git a/include/9p.h b/include/9p.h
> @@ -0,0 +1,82 @@
> +struct blk_desc;
> +struct disk_partition;
> +
> +int p9_fs_probe(struct blk_desc *fs_dev_desc, struct disk_partition
> *fs_partition);
These p9_fs_*() functions are only implemented in fs/9p in the next
patch, so please move the declarations there, so each patch is
self-contained.
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> +#include <9p.h>
> +#include <config.h>
> +#include <log.h>
Please define LOG_CATEGORY at the top of the file (LOGC_NET seems
right) so the log calls can be filtered. The <config.h> include is
not needed, since the build system includes it automatically.
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + if (rx_hdr->id == P9_RLERROR) {
> + ecode = get_unaligned_le32(c->rx + 7);
> + log_err("9P Error: %s failed (ecode=%u)\n", step, ecode);
> + return -EIO;
In 9P2000.L the Rlerror ecode is a Linux errno value, so please return
-ecode here rather than -EIO, so callers can distinguish ENOENT from
real I/O errors (as fs/9p does in the next patch). Also please use
sizeof(struct p9_header) instead of the magic 7
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + if (!c->tx)
> + c->tx = memalign(ARCH_DMA_MINALIGN, P9_MSIZE);
> + if (!c->rx)
> + c->rx = memalign(ARCH_DMA_MINALIGN, P9_MSIZE);
These 64KB allocations can fail on boards with a small malloc() pool,
so please check for NULL and return -ENOMEM
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + if (c->ops->request(c, c->tx, hdr->size, c->rx, P9_MSIZE) < 0)
> + return -EIO;
> +
> + return p9_check_error(c, P9_RVERSION, 'Tversion');
The Rversion reply carries the server's negotiated msize and version
string, but both are ignored here. A server may reply with a smaller
msize than requested, in which case the P9_MSIZE - 24 reads in
p9_client_read() can exceed what it accepts, or with the version
string 'unknown' if it does not support 9P2000.L. Please parse the
reply, store the negotiated msize in struct p9_client for the read
paths to use, and fail if the version string does not match.
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + char path_buf[128];
> + char *wnames[16];
strlcpy() silently truncates paths longer than 127 characters, so the
walk would quietly go to the wrong file. Please check its return value
against sizeof(path_buf) and return -ENAMETOOLONG on truncation.
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + return p9_check_error(c, P9_RWALK, 'Twalk');
Not quite complete: per the 9P spec, only a failure on the first path
element produces Rlerror. If a later element fails, the server returns
Rwalk with nwqid < nwname and new_fid is not created, yet this returns
0. Please check that nwqid equals nwname and return -ENOENT otherwise,
so callers such as p9_fs_exists() get the right answer.
> diff --git a/net/9p/client.c b/net/9p/client.c
> @@ -0,0 +1,272 @@
> + if (p9_check_error(c, P9_RREAD, 'Tread') == 0) {
> + ptr = c->rx + sizeof(*hdr);
> + *actread = get_unaligned_le32(ptr);
> + ptr += 4;
> + memcpy(buf, ptr, *actread);
The count returned by the server is trusted here - a buggy or
malicious server could return a value larger than read_len (or the rx
buffer) and overflow the caller's buffer. Please check it against
read_len before the memcpy(). The same applies in p9_client_readdir()
Regards,
Simon
