Re: kmap + memmove

[Ira Weiny <ira.weiny@intel.com>]

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