Re: kmap + memmove

[Julia Lawall <julia.lawall@inria.fr>]

On Sun, 5 May 2024, 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);
>
> If both of the pointers are guaranteed to come from diffeerent calls to
> kmap_local(), memmove() is probably not going to do what you want.
> Worth a smatch or coccinelle rule?
>
> The only time that memmove() is going to do something different from
> memcpy() is when src and dst overlap.  But if src and dst both come
> from kmap_local(), they're guaranteed to not overlap.  Even if dst_page
> and src_page were the same.
>
> Which means the conversion in 6c3014a67a44 was buggy.  Calling kmap()
> for the same page twice gives you the same address.  Calling kmap_local()
> for the same page twice gives you two different addresses.
>
> Fabio, how many other times did you create this same bug?  Ira, I'm
> surprised you didn't catch this one; you created the same bug in
> memmove_page() which I got Fabio to delete in 9384d79249d0.
>

I tried the following rule:

@@
expression dst_ptr, src_ptr, dst_page, src_page, src;
@@

*                                dst_ptr = kmap_local_page(dst_page);
				... when any
*                                src_ptr = kmap_local_page(src_page);
				... when any
*                                memmove(dst_ptr, src_ptr, src);

That is, basically what you wrote, but with anything in between the lines,
and the various variables being any expression.

I only got the following results, which I guess are what you are already
looking at:

@@ -193,9 +193,6 @@ void hfs_bnode_move(struct hfs_bnode *no

 		if (src == dst) {
 			while (src < len) {
-				dst_ptr = kmap_local_page(*dst_page);
-				src_ptr = kmap_local_page(*src_page);
-				memmove(dst_ptr, src_ptr, src);
 				kunmap_local(src_ptr);
 				set_page_dirty(*dst_page);
 				kunmap_local(dst_ptr);
@@ -253,9 +250,6 @@ void hfs_bnode_move(struct hfs_bnode *no

 			while ((len -= l) != 0) {
 				l = min_t(int, len, PAGE_SIZE);
-				dst_ptr = kmap_local_page(*++dst_page);
-				src_ptr = kmap_local_page(*++src_page);
-				memmove(dst_ptr, src_ptr, l);
 				kunmap_local(src_ptr);
 				set_page_dirty(*dst_page);
 				kunmap_local(dst_ptr);

julia