Hi Dan,

On Feb 1, 2023, at 06:27, Dan Carpenter [off-list ref] wrote:

Hello Trond Myklebust,

The patch a52a8a6adad9: "NFS: Simplify struct nfs_cache_array_entry"
from Nov 1, 2020, leads to the following Smatch static checker
warning:

fs/nfs/dir.c:226 nfs_readdir_clear_array()
warn: uncapped user loop index 'i'

fs/nfs/dir.c
   219 static void nfs_readdir_clear_array(struct page *page)
   220 {
   221         struct nfs_cache_array *array;
   222         unsigned int i;
   223 
   224         array = kmap_atomic(page);
   225         for (i = 0; i < array->size; i++)
--> 226                 kfree(array->array[i].name);

I guess I don't really understand how kmap() works.  I thought it was
for mapping userspace memory into kernel space.  So Smatch marks "array"
as untrusted user controlled data.

How should smatch treat kmap()?

   227         array->size = 0;
   228         kunmap_atomic(array);
   229 }

regards,
dan carpenter

Sorry for the delay. I’m not really the right authority for kmap(), but my understanding is that it is about managing high memory rather than user memory. In other words, in cases where you have a 32-bit PAE kernel that can use more than 4GB of RAM, but only has a 32-bit address space, then the kmap() function, kmap_atomic(), kmap_local() etc allows you to temporarily map the high RAM into your 32-bit address space.

The reason for the above code is that until recently, we were allocating the page cache pages for readdir using GFP_HIGHUSER. We could always change back to using that, but since most CPUs these days are 64-bit, it probably makes more sense to go the other way, and start ripping out the kmap code…

Cheers,
  Trond
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com