Re: drivers/block/virtio_blk.c:570:68: warning: '%d' directive output may be truncated writing between 1 and 11 bytes into a region of size 7

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6309 bytes --]

On Mon, Dec 04, 2023 at 04:02:07AM -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 04, 2023 at 04:56:35PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   33cc938e65a98f1d29d0a18403dbbee050dcad9a
> > commit: 4e0400525691d0e676dbe002641f9a61261f1e1b virtio-blk: support polling I/O
> > date:   1 year, 6 months ago
> > config: x86_64-buildonly-randconfig-006-20230906 (https://download.01.org/0day-ci/archive/20231204/202312041509.DIyvEt9h-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231204/202312041509.DIyvEt9h-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312041509.DIyvEt9h-lkp@intel.com/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    drivers/block/virtio_blk.c: In function 'init_vq':
> > >> drivers/block/virtio_blk.c:570:68: warning: '%d' directive output may be truncated writing between 1 and 11 bytes into a region of size 7 [-Wformat-truncation=]
> >      570 |                 snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req_poll.%d", i);
> >          |                                                                    ^~
> >    drivers/block/virtio_blk.c:570:58: note: directive argument in the range [-2147483648, 65534]
> >      570 |                 snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req_poll.%d", i);
> >          |                                                          ^~~~~~~~~~~~~
> >    drivers/block/virtio_blk.c:570:17: note: 'snprintf' output between 11 and 21 bytes into a destination of size 16
> >      570 |                 snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req_poll.%d", i);
> >          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > 
> > vim +570 drivers/block/virtio_blk.c
> > 
> >    511	
> >    512	static int init_vq(struct virtio_blk *vblk)
> >    513	{
> >    514		int err;
> >    515		int i;
> >    516		vq_callback_t **callbacks;
> >    517		const char **names;
> >    518		struct virtqueue **vqs;
> >    519		unsigned short num_vqs;
> >    520		unsigned int num_poll_vqs;
> >    521		struct virtio_device *vdev = vblk->vdev;
> >    522		struct irq_affinity desc = { 0, };
> >    523	
> >    524		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
> >    525					   struct virtio_blk_config, num_queues,
> >    526					   &num_vqs);
> >    527		if (err)
> >    528			num_vqs = 1;
> >    529	
> >    530		if (!err && !num_vqs) {
> >    531			dev_err(&vdev->dev, "MQ advertised but zero queues reported\n");
> >    532			return -EINVAL;
> >    533		}
> >    534	
> >    535		num_vqs = min_t(unsigned int,
> >    536				min_not_zero(num_request_queues, nr_cpu_ids),
> >    537				num_vqs);
> >    538	
> >    539		num_poll_vqs = min_t(unsigned int, poll_queues, num_vqs - 1);
> >    540	
> >    541		vblk->io_queues[HCTX_TYPE_DEFAULT] = num_vqs - num_poll_vqs;
> >    542		vblk->io_queues[HCTX_TYPE_READ] = 0;
> >    543		vblk->io_queues[HCTX_TYPE_POLL] = num_poll_vqs;
> >    544	
> >    545		dev_info(&vdev->dev, "%d/%d/%d default/read/poll queues\n",
> >    546					vblk->io_queues[HCTX_TYPE_DEFAULT],
> >    547					vblk->io_queues[HCTX_TYPE_READ],
> >    548					vblk->io_queues[HCTX_TYPE_POLL]);
> >    549	
> >    550		vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
> >    551		if (!vblk->vqs)
> >    552			return -ENOMEM;
> >    553	
> >    554		names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL);
> >    555		callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL);
> >    556		vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL);
> >    557		if (!names || !callbacks || !vqs) {
> >    558			err = -ENOMEM;
> >    559			goto out;
> >    560		}
> >    561	
> >    562		for (i = 0; i < num_vqs - num_poll_vqs; i++) {
> >    563			callbacks[i] = virtblk_done;
> >    564			snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req.%d", i);
> >    565			names[i] = vblk->vqs[i].name;
> >    566		}
> >    567	
> >    568		for (; i < num_vqs; i++) {
> >    569			callbacks[i] = NULL;
> >  > 570			snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req_poll.%d", i);
> >    571			names[i] = vblk->vqs[i].name;
> >    572		}
> >    573	
> >    574		/* Discover virtqueues and write information to configuration.  */
> >    575		err = virtio_find_vqs(vdev, num_vqs, vqs, callbacks, names, &desc);
> >    576		if (err)
> >    577			goto out;
> >    578	
> >    579		for (i = 0; i < num_vqs; i++) {
> >    580			spin_lock_init(&vblk->vqs[i].lock);
> >    581			vblk->vqs[i].vq = vqs[i];
> >    582		}
> >    583		vblk->num_vqs = num_vqs;
> >    584	
> >    585	out:
> >    586		kfree(vqs);
> >    587		kfree(callbacks);
> >    588		kfree(names);
> >    589		if (err)
> >    590			kfree(vblk->vqs);
> >    591		return err;
> >    592	}
> >    593	
> > 
> > -- 
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> 
> Stefan, Paolo,
> It's a false positive but do we want to fix it? Make i unsigned?

It's a false positive:

That maximum number of virtqueues is 65535. That's 5 characters. The
maximum value of num_poll_vqs is actually 65534 because it's capped at
num_vqs - 1.

The warning reveals that the analysis thinks i can be negative:

  drivers/block/virtio_blk.c:570:58: note: directive argument in the range [-2147483648, 65534]

It can't because of num_poll_vqs = min_t(unsigned int, poll_queues,
num_vqs - 1).

VQ_NAME_LEN is 16, so 9 bytes are used for "req_poll." and the NUL
terminator:

  snprintf(vblk->vqs[i].name, VQ_NAME_LEN, "req_poll.%d", i);

9 + 5 = 14 is always less than VQ_NAME_LEN (16), so it looks like a
false positive.

That said, it's still worth cleaning up the types because they are
inconsistent:

  unsigned short num_vqs;
  unsigned int num_poll_vqs;

and using "%d" for an unsigned value also contributes to the problem.

I'll send a patch.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]