All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sebastian Ene <sebastianene@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org, kernel-team@android.com,
	stable@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Fix circular locking dependency
Date: Wed, 24 Jan 2024 09:07:33 +0000	[thread overview]
Message-ID: <ZbDTVX1y2bRtJf-G@google.com> (raw)
In-Reply-To: <Za_43qOnVsCPauEr@linux.dev>

On Tue, Jan 23, 2024 at 05:35:26PM +0000, Oliver Upton wrote:
> On Tue, Jan 23, 2024 at 04:48:19PM +0000, Sebastian Ene wrote:
> > The rule inside kvm enforces that the vcpu->mutex is taken *inside*
> > kvm->lock. The rule is violated by the pkvm_create_hyp_vm which acquires

Hi Oliver,

>                                          ^~~~~~~~~~~~~~~~~~
> 
> nit: always suffix function names with '()'
> 
> > the kvm->lock while already holding the vcpu->mutex lock from
> > kvm_vcpu_ioctl. Follow the rule by taking the config lock while getting the
> > VM handle and make sure that this is cleaned on VM destroy under the
> > same lock.
> 
> It is always better to describe a lock in terms of what data it
> protects, the critical section(s) are rather obvious here.
> 
>   Avoid the circular locking dependency altogether by protecting the hyp
>   vm handle with the config_lock, much like we already do for other
>   forms of VM-scoped data.
> 
> > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > Cc: stable@vger.kernel.org
> 
> nitpicks aside, this looks fine.
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 

Thanks for the suggestions, I updated the comit message and I will push
a V2 of the patch with the above and the Reviewed-by tag.

Thanks,
Seb

> -- 
> Thanks,
> Oliver

WARNING: multiple messages have this Message-ID (diff)
From: Sebastian Ene <sebastianene@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org, kernel-team@android.com,
	stable@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Fix circular locking dependency
Date: Wed, 24 Jan 2024 09:07:33 +0000	[thread overview]
Message-ID: <ZbDTVX1y2bRtJf-G@google.com> (raw)
In-Reply-To: <Za_43qOnVsCPauEr@linux.dev>

On Tue, Jan 23, 2024 at 05:35:26PM +0000, Oliver Upton wrote:
> On Tue, Jan 23, 2024 at 04:48:19PM +0000, Sebastian Ene wrote:
> > The rule inside kvm enforces that the vcpu->mutex is taken *inside*
> > kvm->lock. The rule is violated by the pkvm_create_hyp_vm which acquires

Hi Oliver,

>                                          ^~~~~~~~~~~~~~~~~~
> 
> nit: always suffix function names with '()'
> 
> > the kvm->lock while already holding the vcpu->mutex lock from
> > kvm_vcpu_ioctl. Follow the rule by taking the config lock while getting the
> > VM handle and make sure that this is cleaned on VM destroy under the
> > same lock.
> 
> It is always better to describe a lock in terms of what data it
> protects, the critical section(s) are rather obvious here.
> 
>   Avoid the circular locking dependency altogether by protecting the hyp
>   vm handle with the config_lock, much like we already do for other
>   forms of VM-scoped data.
> 
> > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > Cc: stable@vger.kernel.org
> 
> nitpicks aside, this looks fine.
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 

Thanks for the suggestions, I updated the comit message and I will push
a V2 of the patch with the above and the Reviewed-by tag.

Thanks,
Seb

> -- 
> Thanks,
> Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2024-01-24  9:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-23 16:48 [PATCH] KVM: arm64: Fix circular locking dependency Sebastian Ene
2024-01-23 16:48 ` Sebastian Ene
2024-01-23 17:35 ` Oliver Upton
2024-01-23 17:35   ` Oliver Upton
2024-01-24  9:07   ` Sebastian Ene [this message]
2024-01-24  9:07     ` Sebastian Ene

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=ZbDTVX1y2bRtJf-G@google.com \
    --to=sebastianene@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=james.morse@arm.com \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=oliver.upton@linux.dev \
    --cc=stable@vger.kernel.org \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    --cc=yuzenghui@huawei.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.