Follow-up fix for:
https://lore.kernel.org/linux-kernel/[email protected]/
Stefano, I am optimistic that this also works with Xen, but I have not
tested it.
Christian Schoenebeck (2):
net/9p: distinguish zero-copy requests
net/9p: fix response size check in p9_check_errors()
include/net/9p/9p.h | 2 ++
net/9p/client.c | 12 ++++++++----
2 files changed, 10 insertions(+), 4 deletions(-)
--
2.30.2
Since 60ece0833b6c (net/9p: allocate appropriate reduced message buffers)
it is no longer appropriate to check server's response size against
msize. Check against the previously allocated buffer capacity instead.
- Omit this size check entirely for zero-copy messages, as those always
allocate 4k (P9_ZC_HDR_SZ) linear buffers which are not used for actual
payload and can be much bigger than 4k.
- Replace p9_debug() by pr_err() to make sure this message is always
printed in case this error is triggered.
- Add 9p message type to error message to ease investigation.
Signed-off-by: Christian Schoenebeck <[email protected]>
---
net/9p/client.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/9p/client.c b/net/9p/client.c
index 30dd82f49b28..63f13dd1ecff 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -514,10 +514,10 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
int ecode;
err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
- if (req->rc.size >= c->msize) {
- p9_debug(P9_DEBUG_ERROR,
- "requested packet size too big: %d\n",
- req->rc.size);
+ if (req->rc.size > req->rc.capacity && !req->rc.zc) {
+ pr_err(
+ "requested packet size too big: %d does not fit %ld (type=%d)\n",
+ req->rc.size, req->rc.capacity, req->rc.id);
return -EIO;
}
/* dump the response from server
--
2.30.2
Signed-off-by: Christian Schoenebeck <[email protected]>
---
include/net/9p/9p.h | 2 ++
net/9p/client.c | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/include/net/9p/9p.h b/include/net/9p/9p.h
index 13abe013af21..b0a6dec20b92 100644
--- a/include/net/9p/9p.h
+++ b/include/net/9p/9p.h
@@ -530,6 +530,7 @@ struct p9_rstatfs {
* @tag: transaction id of the request
* @offset: used by marshalling routines to track current position in buffer
* @capacity: used by marshalling routines to track total malloc'd capacity
+ * @zc: whether zero-copy is used
* @sdata: payload
*
* &p9_fcall represents the structure for all 9P RPC
@@ -546,6 +547,7 @@ struct p9_fcall {
size_t offset;
size_t capacity;
+ bool zc;
struct kmem_cache *cache;
u8 *sdata;
diff --git a/net/9p/client.c b/net/9p/client.c
index aaa37b07e30a..30dd82f49b28 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -680,6 +680,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
if (IS_ERR(req))
return req;
+ req->tc.zc = req->rc.zc = false;
+
if (signal_pending(current)) {
sigpending = 1;
clear_thread_flag(TIF_SIGPENDING);
@@ -778,6 +780,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
if (IS_ERR(req))
return req;
+ req->tc.zc = req->rc.zc = true;
+
if (signal_pending(current)) {
sigpending = 1;
clear_thread_flag(TIF_SIGPENDING);
--
2.30.2
Christian Schoenebeck wrote on Tue, Nov 22, 2022 at 12:04:08AM +0100:
> Since 60ece0833b6c (net/9p: allocate appropriate reduced message buffers)
> it is no longer appropriate to check server's response size against
> msize. Check against the previously allocated buffer capacity instead.
Thanks for the follow up!
> - Omit this size check entirely for zero-copy messages, as those always
> allocate 4k (P9_ZC_HDR_SZ) linear buffers which are not used for actual
> payload and can be much bigger than 4k.
[review includes the new flag patch]
hmm, unless there's anywhere else you think we might use these flags it
looks simpler to just pass a flag to p9_check_errors?
In particular adding a bool in this position is not particularly efficient:
-------(pahole)-----
struct p9_fcall {
u32 size; /* 0 4 */
u8 id; /* 4 1 */
/* XXX 1 byte hole, try to pack */
u16 tag; /* 6 2 */
size_t offset; /* 8 8 */
size_t capacity; /* 16 8 */
bool zc; /* 24 1 */
/* XXX 7 bytes hole, try to pack */
struct kmem_cache * cache; /* 32 8 */
u8 * sdata; /* 40 8 */
/* size: 48, cachelines: 1, members: 8 */
/* sum members: 40, holes: 2, sum holes: 8 */
/* last cacheline: 48 bytes */
};
----------------
Not that adding it between id and tag sounds better to me, so this is
probably just as good as anywhere else :-D
Anyway, I'm just nitpicking -- on principle I agree just whitelisting zc
requests from this check makes most sense, happy with either way if you
think this is better for the future.
> - Replace p9_debug() by pr_err() to make sure this message is always
> printed in case this error is triggered.
>
> - Add 9p message type to error message to ease investigation.
Yes to these log changes!
>
> Signed-off-by: Christian Schoenebeck <[email protected]>
> ---
> net/9p/client.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 30dd82f49b28..63f13dd1ecff 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -514,10 +514,10 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
> int ecode;
>
> err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
> - if (req->rc.size >= c->msize) {
> - p9_debug(P9_DEBUG_ERROR,
> - "requested packet size too big: %d\n",
> - req->rc.size);
> + if (req->rc.size > req->rc.capacity && !req->rc.zc) {
> + pr_err(
> + "requested packet size too big: %d does not fit %ld (type=%d)\n",
> + req->rc.size, req->rc.capacity, req->rc.id);
Haven't seen this style before -- is that what qemu uses?
We normally keep the message on first line and align e.g.
> + pr_err("requested packet size too big: %d does not fit %ld (type=%d)\n",
> + req->rc.size, req->rc.capacity, req->rc.id);
(at least what's what other grep -A 1 'pr_err.*,$' seem to do, and
checkpatch is happier with that)
--
Dominique
On Tue, 22 Nov 2022, Christian Schoenebeck wrote:
> Follow-up fix for:
> https://lore.kernel.org/linux-kernel/[email protected]/
>
> Stefano, I am optimistic that this also works with Xen, but I have not
> tested it.
Tested-by: Stefano Stabellini <[email protected]>
> Christian Schoenebeck (2):
> net/9p: distinguish zero-copy requests
> net/9p: fix response size check in p9_check_errors()
>
> include/net/9p/9p.h | 2 ++
> net/9p/client.c | 12 ++++++++----
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> --
> 2.30.2
>
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.1-rc6]
[also build test WARNING on linus/master next-20221122]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Schoenebeck/net-9p-fix-response-size-check-in-p9_check_errors/20221122-075849
patch link: https://lore.kernel.org/r/fffb512c532bf1290f0f2b1df6068b2ff6cd14c0.1669072186.git.linux_oss%40crudebyte.com
patch subject: [PATCH 2/2] net/9p: fix response size check in p9_check_errors()
config: hexagon-randconfig-r041-20221120
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project af8c49dc1ec44339d915d988ffe0f38da68ca0e7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/33e592d4a74029c0a04584fbc23c55a91fd28dcb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Schoenebeck/net-9p-fix-response-size-check-in-p9_check_errors/20221122-075849
git checkout 33e592d4a74029c0a04584fbc23c55a91fd28dcb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash net/9p/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from net/9p/client.c:29:
In file included from include/trace/events/9p.h:222:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:21:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from net/9p/client.c:29:
In file included from include/trace/events/9p.h:222:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:21:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from net/9p/client.c:29:
In file included from include/trace/events/9p.h:222:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:21:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> net/9p/client.c:520:19: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
req->rc.size, req->rc.capacity, req->rc.id);
^~~~~~~~~~~~~~~~
include/linux/printk.h:500:33: note: expanded from macro 'pr_err'
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/printk.h:457:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/printk.h:429:19: note: expanded from macro 'printk_index_wrap'
_p_func(_fmt, ##__VA_ARGS__); \
~~~~ ^~~~~~~~~~~
7 warnings generated.
vim +520 net/9p/client.c
498
499 /**
500 * p9_check_errors - check 9p packet for error return and process it
501 * @c: current client instance
502 * @req: request to parse and check for error conditions
503 *
504 * returns error code if one is discovered, otherwise returns 0
505 *
506 * this will have to be more complicated if we have multiple
507 * error packet types
508 */
509
510 static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
511 {
512 s8 type;
513 int err;
514 int ecode;
515
516 err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
517 if (req->rc.size > req->rc.capacity && !req->rc.zc) {
518 pr_err(
519 "requested packet size too big: %d does not fit %ld (type=%d)\n",
> 520 req->rc.size, req->rc.capacity, req->rc.id);
521 return -EIO;
522 }
523 /* dump the response from server
524 * This should be after check errors which poplulate pdu_fcall.
525 */
526 trace_9p_protocol_dump(c, &req->rc);
527 if (err) {
528 p9_debug(P9_DEBUG_ERROR, "couldn't parse header %d\n", err);
529 return err;
530 }
531 if (type != P9_RERROR && type != P9_RLERROR)
532 return 0;
533
534 if (!p9_is_proto_dotl(c)) {
535 char *ename;
536
537 err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
538 &ename, &ecode);
539 if (err)
540 goto out_err;
541
542 if (p9_is_proto_dotu(c) && ecode < 512)
543 err = -ecode;
544
545 if (!err) {
546 err = p9_errstr2errno(ename, strlen(ename));
547
548 p9_debug(P9_DEBUG_9P, "<<< RERROR (%d) %s\n",
549 -ecode, ename);
550 }
551 kfree(ename);
552 } else {
553 err = p9pdu_readf(&req->rc, c->proto_version, "d", &ecode);
554 if (err)
555 goto out_err;
556 err = -ecode;
557
558 p9_debug(P9_DEBUG_9P, "<<< RLERROR (%d)\n", -ecode);
559 }
560
561 return err;
562
563 out_err:
564 p9_debug(P9_DEBUG_ERROR, "couldn't parse error%d\n", err);
565
566 return err;
567 }
568
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tuesday, November 22, 2022 1:21:43 AM CET Dominique Martinet wrote:
> Christian Schoenebeck wrote on Tue, Nov 22, 2022 at 12:04:08AM +0100:
> > Since 60ece0833b6c (net/9p: allocate appropriate reduced message buffers)
> > it is no longer appropriate to check server's response size against
> > msize. Check against the previously allocated buffer capacity instead.
>
> Thanks for the follow up!
>
> > - Omit this size check entirely for zero-copy messages, as those always
> > allocate 4k (P9_ZC_HDR_SZ) linear buffers which are not used for actual
> > payload and can be much bigger than 4k.
>
> [review includes the new flag patch]
>
> hmm, unless there's anywhere else you think we might use these flags it
> looks simpler to just pass a flag to p9_check_errors?
For now that would do as well of course. I just had a feeling that this might
be used for other purposes as well in future and some of these functions are
already somewhat overloaded with arguments.
No strong opinion, your choice.
> In particular adding a bool in this position is not particularly efficient:
> -------(pahole)-----
> struct p9_fcall {
> u32 size; /* 0 4 */
> u8 id; /* 4 1 */
>
> /* XXX 1 byte hole, try to pack */
>
> u16 tag; /* 6 2 */
> size_t offset; /* 8 8 */
> size_t capacity; /* 16 8 */
> bool zc; /* 24 1 */
>
> /* XXX 7 bytes hole, try to pack */
>
> struct kmem_cache * cache; /* 32 8 */
> u8 * sdata; /* 40 8 */
>
> /* size: 48, cachelines: 1, members: 8 */
> /* sum members: 40, holes: 2, sum holes: 8 */
> /* last cacheline: 48 bytes */
> };
> ----------------
> Not that adding it between id and tag sounds better to me, so this is
> probably just as good as anywhere else :-D
Yeah, that layout optimization would make sense indeed.
> Anyway, I'm just nitpicking -- on principle I agree just whitelisting zc
> requests from this check makes most sense, happy with either way if you
> think this is better for the future.
>
> > - Replace p9_debug() by pr_err() to make sure this message is always
> > printed in case this error is triggered.
> >
> > - Add 9p message type to error message to ease investigation.
>
> Yes to these log changes!
>
> >
> > Signed-off-by: Christian Schoenebeck <[email protected]>
> > ---
> > net/9p/client.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/9p/client.c b/net/9p/client.c
> > index 30dd82f49b28..63f13dd1ecff 100644
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> > @@ -514,10 +514,10 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
> > int ecode;
> >
> > err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
> > - if (req->rc.size >= c->msize) {
> > - p9_debug(P9_DEBUG_ERROR,
> > - "requested packet size too big: %d\n",
> > - req->rc.size);
> > + if (req->rc.size > req->rc.capacity && !req->rc.zc) {
> > + pr_err(
> > + "requested packet size too big: %d does not fit %ld (type=%d)\n",
> > + req->rc.size, req->rc.capacity, req->rc.id);
>
> Haven't seen this style before -- is that what qemu uses?
> We normally keep the message on first line and align e.g.
Lazy me, I haven't run checkpatch.pl this time. I'll fix that.
I also have to fix the format specifier for `capacity` that kernel test bot
barked on.
> > + pr_err("requested packet size too big: %d does not fit %ld (type=%d)\n",
> > + req->rc.size, req->rc.capacity, req->rc.id);
>
> (at least what's what other grep -A 1 'pr_err.*,$' seem to do, and
> checkpatch is happier with that)