Julia Lawall wrote:
>
>
> 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);
>
I'm no expert but this did not catch all theplaces there might be a
problem.
hfsplus/bnode.c: hfs_bnode_move() also does:
216 dst_ptr = kmap_local_page(*dst_page) + dst;
217 src_ptr = kmap_local_page(*src_page) + src;
...
228 memmove(dst_ptr - l, src_ptr - l, l);
...
247 dst_ptr = kmap_local_page(*dst_page) + src;
248 src_ptr = kmap_local_page(*src_page) + src;
249 memmove(dst_ptr, src_ptr, l);
...
265 dst_ptr = kmap_local_page(*dst_page) + dst;
266 src_ptr = kmap_local_page(*src_page) + src;
...
278 memmove(dst_ptr, src_ptr, l);
Can you wildcard the pointer arithmetic?
Ira
> @@ -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
