Re: [Virtio-fs] [PATCH] Virtiofs: fix null pointer deference in directIO

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Jul 18, 2019 at 04:39:12PM -0700, Liu Bo wrote:
> On Thu, Jul 18, 2019 at 04:51:10PM -0400, Vivek Goyal wrote:
> > On Thu, Jul 18, 2019 at 11:58:01AM -0700, Liu Bo wrote:
> > > On Thu, Jul 18, 2019 at 02:18:39PM -0400, Vivek Goyal wrote:
> > > > On Fri, Jul 12, 2019 at 06:35:42AM +0800, Liu Bo wrote:
> > > > > Virtiofs has used memremap to create page structs for its managed memory
> > > > > and set pagemap's type to MEMORY_DEVICE_FS_DAX.  Thus, the page from
> > > > > virtiofs's managed memory is a ZONE_DEVICE page, if %devmap_managed_key
> > > > > was enabled by pmem drivers, put_page() will run into
> > > > > __put_devmap_managed_page().  However, as page_free ops is not defined, it
> > > > > ends up with a NULL pointer dereference.
> > > > > 
> > > > > As virtiofs presents its managed memory range as a dax device, this patch
> > > > > makes virtiofs mimic how nvdimm/pmem.c setups pagemap fsdax.
> > > > > 
> > > > > =================
> > > > > 
> > > > > [ 1223.732964] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> > > > > [ 1223.733043] PGD 800000026bd91067 P4D 800000026bd91067 PUD 27631e067 PMD 0
> > > > > [ 1223.733085] Oops: 0010 [#1] SMP PTI
> > > > > [ 1223.733114] CPU: 1 PID: 16505 Comm: build Not tainted 4.19.48-dff9509 #1
> > > > > [ 1223.733157] RIP: 0010:          (null)
> > > > > [ 1223.733179] Code: Bad RIP value.
> > > > > ..
> > > > > [ 1223.733671] Call Trace:
> > > > > [ 1223.733704]  fuse_release_user_pages+0xad/0xb0
> > > > > [ 1223.733751]  fuse_direct_io+0x3ec/0x6a0
> > > > > [ 1223.733823]  fuse_dax_direct_write+0x3c/0x70
> > > > > [ 1223.733863]  fuse_file_write_iter+0x2ea/0x4c0
> > > > > [ 1223.733978]  __vfs_write+0xde/0x160
> > > > > [ 1223.734009]  vfs_write+0xab/0x190
> > > > > [ 1223.734061]  ksys_write+0x45/0xc0
> > > > > [ 1223.734094]  do_syscall_64+0x6b/0x330
> > > > > [ 1223.734214]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > [ 1223.734261] RIP: 0033:0x15536bcddfd0
> > > > > 
> > > > > The reproducer could be as simply as,
> > > > > 
> > > > > addr = mmap(..., fileA, ...);
> > > > > fdB = open(fileB, O_DIRECT);
> > > > > write(fdB, addr);
> > > > > 
> > > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> > > > 
> > > > I am looking at this now and trying to understand it better. One problem
> > > > I see is with page pinning. If a page is pinned, how do we make sure it
> > > > is not evicted by reclaim logic. IIUC, right now memory range relciam
> > > > logic is not aware about page pinning.
> > > > 
> > > > For instance, in above example, write(fdB, addr) will pin pages in fileA.
> > > > While write is going on we need to make sure dax memory ranges covering
> > > > those pages are not reclaimed.
> > > 
> > > Good question, it is indeed a problem, i_mmap_sem is not held by page
> > > pinning so that dax range reclaim is free to go, nor page pinning in
> > > within iomap_begin() and iomap_end() pair.
> > > 
> > > While that is a problem, this patch is fixing a different one because
> > > of the lack of ->page_free definition.
> > 
> > Sure, but in the definition of ->page_free, you are calling
> > "wake_up_var(page->_refcount)". This will make sense only if somebody
> > is waiting on this.
> >
> 
> Right, looks like we also need to follow this patch[1], but looks like
> the fix for the whole problem is still WIP[2].
> 
> > I am checking xfs code and it seems to be waiting for this. We probably
> > will have to modify virtio-fs to fix get_user_pages()/DMA races.
> > 
> > So give me some time to go through and understand what xfs/dax/mm stack
> > is doing for these races and try to fix it properly. 
> 
> Right, I didn't realize the race with dax mapping reclaim.
> 
> So there're two things to fix,
> a) the race between pinned entries/pages and fs fallocate/truncate.
> b) the race between pinned entries/pages and dax ranges background reclaim.

Right.

I think we can take care of both the races by calling
dax_layout_busy_page() in path a) and path b)?

> 
> In terms of a), page->_refcount can be used to avoid the race between
> dma and dax.
> 
> a "dmap->refcnt--" is actually what we need to add for b) at ->page_free.

I am not seeing any callback where we can increment dmap->refcnt. That
probably means we will have to reply on calling dax_layout_busy_page()
in reclaim path. This is expected to be called with inode lock held.

I am thinking of following sequence.

1. inode_trylock(inode)
2. Take i_mmap_sem
3. call dax_layout_busy_page() to make sure no pages on this inode are idle.
4. Take i_dmap_sem
5. Reclaim range

This means that we can't drop inode lock in range reclaim path as your
dmap refcount patches are currently doing.

This probably means that range reclaim might get enen more slower. I 
am thinking that once we get hold of a inode lock and have been
able to do dax_layout_busy_page(), we should reclaim multiple ranges
from same inode (using one FUSE_REMOVEMAPPING()) command, instead
of freeing one range at a time.

And that might provide some speed up with range reclaim.

/me is not 100% sure yet if we need to keep inode lock held when we 
call dax_layout_busy_page(). As of now truncate/punch_hole seems
to hold it. Also we are waiting for pages to become idle, so it
kind of makes sense to not start more read/writes while we are
waiting for pages to become idle.

Thanks
Vivek

> 
> 
> [1]: "mm, fs, dax: handle layout changes to pinned dax mappings"
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5fac7408d828719db6d3fdba63e3c3726a6d1ee5
> 
> [2]: "mm: introduce put_user_page*(), placeholder versions"
> https://patchwork.kernel.org/patch/10637929/
> 
> thanks,
> -liubo