From: Julia Lawall <julia.lawall@inria.fr>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Julia Lawall <julia.lawall@inria.fr>,
Matthew Wilcox <willy@infradead.org>,
Dan Carpenter <dan.carpenter@oracle.com>,
"Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
Viacheslav Dubeyko <slava@dubeyko.com>,
Andrew Morton <akpm@linux-foundation.org>,
Bart Van Assche <bvanassche@acm.org>,
Kees Cook <keescook@chromium.org>,
linux-fsdevel@vger.kernel.org
Subject: Re: kmap + memmove
Date: Mon, 6 May 2024 07:48:25 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2405060743420.3799@hadrien> (raw)
In-Reply-To: <6638512f36503_25842129471@iweiny-mobl.notmuch>
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
>
>
>
next prev parent reply other threads:[~2024-05-06 5:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-05 12:25 kmap + memmove Matthew Wilcox
2024-05-05 13:01 ` Julia Lawall
2024-05-06 3:40 ` Ira Weiny
2024-05-06 5:15 ` Julia Lawall
2024-05-06 5:48 ` Julia Lawall [this message]
2024-05-06 5:50 ` Julia Lawall
2024-05-06 3:47 ` Matthew Wilcox
2024-05-06 4:14 ` Matthew Wilcox
2024-05-24 19:35 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.22.394.2405060743420.3799@hadrien \
--to=julia.lawall@inria.fr \
--cc=akpm@linux-foundation.org \
--cc=bvanassche@acm.org \
--cc=dan.carpenter@oracle.com \
--cc=fmdefrancesco@gmail.com \
--cc=ira.weiny@intel.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=slava@dubeyko.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).