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:15:04 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2405060714480.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?
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
>
>
>
next prev parent reply other threads:[~2024-05-06 5:15 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 [this message]
2024-05-06 5:48 ` Julia Lawall
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.2405060714480.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).