On Sun, 5 May 2024, Ira Weiny wrote:
> 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?
Yes. Will try it.
julia
>
> 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
>
>
>
