Hi Dan,
> On Feb 1, 2023, at 06:27, Dan Carpenter <error27@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
