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