lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZsNzzajqBkmuu5Xm@google.com>
Date: Mon, 19 Aug 2024 09:33:17 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, kernel@...labora.com, 
	kvm@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Christian Borntraeger <borntraeger@...ux.ibm.com>, 
	Janosch Frank <frankja@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, 
	Anup Patel <anup@...infault.org>
Subject: Re: [PATCH v2] selftests: kvm: fix mkdir error when building for
 unsupported arch

+KVM arch maintainers

On Mon, Aug 19, 2024, Muhammad Usama Anjum wrote:
> The tests are built on per architecture basis. When unsupported
> architecture is specified, it has no tests and TEST_GEN_PROGS is empty.
> The lib.mk has support for not building anything for such case. But KVM
> makefile doesn't handle such case correctly. It doesn't check if
> TEST_GEN_PROGS is empty or not and try to create directory by mkdir.
> Hence mkdir generates the error.
> 
> mkdir: missing operand
> Try 'mkdir --help' for more information.
> 
> This can be easily fixed by checking if TEST_GEN_PROGS isn't empty
> before calling mkdir.
> 
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> ---
> Changes since v1:
> - Instead of ignoring error, check TEST_GEN_PROGS's validity first
> ---
>  tools/testing/selftests/kvm/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 48d32c5aa3eb7..9f8ed82ff1d65 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -317,7 +317,9 @@ $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S $(GEN_HDRS)
>  $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c
>  	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@
>  
> +ifneq ($(strip $(TEST_GEN_PROGS)),)
>  $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
> +endif

This just suppresses an error, it doesn't fix the underlying problem.  E.g. there
are other weird side effects, such as an above mkdir creating the $(ARCH) directory
even though it shouldn't exist in the end.

It's also very opaque, e.g. without a comment or the context of the changelog,
I'd have no idea what purpose the above serves.

Rather than bury the effective "is this arch supported" check in the middle of
the Makefile, what if we wrap the "real" makefile and include it only for
supported architectures, and provide dummy targets for everything else?

E.g.

---
# SPDX-License-Identifier: GPL-2.0-only
top_srcdir = ../../../..
include $(top_srcdir)/scripts/subarch.include
ARCH            ?= $(SUBARCH)

ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64))
ifeq ($(ARCH),x86)
        ARCH_DIR := x86_64
else ifeq ($(ARCH),arm64)
        ARCH_DIR := aarch64
else ifeq ($(ARCH),s390)
        ARCH_DIR := s390x
else
        ARCH_DIR := $(ARCH)
endif

include Makefile.kvm
else
all:
clean:
endif
---

And other KVM maintainers, the big question is: if we do the above, would now be
a decent time to bite the bullet and switch to the kernel's canonical arch paths,
i.e. arm64, s390, and x86?  I feel like if we're ever going to get away from
using aarch64, x86_64, and s390x, this is as about a good of an opportunity as
we're going to get.

The annoying x86_64=>x86 alias still needs to be handled to avoid breaking explicit
ARCH=x86_64 builds (which apparently are allowed, *sigh*), but we can ditch ARCH_DIR
and the KVM selftests dirs match tools' include paths.

---
# SPDX-License-Identifier: GPL-2.0-only
top_srcdir = ../../../..
include $(top_srcdir)/scripts/subarch.include
ARCH            ?= $(SUBARCH)

ifeq ($(ARCH),$(filter $(ARCH),arm64 s390 riscv x86 x86_64))
# Top-level selftests allows ARCH=x86_64 :-(
ifeq ($(ARCH),x86_64)
	ARCH := x86
endif
include Makefile.kvm
else
all:
clean:
endif
---

If no one objects or has a better idea, I'll post a series to do the above.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ