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