Skip to main content

Browse the archive

https://github.com/torvalds/linux
23 October 2021, 00:29:24 UTC
Revision f7d8a19f9a056a05c5c509fa65af472a322abfee authored by Sean Christopherson on 13 October 2021, 00:35:53 UTC, committed by Paolo Bonzini on 18 October 2021, 18:07:18 UTC 
Revert "KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET"
 
Revert a change to open code bits of kvm_lapic_set_base() when emulating
APIC RESET to fix an apic_hw_disabled underflow bug due to arch.apic_base
and apic_hw_disabled being unsyncrhonized when the APIC is created.  If
kvm_arch_vcpu_create() fails after creating the APIC, kvm_free_lapic()
will see the initialized-to-zero vcpu->arch.apic_base and decrement
apic_hw_disabled without KVM ever having incremented apic_hw_disabled.

Using kvm_lapic_set_base() in kvm_lapic_reset() is also desirable for a
potential future where KVM supports RESET outside of vCPU creation, in
which case all the side effects of kvm_lapic_set_base() are needed, e.g.
to handle the transition from x2APIC => xAPIC.

Alternatively, KVM could temporarily increment apic_hw_disabled (and call
kvm_lapic_set_base() at RESET), but that's a waste of cycles and would
impact the performance of other vCPUs and VMs.  The other subtle side
effect is that updating the xAPIC ID needs to be done at RESET regardless
of whether the APIC was previously enabled, i.e. kvm_lapic_reset() needs
an explicit call to kvm_apic_set_xapic_id() regardless of whether or not
kvm_lapic_set_base() also performs the update.  That makes stuffing the
enable bit at vCPU creation slightly more palatable, as doing so affects
only the apic_hw_disabled key.

Opportunistically tweak the comment to explicitly call out the connection
between vcpu->arch.apic_base and apic_hw_disabled, and add a comment to
call out the need to always do kvm_apic_set_xapic_id() at RESET.

Underflow scenario:

  kvm_vm_ioctl() {
    kvm_vm_ioctl_create_vcpu() {
      kvm_arch_vcpu_create() {
        if (something_went_wrong)
          goto fail_free_lapic;
        /* vcpu->arch.apic_base is initialized when something_went_wrong is false. */
        kvm_vcpu_reset() {
          kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) {
            vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
          }
        }
        return 0;
      fail_free_lapic:
        kvm_free_lapic() {
          /* vcpu->arch.apic_base is not yet initialized when something_went_wrong is true. */
          if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE))
            static_branch_slow_dec_deferred(&apic_hw_disabled); // <= underflow bug.
        }
        return r;
      }
    }
  }

This (mostly) reverts commit 421221234ada41b4a9f0beeb08e30b07388bd4bd.

Fixes: 421221234ada ("KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET")
Reported-by: syzbot+9fc046ab2b0cf295a063@syzkaller.appspotmail.com
Debugged-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-Id: <20211013003554.47705-2-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
1 parent baa1e5c
Raw File
Tip revision: f7d8a19f9a056a05c5c509fa65af472a322abfee authored by Sean Christopherson on 13 October 2021, 00:35:53 UTC
Revert "KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET"
Tip revision: f7d8a19
gfp-translate 
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0-only
# Translate the bits making up a GFP mask
# (c) 2009, Mel Gorman <mel@csn.ul.ie>
SOURCE=
GFPMASK=none

# Helper function to report failures and exit
die() {
	echo ERROR: $@
	if [ "$TMPFILE" != "" ]; then
		rm -f $TMPFILE
	fi
	exit -1
}

usage() {
	echo "usage: gfp-translate [-h] [ --source DIRECTORY ] gfpmask"
	exit 0
}

# Parse command-line arguments
while [ $# -gt 0 ]; do
	case $1 in
		--source)
			SOURCE=$2
			shift 2
			;;
		-h)
			usage
			;;
		--help)
			usage
			;;
		*)
			GFPMASK=$1
			shift
			;;
	esac
done

# Guess the kernel source directory if it's not set. Preference is in order of
# o current directory
# o /usr/src/linux
if [ "$SOURCE" = "" ]; then
	if [ -r "/usr/src/linux/Makefile" ]; then
		SOURCE=/usr/src/linux
	fi
	if [ -r "`pwd`/Makefile" ]; then
		SOURCE=`pwd`
	fi
fi

# Confirm that a source directory exists
if [ ! -r "$SOURCE/Makefile" ]; then
	die "Could not locate kernel source directory or it is invalid"
fi

# Confirm that a GFP mask has been specified
if [ "$GFPMASK" = "none" ]; then
	usage
fi

# Extract GFP flags from the kernel source
TMPFILE=`mktemp -t gfptranslate-XXXXXX` || exit 1
grep -q ___GFP $SOURCE/include/linux/gfp.h
if [ $? -eq 0 ]; then
	grep "^#define ___GFP" $SOURCE/include/linux/gfp.h | sed -e 's/u$//' | grep -v GFP_BITS > $TMPFILE
else
	grep "^#define __GFP" $SOURCE/include/linux/gfp.h | sed -e 's/(__force gfp_t)//' | sed -e 's/u)/)/' | grep -v GFP_BITS | sed -e 's/)\//) \//' > $TMPFILE
fi

# Parse the flags
IFS="
"
echo Source: $SOURCE
echo Parsing: $GFPMASK
for LINE in `cat $TMPFILE`; do
	MASK=`echo $LINE | awk '{print $3}'`
	if [ $(($GFPMASK&$MASK)) -ne 0 ]; then
		echo $LINE
	fi
done

rm -f $TMPFILE
exit 0
back to top