Re: kmap + memmove

[Matthew Wilcox <willy@infradead.org>]

On Fri, May 24, 2024 at 08:35:42PM +0100, Matthew Wilcox wrote:
> On Mon, May 06, 2024 at 05:14:21AM +0100, Matthew Wilcox wrote:
> > On Sun, May 05, 2024 at 01:25:58PM +0100, Matthew Wilcox wrote:
> > > Here's a fun bug that's not obvious:
> > > 
> > > hfs_bnode_move:
> > >                                 dst_ptr = kmap_local_page(*dst_page);
> > >                                 src_ptr = kmap_local_page(*src_page);
> > >                                 memmove(dst_ptr, src_ptr, src);
> > 
> > OK, so now we know this is the only place with this problem, how are we
> > going to fix it?
> 
> Fabio?

Fabio?

> > I think the obvious thing to do is to revert the kmap -> kmap_local
> > conversion in this function.  The other functions look fine.
> > 
> > Longer term, hfs_bnode_move() makes my eyes bleed.  I really think we
> > need to do something stupider.  Something like ...
> > 
> > void hfs_bnode_move(struct hfs_bnode *node, int dst, int src, int len)
> > {
> > 	void *data;
> > 	int first, last;
> > 
> > 	if (!len || src == dst)
> > 		return;
> > 	if (src < dst && src + len < dst)
> > 		return hfs_bnode_copy(node, dst, node, src, len);
> > 	if (dst < src && dst + len < src)
> > 		return hfs_bnode_copy(node, dst, node, src, len);
> > 
> > 	src += node->page_offset;
> > 	dst += node->page_offset;
> > 	first = min(dst, src) / PAGE_SIZE;
> > 	last = max(dst + len, src + len) / PAGE_SIZE;
> > 	data = vmap_folios(bnode->folios + first, last - first + 1);
> > 	src -= first * PAGE_SIZE;
> > 	dst -= first * PAGE_SIZE;
> > // maybe an off-by-one in above calculations; check it
> > 	memmove(data + dst, data + src, len);
> > 	vunmap(data);
> > }
> > 
>