Re: [PATCH] Revert "NFS: readdirplus optimization by cache mechanism" (memleak)

[Greg KH]

On Fri, Jul 12, 2019 at 04:18:06PM +0200, Max Kellermann wrote:
> This reverts commit be4c2d4723a4a637f0d1b4f7c66447141a4b3564.
> 
> That commit caused a severe memory leak in nfs_readdir_make_qstr().
> 
> When listing a directory with more than 100 files (this is how many
> struct nfs_cache_array_entry elements fit in one 4kB page), all
> allocated file name strings past those 100 leak.
> 
> The root of the leakage is that those string pointers are managed in
> pages which are never linked into the page cache.
> 
> fs/nfs/dir.c puts pages into the page cache by calling
> read_cache_page(); the callback function nfs_readdir_filler() will
> then fill the given page struct which was passed to it, which is
> already linked in the page cache (by do_read_cache_page() calling
> add_to_page_cache_lru()).
> 
> Commit be4c2d4723a4 added another (local) array of allocated pages, to
> be filled with more data, instead of discarding excess items received
> from the NFS server.  Those additional pages can be used by the next
> nfs_readdir_filler() call (from within the same nfs_readdir() call).
> 
> The leak happens when some of those additional pages are never used
> (copied to the page cache using copy_highpage()).  The pages will be
> freed by nfs_readdir_free_pages(), but their contents will not.  The
> commit did not invoke nfs_readdir_clear_array() (and doing so would
> have been dangerous, because it did not track which of those pages
> were already copied to the page cache, risking double free bugs).
> 
> How to reproduce the leak:
> 
> - Use a kernel with CONFIG_SLUB_DEBUG_ON.
> 
> - Create a directory on a NFS mount with more than 100 files with
>   names long enough to use the "kmalloc-32" slab (so we can easily
>   look up the allocation counts):
> 
>   for i in `seq 110`; do touch ${i}_0123456789abcdef; done
> 
> - Drop all caches:
> 
>   echo 3 >/proc/sys/vm/drop_caches
> 
> - Check the allocation counter:
> 
>   grep nfs_readdir /sys/kernel/slab/kmalloc-32/alloc_calls
>   30564391 nfs_readdir_add_to_array+0x73/0xd0 age=534558/4791307/6540952 pid=370-1048386 cpus=0-47 nodes=0-1
> 
> - Request a directory listing and check the allocation counters again:
> 
>   ls
>   [...]
>   grep nfs_readdir /sys/kernel/slab/kmalloc-32/alloc_calls
>   30564511 nfs_readdir_add_to_array+0x73/0xd0 age=207/4792999/6542663 pid=370-1048386 cpus=0-47 nodes=0-1
> 
> There are now 120 new allocations.
> 
> - Drop all caches and check the counters again:
> 
>   echo 3 >/proc/sys/vm/drop_caches
>   grep nfs_readdir /sys/kernel/slab/kmalloc-32/alloc_calls
>   30564401 nfs_readdir_add_to_array+0x73/0xd0 age=735/4793524/6543176 pid=370-1048386 cpus=0-47 nodes=0-1
> 
> 110 allocations are gone, but 10 have leaked and will never be freed.
> 
> Unhelpfully, those allocations are explicitly excluded from KMEMLEAK,
> that's why my initial attempts with KMEMLEAK were not successful:
> 
> 	/*
> 	 * Avoid a kmemleak false positive. The pointer to the name is stored
> 	 * in a page cache page which kmemleak does not scan.
> 	 */
> 	kmemleak_not_leak(string->name);
> 
> It would be possible to solve this bug without reverting the whole
> commit:
> 
> - keep track of which pages were not used, and call
>   nfs_readdir_clear_array() on them, or
> - manually link those pages into the page cache
> 
> But for now I have decided to just revert the commit, because the real
> fix would require complex considerations, risking more dangerous
> (crash) bugs, which may seem unsuitable for the stable branches.
> 
> Signed-off-by: Max Kellermann <mk@xxxxxxxxxx>
> ---
>  fs/nfs/dir.c      | 90 ++++-------------------------------------------
>  fs/nfs/internal.h |  3 +-
>  2 files changed, 7 insertions(+), 86 deletions(-)

No cc: stable@vger on this patch to get it backported?

thanks,

greg k-h