re: pnfs/blocklayout: allocate separate pages for the layoutcommit payload

re: pnfs/blocklayout: allocate separate pages for the layoutcommit payload

[Dan Carpenter]

Hello Christoph Hellwig,

This is old, and since no one has complained it's probably not a bug
but suspicious enough that I wanted to double check.

The patch 34dc93c2fc04: "pnfs/blocklayout: allocate separate pages
for the layoutcommit payload" from Sep 10, 2014, leads to the
following static checker warning:

	fs/nfs/blocklayout/extent_tree.c:559 ext_tree_prepare_commit()
	warn: potential pointer math issue ('start_p' is a pointer to 'unsigned int')

fs/nfs/blocklayout/extent_tree.c
   512  int
   513  ext_tree_prepare_commit(struct nfs4_layoutcommit_args *arg)
   514  {
   515          struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout);
   516          size_t count = 0, buffer_size = PAGE_SIZE;
   517          __be32 *start_p;
                ^^^^^^^^^^^^^^^
Declared here.

   518          int ret;
   519  
   520          dprintk("%s enter\n", __func__);
   521  
   522          arg->layoutupdate_page = alloc_page(GFP_NOFS);
   523          if (!arg->layoutupdate_page)
   524                  return -ENOMEM;
   525          start_p = page_address(arg->layoutupdate_page);
   526          arg->layoutupdate_pages = &arg->layoutupdate_page;
   527  
   528  retry:
   529          ret = ext_tree_encode_commit(bl, start_p + 1, buffer_size, &count);
   530          if (unlikely(ret)) {
   531                  ext_tree_free_commitdata(arg, buffer_size);
   532  
   533                  buffer_size = sizeof(__be32) + BL_EXTENT_SIZE * count;
   534                  count = 0;
   535  
   536                  arg->layoutupdate_pages =
   537                          kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE),
   538                                  sizeof(struct page *), GFP_NOFS);
   539                  if (!arg->layoutupdate_pages)
   540                          return -ENOMEM;
   541  
   542                  start_p = __vmalloc(buffer_size, GFP_NOFS, PAGE_KERNEL);
   543                  if (!start_p) {
   544                          kfree(arg->layoutupdate_pages);
   545                          return -ENOMEM;
   546                  }
   547  
   548                  goto retry;
   549          }
   550  
   551          *start_p = cpu_to_be32(count);
   552          arg->layoutupdate_len = sizeof(__be32) + BL_EXTENT_SIZE * count;
                ^^^^^^^^^^^^^^^^^^^^^
->layoutupdate_len is the size in bytes

   553  
   554          if (unlikely(arg->layoutupdate_pages != &arg->layoutupdate_page)) {
   555                  __be32 *p = start_p;
   556                  int i = 0;
   557  
   558                  for (p = start_p;
   559                       p < start_p + arg->layoutupdate_len;
   560                       p += PAGE_SIZE) {
                             ^^^^^^^^^^^^^^
Since "p" is also a pointer to unsigned int, it means we iterate the
correct number of times.

   561                          arg->layoutupdate_pages[i++] = vmalloc_to_page(p);

But it's possible that we are using the wrong value for "p" here.

   562                  }
   563          }
   564  
   565          dprintk("%s found %zu ranges\n", __func__, count);
   566          return 0;
   567  }

regards,
dan carpenter