[2/2] net/9p: fix response size check in p9_check_errors()

  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 <linux_oss@crudebyte.com>
---
 net/9p/client.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

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 <linux_oss@crudebyte.com>
> ---
>  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)
  

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 <lkp@intel.com>

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
  

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 <linux_oss@crudebyte.com>
> > ---
> >  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)