Should we delete memmove_page?

Should we delete memmove_page?

[Matthew Wilcox]

I was looking at memmove_page() and it occurred to me that it can't
actually work, so we should probably delete it as being an attractive
nuisance.  It doesn't have any users.

memmove() isn't magic.  It compares the source address, source length,
destination address and destination length, and then does either a
forwards copy or a backwards copy, depending where the overlap is.

But 'dst' and 'src' are guaranteed to be non-overlapping.  They come from
different calls to kmap_local_page(), so even if src_page and dst_page
are the same, src and dst will be different.

We could fix it up.  Include a compare of 'dst_page' and 'src_page'
and if they're the same only kmap_local_page() one of them.  But since
it has no users, perhaps just delete it?

Re: Should we delete memmove_page?

[Ira Weiny]

On Thu, May 26, 2022 at 03:48:28PM +0100, Matthew Wilcox wrote:
> 
> I was looking at memmove_page() and it occurred to me that it can't
> actually work,

Oh wow yea.  Because you can't unmap that address correctly.

>
> so we should probably delete it as being an attractive
> nuisance.  It doesn't have any users.
> 
> memmove() isn't magic.  It compares the source address, source length,
> destination address and destination length, and then does either a
> forwards copy or a backwards copy, depending where the overlap is.
> 
> But 'dst' and 'src' are guaranteed to be non-overlapping.  They come from
> different calls to kmap_local_page(), so even if src_page and dst_page
> are the same, src and dst will be different.
> 
> We could fix it up.  Include a compare of 'dst_page' and 'src_page'
> and if they're the same only kmap_local_page() one of them.  But since
> it has no users, perhaps just delete it?

Yes deletion is best.  But...

	copy_user_highpage()
	copy_highpage()

... might suffer from the same potential issue should a user not realize.  I
think memcpy_page() by virtue of the name.

Still perhaps a kernel doc may be in order.

I could not say anything at LSFmm because the Outreachy interns had not been
announced but I've selected Fabio to help with the highmem rework through that
program.

Would you like Fabio or I to send a patch?  I think the main thing right now is
to just drop the memmove_page()

Ira

Re: Should we delete memmove_page?

[Matthew Wilcox]

On Thu, May 26, 2022 at 05:40:44PM -0700, Ira Weiny wrote:
> On Thu, May 26, 2022 at 03:48:28PM +0100, Matthew Wilcox wrote:
> > 
> > I was looking at memmove_page() and it occurred to me that it can't
> > actually work,
> 
> Oh wow yea.  Because you can't unmap that address correctly.

I don't understand what you mean ... you can unmap the address, it's
just that memmove can't know that the two virtual ranges actually
overlap physically.

> Yes deletion is best.  But...
> 
> 	copy_user_highpage()
> 	copy_highpage()
> 
> ... might suffer from the same potential issue should a user not realize.  I
> think memcpy_page() by virtue of the name.

Umm?  They're all using memcpy(), so the caller must guarantee that the
physical addresses are different.

> I could not say anything at LSFmm because the Outreachy interns had not been
> announced but I've selected Fabio to help with the highmem rework through that
> program.

Excellent!  I advised Fabio last year, and I think he'll do a sterling
job this year.

> Would you like Fabio or I to send a patch?  I think the main thing right now is
> to just drop the memmove_page()

Sure, just stick my Reported-by: on it.

Re: Should we delete memmove_page?

[Ira Weiny]

On Fri, May 27, 2022 at 02:26:52AM +0100, Matthew Wilcox wrote:
> On Thu, May 26, 2022 at 05:40:44PM -0700, Ira Weiny wrote:
> > On Thu, May 26, 2022 at 03:48:28PM +0100, Matthew Wilcox wrote:
> > > 
> > > I was looking at memmove_page() and it occurred to me that it can't
> > > actually work,
> > 
> > Oh wow yea.  Because you can't unmap that address correctly.
> 
> I don't understand what you mean ...

Nevermind.  I missed the fact that arch_kmap_local_map_idx() does not actually
use the pfn except on xtensa.

> you can unmap the address, it's
> just that memmove can't know that the two virtual ranges actually
> overlap physically.

I was thinking that the index returned would be the same for both kmaps.

> 
> > Yes deletion is best.  But...
> > 
> > 	copy_user_highpage()
> > 	copy_highpage()
> > 
> > ... might suffer from the same potential issue should a user not realize.  I
> > think memcpy_page() by virtue of the name.
> 
> Umm?  They're all using memcpy(), so the caller must guarantee that the
> physical addresses are different.

Exactly.  But how does the caller know that?  The memcpy() call is buried
inside the copy_user_page() and/or copy_page().  Users of these functions
should not need to dig that far into an implementation to use this interface
IMO.  Adding some kdoc helps I think.

> 
> > I could not say anything at LSFmm because the Outreachy interns had not been
> > announced but I've selected Fabio to help with the highmem rework through that
> > program.
> 
> Excellent!  I advised Fabio last year, and I think he'll do a sterling
> job this year.

Me too!

> 
> > Would you like Fabio or I to send a patch?  I think the main thing right now is
> > to just drop the memmove_page()
> 
> Sure, just stick my Reported-by: on it.

Absolutely!

Thanks for the report,
Ira