kmap + memmove

kmap + memmove

[Matthew Wilcox]

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.

Re: kmap + memmove

[Julia Lawall]

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

Re: kmap + memmove

[Ira Weiny]

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

Re: kmap + memmove

[Matthew Wilcox]

On Sun, May 05, 2024 at 03:01:56PM +0200, 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?
> 
> 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:

Great, thanks!  I tried something a little more crude:

$ git grep -A10 kmap_local |grep memmove
fs/erofs/decompressor.c-				memmove(kin + rq->pageofs_out, kin + pi, cur);
fs/erofs/decompressor.c-				memmove(kin + po,
fs/hfs/bnode.c-	memmove(ptr + dst, ptr + src, len);
fs/hfsplus/bnode.c-				memmove(dst_ptr, src_ptr, src);
fs/hfsplus/bnode.c-			memmove(dst_ptr + src, src_ptr + src, len);
fs/hfsplus/bnode.c-			memmove(dst_ptr, src_ptr, l);
fs/hfsplus/bnode.c-				memmove(dst_ptr, src_ptr, l);

$ git grep -A10 kmap_atomic |grep memmove
net/sunrpc/xdr.c-			memmove(vto + pgto_base, vto + pgfrom_base, copy);
net/sunrpc/xdr.c-			memmove(vto + pgto_base, vto + pgfrom_base, copy);

All of these (other than hfsplus) are "false positives" in that you can
see the src/dst are from the same call to kmap_local/kmap_atomic.  Glad
to see there's nothing else.

Re: kmap + memmove

[Matthew Wilcox]

On Sun, May 05, 2024 at 01:25:58PM +0100, 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);

OK, so now we know this is the only place with this problem, how are we
going to fix it?

I think the obvious thing to do is to revert the kmap -> kmap_local
conversion in this function.  The other functions look fine.

Longer term, hfs_bnode_move() makes my eyes bleed.  I really think we
need to do something stupider.  Something like ...

void hfs_bnode_move(struct hfs_bnode *node, int dst, int src, int len)
{
	void *data;
	int first, last;

	if (!len || src == dst)
		return;
	if (src < dst && src + len < dst)
		return hfs_bnode_copy(node, dst, node, src, len);
	if (dst < src && dst + len < src)
		return hfs_bnode_copy(node, dst, node, src, len);

	src += node->page_offset;
	dst += node->page_offset;
	first = min(dst, src) / PAGE_SIZE;
	last = max(dst + len, src + len) / PAGE_SIZE;
	data = vmap_folios(bnode->folios + first, last - first + 1);
	src -= first * PAGE_SIZE;
	dst -= first * PAGE_SIZE;
// maybe an off-by-one in above calculations; check it
	memmove(data + dst, data + src, len);
	vunmap(data);
}

Re: kmap + memmove

[Julia Lawall]

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
>
>
>

Re: kmap + memmove

[Julia Lawall]

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?

I tried the following, which allows the kmap_local_page to be a
subexpression of the right side of the assignment.  It also allows either
order or the ptr initializations.  And it allows the pointer
initializations to be part of some larger statement, such as an if test.

@@
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)

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

*                                src_ptr = <+...kmap_local_page(src_page)...+>
                              ... when any
*                                dst_ptr = <+...kmap_local_page(dst_page)...+>
                              ... when any
*                                memmove(dst_ptr, src_ptr, src)

I only found the occurrences in fs/hfsplus/bnode.c.

Coccinelle furthermore focuses only on the files that contain both memmove
and kmap_local_page.  These are:

HANDLING: /home/julia/linux/drivers/block/zram/zram_drv.c
HANDLING: /home/julia/linux/drivers/infiniband/hw/hfi1/sdma.c
HANDLING: /home/julia/linux/net/sunrpc/xdr.c
HANDLING: /home/julia/linux/kernel/crash_core.c
HANDLING: /home/julia/linux/fs/hfs/bnode.c
HANDLING: /home/julia/linux/fs/udf/inode.c
HANDLING: /home/julia/linux/net/core/skbuff.c
HANDLING: /home/julia/linux/fs/hfsplus/bnode.c
HANDLING: /home/julia/linux/fs/erofs/decompressor.c

One could check them manually to see if memmove and kmap_local_page are
linked in some more direct way.

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
>
>
>

Re: kmap + memmove

[Julia Lawall]

[Fixed a typo at the end of the message and added Dan's proper email
address]

On Mon, 6 May 2024, Julia Lawall wrote:

>
>
> 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?
>
> I tried the following, which allows the kmap_local_page to be a
> subexpression of the right side of the assignment.  It also allows either
> order or the ptr initializations.  And it allows the pointer
> initializations to be part of some larger statement, such as an if test.
>
> @@
> 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)
>
> @@
> expression dst_ptr, src_ptr, dst_page, src_page, src;
> @@
>
> *                                src_ptr = <+...kmap_local_page(src_page)...+>
>                               ... when any
> *                                dst_ptr = <+...kmap_local_page(dst_page)...+>
>                               ... when any
> *                                memmove(dst_ptr, src_ptr, src)
>
> I only found the occurrences in fs/hfsplus/bnode.c.
>
> Coccinelle furthermore focuses only on the files that contain both memmove
> and kmap_local_page.  These are:
>
> HANDLING: /home/julia/linux/drivers/block/zram/zram_drv.c
> HANDLING: /home/julia/linux/drivers/infiniband/hw/hfi1/sdma.c
> HANDLING: /home/julia/linux/net/sunrpc/xdr.c
> HANDLING: /home/julia/linux/kernel/crash_core.c
> HANDLING: /home/julia/linux/fs/hfs/bnode.c
> HANDLING: /home/julia/linux/fs/udf/inode.c
> HANDLING: /home/julia/linux/net/core/skbuff.c
> HANDLING: /home/julia/linux/fs/hfsplus/bnode.c
> HANDLING: /home/julia/linux/fs/erofs/decompressor.c
>
> One could check them manually to see if memmove and kmap_local_page are
> linked in some more direct way.

This was supposed to be "in some more indirect way".

julia

>
> 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
> >
> >
> >
>

Re: kmap + memmove

[Matthew Wilcox]

On Mon, May 06, 2024 at 05:14:21AM +0100, Matthew Wilcox wrote:
> On Sun, May 05, 2024 at 01:25:58PM +0100, 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);
> 
> OK, so now we know this is the only place with this problem, how are we
> going to fix it?

Fabio?

> I think the obvious thing to do is to revert the kmap -> kmap_local
> conversion in this function.  The other functions look fine.
> 
> Longer term, hfs_bnode_move() makes my eyes bleed.  I really think we
> need to do something stupider.  Something like ...
> 
> void hfs_bnode_move(struct hfs_bnode *node, int dst, int src, int len)
> {
> 	void *data;
> 	int first, last;
> 
> 	if (!len || src == dst)
> 		return;
> 	if (src < dst && src + len < dst)
> 		return hfs_bnode_copy(node, dst, node, src, len);
> 	if (dst < src && dst + len < src)
> 		return hfs_bnode_copy(node, dst, node, src, len);
> 
> 	src += node->page_offset;
> 	dst += node->page_offset;
> 	first = min(dst, src) / PAGE_SIZE;
> 	last = max(dst + len, src + len) / PAGE_SIZE;
> 	data = vmap_folios(bnode->folios + first, last - first + 1);
> 	src -= first * PAGE_SIZE;
> 	dst -= first * PAGE_SIZE;
> // maybe an off-by-one in above calculations; check it
> 	memmove(data + dst, data + src, len);
> 	vunmap(data);
> }
> 

Re: kmap + memmove

[Matthew Wilcox]

On Fri, May 24, 2024 at 08:35:42PM +0100, Matthew Wilcox wrote:
> On Mon, May 06, 2024 at 05:14:21AM +0100, Matthew Wilcox wrote:
> > On Sun, May 05, 2024 at 01:25:58PM +0100, 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);
> > 
> > OK, so now we know this is the only place with this problem, how are we
> > going to fix it?
> 
> Fabio?

Fabio?

> > I think the obvious thing to do is to revert the kmap -> kmap_local
> > conversion in this function.  The other functions look fine.
> > 
> > Longer term, hfs_bnode_move() makes my eyes bleed.  I really think we
> > need to do something stupider.  Something like ...
> > 
> > void hfs_bnode_move(struct hfs_bnode *node, int dst, int src, int len)
> > {
> > 	void *data;
> > 	int first, last;
> > 
> > 	if (!len || src == dst)
> > 		return;
> > 	if (src < dst && src + len < dst)
> > 		return hfs_bnode_copy(node, dst, node, src, len);
> > 	if (dst < src && dst + len < src)
> > 		return hfs_bnode_copy(node, dst, node, src, len);
> > 
> > 	src += node->page_offset;
> > 	dst += node->page_offset;
> > 	first = min(dst, src) / PAGE_SIZE;
> > 	last = max(dst + len, src + len) / PAGE_SIZE;
> > 	data = vmap_folios(bnode->folios + first, last - first + 1);
> > 	src -= first * PAGE_SIZE;
> > 	dst -= first * PAGE_SIZE;
> > // maybe an off-by-one in above calculations; check it
> > 	memmove(data + dst, data + src, len);
> > 	vunmap(data);
> > }
> > 
>