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
