DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation
@ 2020-07-16  2:45 Suren Baghdasaryan
  2020-07-16  3:30 ` Eric Biggers
  2020-07-30  3:23 ` Joel Fernandes
  0 siblings, 2 replies; 5+ messages in thread
From: Suren Baghdasaryan @ 2020-07-16  2:45 UTC (permalink / raw)
  To: surenb
  Cc: devel, hdanton, kernel-team, tkjos, linux-mm, gregkh,
	linux-kernel, mhocko, ebiggers, hridya, arve, joel, maco,
	christian

syzbot report [1] describes a deadlock when write operation against an
ashmem fd executed at the time when ashmem is shrinking its cache results
in the following lock sequence:

Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(fs_reclaim);
                                lock(&sb->s_type->i_mutex_key#13);
                                lock(fs_reclaim);
   lock(&sb->s_type->i_mutex_key#13);

kswapd takes fs_reclaim and then inode_lock while generic_perform_write
takes inode_lock and then fs_reclaim. However ashmem does not support
writing into backing shmem with a write syscall. The only way to change
its content is to mmap it and operate on mapped memory. Therefore the race
that lockdep is warning about is not valid. Resolve this by introducing a
separate lockdep class for the backing shmem inodes.

[1]: https://lkml.kernel.org/lkml/0000000000000b5f9d059aa2037f@google.com/

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 drivers/staging/android/ashmem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index c05a214191da..10b4be1f3e78 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -95,6 +95,15 @@ static DEFINE_MUTEX(ashmem_mutex);
 static struct kmem_cache *ashmem_area_cachep __read_mostly;
 static struct kmem_cache *ashmem_range_cachep __read_mostly;
 
+/*
+ * A separate lockdep class for the backing shmem inodes to resolve the lockdep
+ * warning about the race between kswapd taking fs_reclaim before inode_lock
+ * and write syscall taking inode_lock and then fs_reclaim.
+ * Note that such race is impossible because ashmem does not support write
+ * syscalls operating on the backing shmem.
+ */
+static struct lock_class_key backing_shmem_inode_class;
+
 static inline unsigned long range_size(struct ashmem_range *range)
 {
 	return range->pgend - range->pgstart + 1;
@@ -396,6 +405,7 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 	if (!asma->file) {
 		char *name = ASHMEM_NAME_DEF;
 		struct file *vmfile;
+		struct inode *inode;
 
 		if (asma->name[ASHMEM_NAME_PREFIX_LEN] != '\0')
 			name = asma->name;
@@ -407,6 +417,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 			goto out;
 		}
 		vmfile->f_mode |= FMODE_LSEEK;
+		inode = file_inode(vmfile);
+		lockdep_set_class(&inode->i_rwsem, &backing_shmem_inode_class);
 		asma->file = vmfile;
 		/*
 		 * override mmap operation of the vmfile so that it can't be
-- 
2.28.0.rc0.105.gf9edc3c819-goog

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation
  2020-07-16  2:45 [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation Suren Baghdasaryan
@ 2020-07-16  3:30 ` Eric Biggers
  2020-07-16 19:30   ` Suren Baghdasaryan
  2020-07-30  3:23 ` Joel Fernandes
  1 sibling, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2020-07-16  3:30 UTC (permalink / raw)
  To: Suren Baghdasaryan
  Cc: devel, hdanton, kernel-team, tkjos, linux-mm, gregkh,
	linux-kernel, mhocko, hridya, arve, joel, maco, christian

On Wed, Jul 15, 2020 at 07:45:27PM -0700, Suren Baghdasaryan wrote:
> syzbot report [1] describes a deadlock when write operation against an
> ashmem fd executed at the time when ashmem is shrinking its cache results
> in the following lock sequence:
> 
> Possible unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(fs_reclaim);
>                                 lock(&sb->s_type->i_mutex_key#13);
>                                 lock(fs_reclaim);
>    lock(&sb->s_type->i_mutex_key#13);
> 
> kswapd takes fs_reclaim and then inode_lock while generic_perform_write
> takes inode_lock and then fs_reclaim. However ashmem does not support
> writing into backing shmem with a write syscall. The only way to change
> its content is to mmap it and operate on mapped memory. Therefore the race
> that lockdep is warning about is not valid. Resolve this by introducing a
> separate lockdep class for the backing shmem inodes.
> 
> [1]: https://lkml.kernel.org/lkml/0000000000000b5f9d059aa2037f@google.com/
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Please add proper tags:

Reported-by: syzbot+7a0d9d0b26efefe61780@syzkaller.appspotmail.com
Fixes: ...
Cc: stable@vger.kernel.org


The Reported-by tag to use was given in the original syzbot report.

- Eric
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation
  2020-07-16  3:30 ` Eric Biggers
@ 2020-07-16 19:30   ` Suren Baghdasaryan
  0 siblings, 0 replies; 5+ messages in thread
From: Suren Baghdasaryan @ 2020-07-16 19:30 UTC (permalink / raw)
  To: Eric Biggers
  Cc: open list:ANDROID DRIVERS, Hillf Danton, kernel-team, Todd Kjos,
	linux-mm, Greg Kroah-Hartman, LKML, Michal Hocko,
	Hridya Valsaraju, Arve Hjønnevåg, Joel Fernandes,
	Martijn Coenen, Christian Brauner

On Wed, Jul 15, 2020 at 8:30 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jul 15, 2020 at 07:45:27PM -0700, Suren Baghdasaryan wrote:
> > syzbot report [1] describes a deadlock when write operation against an
> > ashmem fd executed at the time when ashmem is shrinking its cache results
> > in the following lock sequence:
> >
> > Possible unsafe locking scenario:
> >
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(fs_reclaim);
> >                                 lock(&sb->s_type->i_mutex_key#13);
> >                                 lock(fs_reclaim);
> >    lock(&sb->s_type->i_mutex_key#13);
> >
> > kswapd takes fs_reclaim and then inode_lock while generic_perform_write
> > takes inode_lock and then fs_reclaim. However ashmem does not support
> > writing into backing shmem with a write syscall. The only way to change
> > its content is to mmap it and operate on mapped memory. Therefore the race
> > that lockdep is warning about is not valid. Resolve this by introducing a
> > separate lockdep class for the backing shmem inodes.
> >
> > [1]: https://lkml.kernel.org/lkml/0000000000000b5f9d059aa2037f@google.com/
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> Please add proper tags:
>
> Reported-by: syzbot+7a0d9d0b26efefe61780@syzkaller.appspotmail.com
> Fixes: ...
> Cc: stable@vger.kernel.org
>
>
> The Reported-by tag to use was given in the original syzbot report.

Will add in v2. Thanks!

>
> - Eric
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation
  2020-07-16  2:45 [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation Suren Baghdasaryan
  2020-07-16  3:30 ` Eric Biggers
@ 2020-07-30  3:23 ` Joel Fernandes
  2020-07-30 19:15   ` Suren Baghdasaryan
  1 sibling, 1 reply; 5+ messages in thread
From: Joel Fernandes @ 2020-07-30  3:23 UTC (permalink / raw)
  To: Suren Baghdasaryan
  Cc: open list:ANDROID DRIVERS, hdanton, kernel-team, Todd Kjos,
	Greg Kroah-Hartman, LKML, Michal Hocko, ebiggers, linux-mm,
	Arve Hjønnevåg, Hridya Valsaraju, Martijn Coenen,
	Christian Brauner

On Wed, Jul 15, 2020 at 10:45 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> syzbot report [1] describes a deadlock when write operation against an
> ashmem fd executed at the time when ashmem is shrinking its cache results
> in the following lock sequence:
>
> Possible unsafe locking scenario:
>
>         CPU0                    CPU1
>         ----                    ----
>    lock(fs_reclaim);
>                                 lock(&sb->s_type->i_mutex_key#13);
>                                 lock(fs_reclaim);
>    lock(&sb->s_type->i_mutex_key#13);
>
> kswapd takes fs_reclaim and then inode_lock while generic_perform_write
> takes inode_lock and then fs_reclaim. However ashmem does not support
> writing into backing shmem with a write syscall. The only way to change
> its content is to mmap it and operate on mapped memory. Therefore the race
> that lockdep is warning about is not valid. Resolve this by introducing a
> separate lockdep class for the backing shmem inodes.
>
> [1]: https://lkml.kernel.org/lkml/0000000000000b5f9d059aa2037f@google.com/
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---

Once Eric's nits are resolved:

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation
  2020-07-30  3:23 ` Joel Fernandes
@ 2020-07-30 19:15   ` Suren Baghdasaryan
  0 siblings, 0 replies; 5+ messages in thread
From: Suren Baghdasaryan @ 2020-07-30 19:15 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: open list:ANDROID DRIVERS, Hillf Danton, kernel-team, Todd Kjos,
	Greg Kroah-Hartman, LKML, Michal Hocko, Eric Biggers, linux-mm,
	Arve Hjønnevåg, Hridya Valsaraju, Martijn Coenen,
	Christian Brauner

On Wed, Jul 29, 2020 at 8:24 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Wed, Jul 15, 2020 at 10:45 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > syzbot report [1] describes a deadlock when write operation against an
> > ashmem fd executed at the time when ashmem is shrinking its cache results
> > in the following lock sequence:
> >
> > Possible unsafe locking scenario:
> >
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(fs_reclaim);
> >                                 lock(&sb->s_type->i_mutex_key#13);
> >                                 lock(fs_reclaim);
> >    lock(&sb->s_type->i_mutex_key#13);
> >
> > kswapd takes fs_reclaim and then inode_lock while generic_perform_write
> > takes inode_lock and then fs_reclaim. However ashmem does not support
> > writing into backing shmem with a write syscall. The only way to change
> > its content is to mmap it and operate on mapped memory. Therefore the race
> > that lockdep is warning about is not valid. Resolve this by introducing a
> > separate lockdep class for the backing shmem inodes.
> >
> > [1]: https://lkml.kernel.org/lkml/0000000000000b5f9d059aa2037f@google.com/
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
>
> Once Eric's nits are resolved:
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks Joel!
I'm fixing the nits and will report the patch shortly. One note about
adding the "Fixes: " tag - this is a fix for a false positive lockdep
warning and it's unclear which patch should be quoted here (I could
not find a clear cause that started this warning). In similar
situations, for example here: https://lkml.org/lkml/2020/6/15/958
developers seem to skip that tag. So I'll do the same.

>
> Thanks.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16  2:45 [PATCH 1/1] staging: android: ashmem: Fix lockdep warning for write operation Suren Baghdasaryan
2020-07-16  3:30 ` Eric Biggers
2020-07-16 19:30   ` Suren Baghdasaryan
2020-07-30  3:23 ` Joel Fernandes
2020-07-30 19:15   ` Suren Baghdasaryan

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git