From: Hillf Danton <hdanton@sina.com>
To: Suren Baghdasaryan <surenb@google.com>
Cc: "Todd Kjos" <tkjos@google.com>,
"Michal Hocko" <mhocko@kernel.org>,
"Hridya Valsaraju" <hridya@google.com>,
"Hillf Danton" <hdanton@sina.com>,
"Eric Biggers" <ebiggers@kernel.org>,
syzbot <syzbot+7a0d9d0b26efefe61780@syzkaller.appspotmail.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Christian Brauner" <christian@brauner.io>,
"open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Hugh Dickins" <hughd@google.com>,
"Joel Fernandes (Google)" <joel@joelfernandes.org>,
LKML <linux-kernel@vger.kernel.org>,
Linux-MM <linux-mm@kvack.org>,
"Martijn Coenen" <maco@android.com>,
syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
"Todd Kjos" <tkjos@android.com>,
"Markus Elfring" <Markus.Elfring@web.de>
Subject: Re: possible deadlock in shmem_fallocate (4)
Date: Wed, 15 Jul 2020 11:52:56 +0800 [thread overview]
Message-ID: <20200715035256.13628-1-hdanton@sina.com> (raw)
In-Reply-To: <CAJuCfpFz9kEfTPxcausVj63mUvU7i6Dvv6=KNePVX2qR+-Ci2A@mail.gmail.com>
On Tue, 14 Jul 2020 10:32:20 -0700 Suren Baghdasaryan wrote:
> On Tue, Jul 14, 2020 at 9:41 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 8:47 AM Todd Kjos <tkjos@google.com> wrote:
> > >
> > > +Suren Baghdasaryan +Hridya Valsaraju who support the ashmem driver.
> >
> > Thanks for looping me in.
> >
> > > On Tue, Jul 14, 2020 at 7:18 AM Michal Hocko <mhocko@kernel.org> wrote:
> > > >
> > > > On Tue 14-07-20 22:08:59, Hillf Danton wrote:
> > > > >
> > > > > On Tue, 14 Jul 2020 10:26:29 +0200 Michal Hocko wrote:
> > > > > > On Tue 14-07-20 13:32:05, Hillf Danton wrote:
> > > > > > >
> > > > > > > On Mon, 13 Jul 2020 20:41:11 -0700 Eric Biggers wrote:
> > > > > > > > On Tue, Jul 14, 2020 at 11:32:52AM +0800, Hillf Danton wrote:
> > > > > > > > >
> > > > > > > > > Add FALLOC_FL_NOBLOCK and on the shmem side try to lock inode upon the
> > > > > > > > > new flag. And the overall upside is to keep the current gfp either in
> > > > > > > > > the khugepaged context or not.
> > > > > > > > >
> > > > > > > > > --- a/include/uapi/linux/falloc.h
> > > > > > > > > +++ b/include/uapi/linux/falloc.h
> > > > > > > > > @@ -77,4 +77,6 @@
> > > > > > > > > */
> > > > > > > > > #define FALLOC_FL_UNSHARE_RANGE 0x40
> > > > > > > > >
> > > > > > > > > +#define FALLOC_FL_NOBLOCK 0x80
> > > > > > > > > +
> > > > > > > >
> > > > > > > > You can't add a new UAPI flag to fix a kernel-internal problem like this.
> > > > > > >
> > > > > > > Sounds fair, see below.
> > > > > > >
> > > > > > > What the report indicates is a missing PF_MEMALLOC_NOFS and it's
> > > > > > > checked on the ashmem side and added as an exception before going
> > > > > > > to filesystem. On shmem side, no more than a best effort is paid
> > > > > > > on the inteded exception.
> > > > > > >
> > > > > > > --- a/drivers/staging/android/ashmem.c
> > > > > > > +++ b/drivers/staging/android/ashmem.c
> > > > > > > @@ -437,6 +437,7 @@ static unsigned long
> > > > > > > ashmem_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
> > > > > > > {
> > > > > > > unsigned long freed = 0;
> > > > > > > + bool nofs;
> > > > > > >
> > > > > > > /* We might recurse into filesystem code, so bail out if necessary */
> > > > > > > if (!(sc->gfp_mask & __GFP_FS))
> > > > > > > @@ -445,6 +446,11 @@ ashmem_shrink_scan(struct shrinker *shri
> > > > > > > if (!mutex_trylock(&ashmem_mutex))
> > > > > > > return -1;
> > > > > > >
> > > > > > > + /* enter filesystem with caution: nonblock on locking */
> > > > > > > + nofs = current->flags & PF_MEMALLOC_NOFS;
> > > > > > > + if (!nofs)
> > > > > > > + current->flags |= PF_MEMALLOC_NOFS;
> > > > > > > +
> > > > > > > while (!list_empty(&ashmem_lru_list)) {
> > > > > > > struct ashmem_range *range =
> > > > > > > list_first_entry(&ashmem_lru_list, typeof(*range), lru);
> > > > > >
> > > > > > I do not think this is an appropriate fix. First of all is this a real
> > > > > > deadlock or a lockdep false positive? Is it possible that ashmem just
> > > > >
> > > > > The warning matters and we can do something to quiesce it.
> > > >
> > > > The underlying issue should be fixed rather than _something_ done to
> > > > silence it.
> > > >
> > > > > > needs to properly annotate its shmem inodes? Or is it possible that
> > > > > > the internal backing shmem file is visible to the userspace so the write
> > > > > > path would be possible?
> > > > > >
> > > > > > If this a real problem then the proper fix would be to set internal
> > > > > > shmem mapping's gfp_mask to drop __GFP_FS.
> > > > >
> > > > > Thanks for the tip, see below.
> > > > >
> > > > > Can you expand a bit on how it helps direct reclaimers like khugepaged
> > > > > in the syzbot report wrt deadlock?
> > > >
> > > > I do not understand your question.
> > > >
> > > > > TBH I have difficult time following
> > > > > up after staring at the chart below for quite a while.
> > > >
> > > > Yes, lockdep reports are quite hard to follow and they tend to confuse
> > > > one hell out of me. But this one says that there is a reclaim dependency
> > > > between the shmem inode lock and the reclaim context.
> > > >
> > > > > Possible unsafe locking scenario:
> > > > >
> > > > > CPU0 CPU1
> > > > > ---- ----
> > > > > lock(fs_reclaim);
> > > > > lock(&sb->s_type->i_mutex_key#15);
> > > > > lock(fs_reclaim);
> > > > >
> > > > > lock(&sb->s_type->i_mutex_key#15);
> > > >
> > > > Please refrain from proposing fixes until the actual problem is
> > > > understood. I suspect that this might be just false positive because the
> > > > lockdep cannot tell the backing shmem which is internal to ashmem(?)
> > > > with any general shmem.
>
> Actually looking some more into this, I think you are right. Ashmem
> currently does not redirect writes into the backing shmem and
> fallocate call from ashmem_shrink_scan is always performed against
> asma->file, which is the backing shmem. IOW writes into the backing
> shmem are not supported, therefore this concurrent locking can't
> happen.
The print of generic_file_write_iter in the syzbot report backs that
concurrency because of f_op::fallocate and another is
Reported-by: syzbot+7a0d9d0b26efefe61780@syzkaller.appspotmail.com
>
> I'm not sure how we can annotate the fact that the inode_lock in
> generic_file_write_iter and in shmem_fallocate always operate on
> different inodes. Ideas?
>
> > > > But somebody really familiar with ashmem code
> > > > should have a look I believe.
> >
> > I believe the deadlock is possible if a write to ashmem fd coincides
> > with shrinking of ashmem caches. I just developed a possible fix here
> > https://android-review.googlesource.com/c/kernel/common/+/1361205 but
> > wanted to test it before posting upstream. The idea is to detect such
> > a race between write and cache shrinking operations and let
> > ashmem_shrink_scan bail out if the race is detected instead of taking
> > inode_lock. AFAIK writing ashmem files is not a usual usage for ashmem
> > (standard usage is to mmap it and use as shared memory), therefore
> > this bailing out early should not affect ashmem cache maintenance
> > much. Besides ashmem_shrink_scan already bails out early if a
> > contention on ashmem_mutex is detected, which is a much more probable
> > case (see: https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/staging/android/ashmem.c#L497).
> >
> > I'll test and post the patch here in a day or so if there are no early
> > objections to it.
> > Thanks!
> >
> > > >
> > > > --
> > > > Michal Hocko
> > > > SUSE Labs
next prev parent reply other threads:[~2020-07-15 3:53 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-26 21:25 possible deadlock in shmem_fallocate (4) syzbot
2020-03-07 21:43 ` [ashmem] " Eric Biggers
2020-03-08 15:05 ` Hillf Danton
2020-07-14 0:32 ` syzbot
2020-07-14 3:32 ` Hillf Danton
2020-07-14 3:41 ` Eric Biggers
2020-07-14 5:32 ` Hillf Danton
2020-07-14 8:26 ` Michal Hocko
2020-07-14 14:08 ` Hillf Danton
2020-07-14 14:18 ` Michal Hocko
2020-07-14 15:46 ` Todd Kjos
2020-07-14 16:41 ` Suren Baghdasaryan
2020-07-14 17:32 ` Suren Baghdasaryan
2020-07-15 3:52 ` Hillf Danton [this message]
2020-07-15 6:36 ` Michal Hocko
2020-07-16 2:49 ` Suren Baghdasaryan
2020-07-14 3:07 ` syzbot
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=20200715035256.13628-1-hdanton@sina.com \
--to=hdanton@sina.com \
--cc=Markus.Elfring@web.de \
--cc=akpm@linux-foundation.org \
--cc=arve@android.com \
--cc=christian@brauner.io \
--cc=devel@driverdev.osuosl.org \
--cc=ebiggers@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hridya@google.com \
--cc=hughd@google.com \
--cc=joel@joelfernandes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=maco@android.com \
--cc=mhocko@kernel.org \
--cc=surenb@google.com \
--cc=syzbot+7a0d9d0b26efefe61780@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=tkjos@android.com \
--cc=tkjos@google.com \
/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 NNTP newsgroup(s).