From: Julia Lawall <julia.lawall@inria.fr>
To: Julia Lawall <julia.lawall@inria.fr>
Cc: Ira Weiny <ira.weiny@intel.com>,
Matthew Wilcox <willy@infradead.org>,
"Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
Dan Carpenter <dan.carpenter@linaro.org>,
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:50:18 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2405060749120.3799@hadrien> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2405060743420.3799@hadrien>
[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
> >
> >
> >
>
next prev parent reply other threads:[~2024-05-06 5:50 UTC|newest]
Thread overview: 8+ 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
2024-05-06 5:50 ` Julia Lawall [this message]
2024-05-06 3:47 ` Matthew Wilcox
2024-05-06 4:14 ` 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.2405060749120.3799@hadrien \
--to=julia.lawall@inria.fr \
--cc=akpm@linux-foundation.org \
--cc=bvanassche@acm.org \
--cc=dan.carpenter@linaro.org \
--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).