RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

["Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>]

> -----Original Message-----
> From: Song Bao Hua (Barry Song)
> Sent: Thursday, November 26, 2020 8:53 PM
> To: 'Miles Chen' <miles.chen@mediatek.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton
> <akpm@linux-foundation.org>; linux-kernel@vger.kernel.org;
> linux-fsdevel@vger.kernel.org; linux-mediatek@lists.infradead.org;
> wsd_upstream@mediatek.com
> Subject: RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read
> addresses
> 
> 
> 
> > -----Original Message-----
> > From: Miles Chen [mailto:miles.chen@mediatek.com]
> > Sent: Thursday, November 26, 2020 8:49 PM
> > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton
> > <akpm@linux-foundation.org>; linux-kernel@vger.kernel.org;
> > linux-fsdevel@vger.kernel.org; linux-mediatek@lists.infradead.org;
> > wsd_upstream@mediatek.com
> > Subject: RE: [RESEND PATCH v1] proc: use untagged_addr() for
> pagemap_read
> > addresses
> >
> > On Thu, 2020-11-26 at 07:16 +0000, Song Bao Hua (Barry Song) wrote:
> > >
> > > > -----Original Message-----
> > > > From: Miles Chen [mailto:miles.chen@mediatek.com]
> > > > Sent: Monday, November 23, 2020 7:39 PM
> > > > To: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton
> > > > <akpm@linux-foundation.org>
> > > > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > > linux-mediatek@lists.infradead.org; wsd_upstream@mediatek.com; Miles
> > > > Chen <miles.chen@mediatek.com>
> > > > Subject: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read
> > > > addresses
> > > >
> > > > When we try to visit the pagemap of a tagged userspace pointer, we find
> > > > that the start_vaddr is not correct because of the tag.
> > > > To fix it, we should untag the usespace pointers in pagemap_read().
> > > >
> > > > I tested with 5.10-rc4 and the issue remains.
> > > >
> > > > My test code is baed on [1]:
> > > >
> > > > A userspace pointer which has been tagged by 0xb4:
> 0xb400007662f541c8
> > > >
> > > > === userspace program ===
> > > >
> > > > uint64 OsLayer::VirtualToPhysical(void *vaddr) {
> > > > 	uint64 frame, paddr, pfnmask, pagemask;
> > > > 	int pagesize = sysconf(_SC_PAGESIZE);
> > > > 	off64_t off = ((uintptr_t)vaddr) / pagesize * 8; // off =
> > > > 0xb400007662f541c8 / pagesize * 8 = 0x5a00003b317aa0
> > > > 	int fd = open(kPagemapPath, O_RDONLY);
> > > > 	...
> > > >
> > > > 	if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) {
> > > > 		int err = errno;
> > > > 		string errtxt = ErrorString(err);
> > > > 		if (fd >= 0)
> > > > 			close(fd);
> > > > 		return 0;
> > > > 	}
> > > > ...
> > > > }
> > > >
> > > > === kernel fs/proc/task_mmu.c ===
> > > >
> > > > static ssize_t pagemap_read(struct file *file, char __user *buf,
> > > > 		size_t count, loff_t *ppos)
> > > > {
> > > > 	...
> > > > 	src = *ppos;
> > > > 	svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54
> > > > 	start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr ==
> > > > 0xb400007662f54000
> > > > 	end_vaddr = mm->task_size;
> > > >
> > > > 	/* watch out for wraparound */
> > > > 	// svpfn == 0xb400007662f54
> > > > 	// (mm->task_size >> PAGE) == 0x8000000
> > > > 	if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true
> because
> > > > of the tag 0xb4
> > > > 		start_vaddr = end_vaddr;
> > > >
> > > > 	ret = 0;
> > > > 	while (count && (start_vaddr < end_vaddr)) { // we cannot visit
> correct
> > > > entry because start_vaddr is set to end_vaddr
> > > > 		int len;
> > > > 		unsigned long end;
> > > > 		...
> > > > 	}
> > > > 	...
> > > > }
> > > >
> > > > [1]
> > > >
> > https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158
> > > >
> > > > Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> > > > ---
> > > >  fs/proc/task_mmu.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > index 217aa2705d5d..e9a70f7ee515 100644
> > > > --- a/fs/proc/task_mmu.c
> > > > +++ b/fs/proc/task_mmu.c
> > > > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file,
> > char
> > > > __user *buf,
> > > >
> > > >  	src = *ppos;
> > > >  	svpfn = src / PM_ENTRY_BYTES;
> > > > -	start_vaddr = svpfn << PAGE_SHIFT;
> > > > +	start_vaddr = untagged_addr(svpfn << PAGE_SHIFT);
> > > >  	end_vaddr = mm->task_size;
> > > >
> > > >  	/* watch out for wraparound */
> > > > -	if (svpfn > mm->task_size >> PAGE_SHIFT)
> > > > +	if (start_vaddr > mm->task_size)
> > > >  		start_vaddr = end_vaddr;
> > >
> > > Wouldn't the untag be done by the user reading pagemap file?
> > > With this patch, even users pass an illegal address, for example,
> > > users put a tag on a virtual address which hasn't really a tag,
> > > they will still get the right pagemap.
> > >
> >
> >
> > I run arm64 devices.
> > If the tagged pointer is enabled, the ARM top-byte Ignore is also
> > enabled. It means the top-byte is always be ignored.
> >
> > I think the kernel should not break existing userspace program, so it's
> > better to handle the tagged user pointer in kernel.
> >
> > grep 'untag' mm -RnH to see existing examples.
> 
> I see your point. But the existing examples such as gup etc are receiving
> an address from userspace.
> 
> pagemap isn't getting an address, it is getting a file offset which
> should be figured out by userspace correctly. While we read a file,
> we should make sure the offset is in the range of the file length.

BTW, personally I don't object to this patch. I just feel a little bit weird
as the parameter in pagemap_read is an offset in one file rather than
a virtual address.

if there are many user programs which are written without the
awareness of tag while reading pagemap or if other people like
this patch, it might make sense to have it in kernel :-)

> 
> >
> >
> > thanks,
> > Miles
> >
> > > >
> > > >  	/*
> > > > --
> > > > 2.18.0
> > >
> 

Thanks
Barry