Re: [PATCH v2 2/9] vgacon: rework screen_info #ifdef checks

[Khalid Aziz]

On 7/19/23 6:39 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

On non-x86 architectures, the screen_info variable is generally only
used for the VGA console where supported, and in some cases the EFI
framebuffer or vga16fb.

Now that we have a definite list of which architectures actually use it
for what, use consistent #ifdef checks so the global variable is only
defined when it is actually used on those architectures.

Loongarch and riscv have no support for vgacon or vga16fb, but
they support EFI firmware, so only that needs to be checked, and the
initialization can be removed because that is handled by EFI.
IA64 has both vgacon and EFI, though EFI apparently never uses
a framebuffer here.

Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
Signed-off-by: Arnd Bergmann 



Reviewed-by: Khalid Aziz 


---
v2 changes:
  - split out mips/jazz change
  - improve ia64 #ifdef changes
---
  arch/alpha/kernel/setup.c  |  2 ++
  arch/alpha/kernel/sys_sio.c|  2 ++
  arch/ia64/kernel/setup.c   |  6 ++
  arch/loongarch/kernel/setup.c  |  2 ++
  arch/mips/kernel/setup.c   |  2 +-
  arch/mips/sibyte/swarm/setup.c |  2 +-
  arch/mips/sni/setup.c  |  2 +-
  arch/riscv/kernel/setup.c  | 11 ++-
  8 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index b650ff1cb022e..b4d2297765c02 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -131,6 +131,7 @@ static void determine_cpu_caches (unsigned int);
  
  static char __initdata command_line[COMMAND_LINE_SIZE];
  
+#ifdef CONFIG_VGA_CONSOLE

  /*
   * The format of "screen_info" is strange, and due to early
   * i386-setup code. This is just enough to make the console
@@ -147,6 +148,7 @@ struct screen_info screen_info = {
  };
  
  EXPORT_SYMBOL(screen_info);

+#endif
  
  /*

   * The direct map I/O window, if any.  This should be the same
diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
index 7c420d8dac53d..7de8a5d2d2066 100644
--- a/arch/alpha/kernel/sys_sio.c
+++ b/arch/alpha/kernel/sys_sio.c
@@ -57,11 +57,13 @@ sio_init_irq(void)
  static inline void __init
  alphabook1_init_arch(void)
  {
+#ifdef CONFIG_VGA_CONSOLE
/* The AlphaBook1 has LCD video fixed at 800x600,
   37 rows and 100 cols. */
screen_info.orig_y = 37;
screen_info.orig_video_cols = 100;
screen_info.orig_video_lines = 37;
+#endif
  
  	lca_init_arch();

  }
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 5a55ac82c13a4..d2c66efdde560 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -86,9 +86,13 @@ EXPORT_SYMBOL(local_per_cpu_offset);
  #endif
  unsigned long ia64_cycles_per_usec;
  struct ia64_boot_param *ia64_boot_param;
+#if defined(CONFIG_VGA_CONSOLE) || defined(CONFIG_EFI)
  struct screen_info screen_info;
+#endif
+#ifdef CONFIG_VGA_CONSOLE
  unsigned long vga_console_iobase;
  unsigned long vga_console_membase;
+#endif
  
  static struct resource data_resource = {

.name   = "Kernel data",
@@ -497,6 +501,7 @@ early_console_setup (char *cmdline)
  static void __init
  screen_info_setup(void)
  {
+#ifdef CONFIG_VGA_CONSOLE
unsigned int orig_x, orig_y, num_cols, num_rows, font_height;
  
  	memset(&screen_info, 0, sizeof(screen_info));

@@ -525,6 +530,7 @@ screen_info_setup(void)
screen_info.orig_video_mode = 3;/* XXX fake */
screen_info.orig_video_isVGA = 1;   /* XXX fake */
screen_info.orig_video_ega_bx = 3;  /* XXX fake */
+#endif
  }
  
  static inline void

diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
index 95e6b579dfdd1..77e7a3722caa6 100644
--- a/arch/loongarch/kernel/setup.c
+++ b/arch/loongarch/kernel/setup.c
@@ -57,7 +57,9 @@
  #define SMBIOS_CORE_PACKAGE_OFFSET0x23
  #define LOONGSON_EFI_ENABLE   (1 << 3)
  
+#ifdef CONFIG_EFI

  struct screen_info screen_info __section(".data");
+#endif
  
  unsigned long fw_arg0, fw_arg1, fw_arg2;

  DEFINE_PER_CPU(unsigned long, kernelsp);
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index cb871eb784a7c..1aba7dc95132c 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -54,7 +54,7 @@ struct cpuinfo_mips cpu_data[NR_CPUS] __read_mostly;
  
  EXPORT_SYMBOL(cpu_data);
  
-#ifdef CONFIG_VT

+#ifdef CONFIG_VGA_CONSOLE
  struct screen_info screen_info;
  #endif
  
diff --git a/arch/mips/sibyte/swarm/setup.c b/arch/mips/sibyte/swarm/setup.c

index 76683993cdd3a..37df504d3ecbb 100644
--- a/arch/mips/sibyte/swarm/setup.c
+++ b/arch/mips/sibyte/swarm/setup.c
@@ -129,7 +129,7 @@ void __init plat_mem_setup(void)
if (m41t81_probe())
swarm_rtc_type = RTC_M41T81;
  
-#ifdef CONFIG_VT

+#ifdef CONFIG_VGA_CONSOLE
screen_info = (struct screen_info) {
.orig_video_page= 52,
.

Re: [PATCH v2 6/9] vgacon: clean up global screen_info instances

[Khalid Aziz]

On 7/19/23 6:39 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

To prepare for completely separating the VGA console screen_info from
the one used in EFI/sysfb, rename the vgacon instances and make them
local as much as possible.

ia64 and arm both have confurations with vgacon and efi, but the contents
never overlaps because ia64 has no EFI framebuffer, and arm only has
vga console on legacy platforms without EFI. Renaming these is required
before the EFI screen_info can be moved into drivers/firmware.

The ia64 vga console is actually registered in two places from
setup_arch(), but one of them is wrong, so drop the one in pcdp.c and
the fix the one in setup.c to use the correct conditional.

x86 has to keep them together, as the boot protocol is used to switch
between VGA text console and framebuffer through the screen_info data.

Signed-off-by: Arnd Bergmann 



PCDP and ia64 changes are reasonable.

Acked-by: Khalid Aziz 


---
  arch/alpha/kernel/proto.h |  2 ++
  arch/alpha/kernel/setup.c |  6 ++--
  arch/alpha/kernel/sys_sio.c   |  6 ++--
  arch/arm/include/asm/setup.h  |  5 
  arch/arm/kernel/atags_parse.c | 18 ++--
  arch/arm/kernel/efi.c |  6 
  arch/arm/kernel/setup.c   | 10 +--
  arch/ia64/kernel/setup.c  | 49 +++
  arch/mips/kernel/setup.c  | 11 ---
  arch/mips/mti-malta/malta-setup.c |  4 ++-
  arch/mips/sibyte/swarm/setup.c| 24 ---
  arch/mips/sni/setup.c | 16 +-
  drivers/firmware/pcdp.c   |  1 -
  13 files changed, 78 insertions(+), 80 deletions(-)

diff --git a/arch/alpha/kernel/proto.h b/arch/alpha/kernel/proto.h
index 5816a31c1b386..2c89c1c557129 100644
--- a/arch/alpha/kernel/proto.h
+++ b/arch/alpha/kernel/proto.h
@@ -1,5 +1,6 @@
  /* SPDX-License-Identifier: GPL-2.0 */
  #include 
+#include 
  #include 
  
  /* Prototypes of functions used across modules here in this directory.  */

@@ -113,6 +114,7 @@ extern int boot_cpuid;
  #ifdef CONFIG_VERBOSE_MCHECK
  extern unsigned long alpha_verbose_mcheck;
  #endif
+extern struct screen_info vgacon_screen_info;
  
  /* srmcons.c */

  #if defined(CONFIG_ALPHA_GENERIC) || defined(CONFIG_ALPHA_SRM)
diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index d73b685fe9852..7b35af2ed2787 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -138,7 +138,7 @@ static char __initdata command_line[COMMAND_LINE_SIZE];
   * code think we're on a VGA color display.
   */
  
-struct screen_info screen_info = {

+struct screen_info vgacon_screen_info = {
.orig_x = 0,
.orig_y = 25,
.orig_video_cols = 80,
@@ -146,8 +146,6 @@ struct screen_info screen_info = {
.orig_video_isVGA = 1,
.orig_video_points = 16
  };
-
-EXPORT_SYMBOL(screen_info);
  #endif
  
  /*

@@ -655,7 +653,7 @@ setup_arch(char **cmdline_p)
  
  #ifdef CONFIG_VT

  #if defined(CONFIG_VGA_CONSOLE)
-   vgacon_register_screen(&screen_info);
+   vgacon_register_screen(&vgacon_screen_info);
  #endif
  #endif
  
diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c

index 7de8a5d2d2066..086488ed83a7f 100644
--- a/arch/alpha/kernel/sys_sio.c
+++ b/arch/alpha/kernel/sys_sio.c
@@ -60,9 +60,9 @@ alphabook1_init_arch(void)
  #ifdef CONFIG_VGA_CONSOLE
/* The AlphaBook1 has LCD video fixed at 800x600,
   37 rows and 100 cols. */
-   screen_info.orig_y = 37;
-   screen_info.orig_video_cols = 100;
-   screen_info.orig_video_lines = 37;
+   vgacon_screen_info.orig_y = 37;
+   vgacon_screen_info.orig_video_cols = 100;
+   vgacon_screen_info.orig_video_lines = 37;
  #endif
  
  	lca_init_arch();

diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
index 546af8b1e3f65..cc106f946c691 100644
--- a/arch/arm/include/asm/setup.h
+++ b/arch/arm/include/asm/setup.h
@@ -11,6 +11,7 @@
  #ifndef __ASMARM_SETUP_H
  #define __ASMARM_SETUP_H
  
+#include 

  #include 
  
  
@@ -35,4 +36,8 @@ void early_mm_init(const struct machine_desc *);

  void adjust_lowmem_bounds(void);
  void setup_dma_zone(const struct machine_desc *desc);
  
+#ifdef CONFIG_VGA_CONSOLE

+extern struct screen_info vgacon_screen_info;
+#endif
+
  #endif
diff --git a/arch/arm/kernel/atags_parse.c b/arch/arm/kernel/atags_parse.c
index 4c815da3b77b0..4ec591bde3dfa 100644
--- a/arch/arm/kernel/atags_parse.c
+++ b/arch/arm/kernel/atags_parse.c
@@ -72,15 +72,15 @@ __tagtable(ATAG_MEM, parse_tag_mem32);
  #if defined(CONFIG_ARCH_FOOTBRIDGE) && defined(CONFIG_VGA_CONSOLE)
  static int __init parse_tag_videotext(const struct tag *tag)
  {
-   screen_info.orig_x= tag->u.videotext.x;
-   screen_info.orig_y= tag->u.videotext.y;
-   screen_info.orig_video_page   = tag->u.videotext.video_page;
-   screen_info.orig_video_mode   = tag->u.videotext.video_mode;
-   sc

Re: [PATCH v2 1/9] vgacon: rework Kconfig dependencies

[Khalid Aziz]

On 7/19/23 6:39 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

The list of dependencies here is phrased as an opt-out, but this is missing
a lot of architectures that don't actually support VGA consoles, and some
of the entries are stale:

  - powerpc used to support VGA consoles in the old arch/ppc codebase, but
the merged arch/powerpc never did

  - arm lists footbridge, integrator and netwinder, but netwinder is actually
part of footbridge, and integrator does not appear to have an actual
VGA hardware, or list it in its ATAG or DT.

  - mips has a few platforms (malta, sibyte, and sni) that initialize
screen_info, on everything else the console is selected but cannot
actually work.

  - csky, hexgagon, loongarch, nios2, riscv and xtensa are not listed
in the opt-out table and declare a screen_info to allow building
vga_con, but this cannot work because the console is never selected.

Replace this with an opt-in table that lists only the platforms that
remain. This is effectively x86, plus a couple of historic workstation
and server machines that reused parts of the x86 system architecture.

Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
Signed-off-by: Arnd Bergmann 



Reviewed-by: Khalid Aziz 



---
  drivers/video/console/Kconfig | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index 1b5a319971ed0..6af90db6d2da9 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -7,9 +7,9 @@ menu "Console display driver support"
  
  config VGA_CONSOLE

bool "VGA text console" if EXPERT || !X86
-   depends on !4xx && !PPC_8xx && !SPARC && !M68K && !PARISC &&  !SUPERH 
&& \
-   (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER) 
&& \
-   !ARM64 && !ARC && !MICROBLAZE && !OPENRISC && !S390 && !UML
+   depends on ALPHA || IA64 || X86 || \
+   (ARM && ARCH_FOOTBRIDGE) || \
+   (MIPS && (MIPS_MALTA || SIBYTE_BCM112X || SIBYTE_SB1250 || 
SIBYTE_BCM1x80 || SNI_RM))
select APERTURE_HELPERS if (DRM || FB || VFIO_PCI_CORE)
default y
help

Re: [PATCH v2 5/9] vgacon: remove screen_info dependency

[Khalid Aziz]

On 7/19/23 6:39 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

The vga console driver is fairly self-contained, and only used by
architectures that explicitly initialize the screen_info settings.

Chance every instance that picks the vga console by setting conswitchp
to call a function instead, and pass a reference to the screen_info
there.

Signed-off-by: Arnd Bergmann 


PCDP and ia64 changes look good to me.

Acked-by: Khalid Azzi 


---
  arch/alpha/kernel/setup.c  |  2 +-
  arch/arm/kernel/setup.c|  2 +-
  arch/ia64/kernel/setup.c   |  2 +-
  arch/mips/kernel/setup.c   |  2 +-
  arch/x86/kernel/setup.c|  2 +-
  drivers/firmware/pcdp.c|  2 +-
  drivers/video/console/vgacon.c | 68 --
  include/linux/console.h|  7 
  8 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index b4d2297765c02..d73b685fe9852 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -655,7 +655,7 @@ setup_arch(char **cmdline_p)
  
  #ifdef CONFIG_VT

  #if defined(CONFIG_VGA_CONSOLE)
-   conswitchp = &vga_con;
+   vgacon_register_screen(&screen_info);
  #endif
  #endif
  
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c

index 40326a35a179b..5d8a7fb3eba45 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -1192,7 +1192,7 @@ void __init setup_arch(char **cmdline_p)
  
  #ifdef CONFIG_VT

  #if defined(CONFIG_VGA_CONSOLE)
-   conswitchp = &vga_con;
+   vgacon_register_screen(&screen_info);
  #endif
  #endif
  
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c

index d2c66efdde560..2c9283fcd3759 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -619,7 +619,7 @@ setup_arch (char **cmdline_p)
 * memory so we can avoid this problem.
 */
if (efi_mem_type(0xA) != EFI_CONVENTIONAL_MEMORY)
-   conswitchp = &vga_con;
+   vgacon_register_screen(&screen_info);
  # endif
}
  #endif
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 1aba7dc95132c..6c3fae62a9f6b 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -794,7 +794,7 @@ void __init setup_arch(char **cmdline_p)
  
  #if defined(CONFIG_VT)

  #if defined(CONFIG_VGA_CONSOLE)
-   conswitchp = &vga_con;
+   vgacon_register_screen(&screen_info);
  #endif
  #endif
  
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c

index fd975a4a52006..b1ea77d504615 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1293,7 +1293,7 @@ void __init setup_arch(char **cmdline_p)
  #ifdef CONFIG_VT
  #if defined(CONFIG_VGA_CONSOLE)
if (!efi_enabled(EFI_BOOT) || (efi_mem_type(0xa) != 
EFI_CONVENTIONAL_MEMORY))
-   conswitchp = &vga_con;
+   vgacon_register_screen(&screen_info);
  #endif
  #endif
x86_init.oem.banner();
diff --git a/drivers/firmware/pcdp.c b/drivers/firmware/pcdp.c
index 715a45442d1cf..667a595373b2d 100644
--- a/drivers/firmware/pcdp.c
+++ b/drivers/firmware/pcdp.c
@@ -72,7 +72,7 @@ setup_vga_console(struct pcdp_device *dev)
return -ENODEV;
}
  
-	conswitchp = &vga_con;

+   vgacon_register_screen(&screen_info);
printk(KERN_INFO "PCDP: VGA console\n");
return 0;
  #else
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index e25ba523892e5..3d7fedf27ffc1 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -97,6 +97,8 @@ static intvga_video_font_height;
  static intvga_scan_lines  __read_mostly;
  static unsigned int   vga_rolled_over; /* last vc_origin offset before wrap */
  
+static struct screen_info *vga_si;

+
  static bool vga_hardscroll_enabled;
  static bool vga_hardscroll_user_enable = true;
  
@@ -161,8 +163,9 @@ static const char *vgacon_startup(void)

u16 saved1, saved2;
volatile u16 *p;
  
-	if (screen_info.orig_video_isVGA == VIDEO_TYPE_VLFB ||

-   screen_info.orig_video_isVGA == VIDEO_TYPE_EFI) {
+   if (!vga_si ||
+   vga_si->orig_video_isVGA == VIDEO_TYPE_VLFB ||
+   vga_si->orig_video_isVGA == VIDEO_TYPE_EFI) {
  no_vga:
  #ifdef CONFIG_DUMMY_CONSOLE
conswitchp = &dummy_con;
@@ -172,29 +175,29 @@ static const char *vgacon_startup(void)
  #endif
}
  
-	/* boot_params.screen_info reasonably initialized? */

-   if ((screen_info.orig_video_lines == 0) ||
-   (screen_info.orig_video_cols  == 0))
+   /* vga_si reasonably initialized? */
+   if ((vga_si->orig_video_lines == 0) ||
+   (vga_si->orig_video_cols  == 0))
goto no_vga;
  
  	/* VGA16 modes are not handled by VGACON */

-   if ((screen_info.orig_video_mode == 0x0D) ||/* 320x200/4 */
-   (sc

Re: [PATCH v3 1/3] scsi: BusLogic remove bus_to_virt

[Khalid Aziz]

On 6/24/22 09:52, Arnd Bergmann wrote:

From: Arnd Bergmann 

The BusLogic driver is the last remaining driver that relies on the
deprecated bus_to_virt() function, which in turn only works on a few
architectures, and is incompatible with both swiotlb and iommu support.

Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."),
the driver had a dependency on x86-32, presumably because of this
problem. However, the change introduced another bug that made it still
impossible to use the driver on any 64-bit machine.

This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix
64-bit system enumeration error for Buslogic"), 8 years later, which
shows that there are not a lot of users.

Maciej is still using the driver on 32-bit hardware, and Khalid mentioned
that the driver works with the device emulation used in VirtualBox
and VMware. Both of those only emulate it for Windows 2000 and older
operating systems that did not ship with the better LSI logic driver.

Do a minimum fix that searches through the list of descriptors to find
one that matches the bus address. This is clearly as inefficient as
was indicated in the code comment about the lack of a bus_to_virt()
replacement. A better fix would likely involve changing out the entire
descriptor allocation for a simpler one, but that would be much
more invasive.

Cc: Maciej W. Rozycki 
Cc: Matt Wang 
Tested-by: Khalid Aziz 
Reviewed-by: Robin Murphy 
Reviewed-by: Hannes Reinecke 
Signed-off-by: Arnd Bergmann 
---
v3: Address issues pointed out by Khalid Aziz
v2: Attempt to fix the driver instead of removing it
---
  drivers/scsi/BusLogic.c | 35 +++
  drivers/scsi/Kconfig|  2 +-
  2 files changed, 24 insertions(+), 13 deletions(-)



This looks good.

Acked-by: Khalid Aziz 



diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index a897c8f914cf..f2abffce2659 100644
--- a/drivers/scsi/BusLogic.c
+++ b/drivers/scsi/BusLogic.c
@@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter 
*adapter,
return (hoststatus << 16) | tgt_status;
  }
  
+/*

+ * turn the dma address from an inbox into a ccb pointer
+ * This is rather inefficient.
+ */
+static struct blogic_ccb *
+blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox)
+{
+   struct blogic_ccb *ccb;
+
+   for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all)
+   if (inbox->ccb == ccb->dma_handle)
+   break;
+
+   return ccb;
+}
  
  /*

blogic_scan_inbox scans the Incoming Mailboxes saving any
Incoming Mailbox entries for completion processing.
  */
-
  static void blogic_scan_inbox(struct blogic_adapter *adapter)
  {
/*
@@ -2540,17 +2554,14 @@ static void blogic_scan_inbox(struct blogic_adapter 
*adapter)
enum blogic_cmplt_code comp_code;
  
  	while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) {

-   /*
-  We are only allowed to do this because we limit our
-  architectures we run on to machines where bus_to_virt(
-  actually works.  There *needs* to be a dma_addr_to_virt()
-  in the new PCI DMA mapping interface to replace
-  bus_to_virt() or else this code is going to become very
-  innefficient.
-*/
-   struct blogic_ccb *ccb =
-   (struct blogic_ccb *) bus_to_virt(next_inbox->ccb);
-   if (comp_code != BLOGIC_CMD_NOTFOUND) {
+   struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, 
next_inbox);
+   if (!ccb) {
+   /*
+* This should never happen, unless the CCB list is
+* corrupted in memory.
+*/
+   blogic_warn("Could not find CCB for dma address %x\n", 
adapter, next_inbox->ccb);
+   } else if (comp_code != BLOGIC_CMD_NOTFOUND) {
if (ccb->status == BLOGIC_CCB_ACTIVE ||
ccb->status == BLOGIC_CCB_RESET) {
/*
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 6e3a04107bb6..689186f3a908 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -514,7 +514,7 @@ config SCSI_HPTIOP
  
  config SCSI_BUSLOGIC

tristate "BusLogic SCSI support"
-   depends on PCI && SCSI && VIRT_TO_BUS
+   depends on PCI && SCSI
help
  This is support for BusLogic MultiMaster and FlashPoint SCSI Host
  Adapters. Consult the SCSI-HOWTO, available from

Re: [PATCH v2 2/3] scsi: BusLogic remove bus_to_virt

[Khalid Aziz]

On 6/23/22 08:47, Arnd Bergmann wrote:



Can you test it again with this patch on top?

diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index d057abfcdd5c..9e67f2ee25ee 100644
--- a/drivers/scsi/BusLogic.c
+++ b/drivers/scsi/BusLogic.c
@@ -2554,8 +2554,14 @@ static void blogic_scan_inbox(struct
blogic_adapter *adapter)
 enum blogic_cmplt_code comp_code;

 while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) {
-   struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter,
adapter->next_inbox);
-   if (comp_code != BLOGIC_CMD_NOTFOUND) {
+   struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter,
next_inbox);
+   if (!ccb) {
+   /*
+* This should never happen, unless the CCB list is
+* corrupted in memory.
+*/
+   blogic_warn("Could not find CCB for dma
address 0x%x\n", adapter, next_inbox->ccb);
+   } else if (comp_code != BLOGIC_CMD_NOTFOUND) {
 if (ccb->status == BLOGIC_CCB_ACTIVE ||
 ccb->status == BLOGIC_CCB_RESET) {


Hi Arnd,

Driver works with this change. next_inbox is the correct pointer to pass.

Thanks,
Khalid

Re: [PATCH v2 2/3] scsi: BusLogic remove bus_to_virt

[Khalid Aziz]

On 6/17/22 06:57, Arnd Bergmann wrote:

From: Arnd Bergmann 

The BusLogic driver is the last remaining driver that relies on the
deprecated bus_to_virt() function, which in turn only works on a few
architectures, and is incompatible with both swiotlb and iommu support.

Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."),
the driver had a dependency on x86-32, presumably because of this
problem. However, the change introduced another bug that made it still
impossible to use the driver on any 64-bit machine.

This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix
64-bit system enumeration error for Buslogic"), 8 years later, which
shows that there are not a lot of users.

Maciej is still using the driver on 32-bit hardware, and Khalid mentioned
that the driver works with the device emulation used in VirtualBox
and VMware. Both of those only emulate it for Windows 2000 and older
operating systems that did not ship with the better LSI logic driver.

Do a minimum fix that searches through the list of descriptors to find
one that matches the bus address. This is clearly as inefficient as
was indicated in the code comment about the lack of a bus_to_virt()
replacement. A better fix would likely involve changing out the entire
descriptor allocation for a simpler one, but that would be much
more invasive.

Cc: Maciej W. Rozycki 
Cc: Matt Wang 
Cc: Khalid Aziz 
Signed-off-by: Arnd Bergmann 
---
  drivers/scsi/BusLogic.c | 27 ---
  drivers/scsi/Kconfig|  2 +-
  2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index a897c8f914cf..d057abfcdd5c 100644
--- a/drivers/scsi/BusLogic.c
+++ b/drivers/scsi/BusLogic.c
@@ -2515,12 +2515,26 @@ static int blogic_resultcode(struct blogic_adapter 
*adapter,
return (hoststatus << 16) | tgt_status;
  }
  
+/*

+ * turn the dma address from an inbox into a ccb pointer
+ * This is rather inefficient.
+ */
+static struct blogic_ccb *
+blogic_inbox_to_ccb(struct blogic_adapter *adapter, struct blogic_inbox *inbox)
+{
+   struct blogic_ccb *ccb;
+
+   for (ccb = adapter->all_ccbs; ccb; ccb = ccb->next_all)
+   if (inbox->ccb == ccb->dma_handle)
+   break;
+
+   return ccb;
+}
  
  /*

blogic_scan_inbox scans the Incoming Mailboxes saving any
Incoming Mailbox entries for completion processing.
  */
-
  static void blogic_scan_inbox(struct blogic_adapter *adapter)
  {
/*
@@ -2540,16 +2554,7 @@ static void blogic_scan_inbox(struct blogic_adapter 
*adapter)
enum blogic_cmplt_code comp_code;
  
  	while ((comp_code = next_inbox->comp_code) != BLOGIC_INBOX_FREE) {

-   /*
-  We are only allowed to do this because we limit our
-  architectures we run on to machines where bus_to_virt(
-  actually works.  There *needs* to be a dma_addr_to_virt()
-  in the new PCI DMA mapping interface to replace
-  bus_to_virt() or else this code is going to become very
-  innefficient.
-*/
-   struct blogic_ccb *ccb =
-   (struct blogic_ccb *) bus_to_virt(next_inbox->ccb);
+   struct blogic_ccb *ccb = blogic_inbox_to_ccb(adapter, 
adapter->next_inbox);


This change looks good enough as workaround to not use bus_to_virt() for 
now. There are two problems I see though. One, I do worry about 
blogic_inbox_to_ccb() returning NULL for ccb which should not happen 
unless the mailbox pointer was corrupted which would indicate a bigger 
problem. Nevertheless a NULL pointer causing kernel panic concerns me. 
How about adding a check before we dereference ccb?


Second, with this patch applied, I am seeing errors from the driver:

=
[ 1623.902685]  sdb: sdb1 sdb2
[ 1623.903245] sd 2:0:0:0: [sdb] Attached SCSI disk
[ 1623.911000] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox
[ 1623.911005] scsi2: Illegal CCB #76 status 2 in Incoming Mailbox
[ 1623.911070] scsi2: Illegal CCB #79 status 2 in Incoming Mailbox
[ 1651.458008] scsi2: Warning: Partition Table appears to have Geometry 
256/63 which is
[ 1651.458013] scsi2: not compatible with current BusLogic Host Adapter 
Geometry 255/63

[ 1658.797609] scsi2: Resetting BusLogic BT-958D Failed
[ 1659.533208] sd 2:0:0:0: Device offlined - not ready after error recovery
[ 1659.51] sd 2:0:0:0: Device offlined - not ready after error recovery
[ 1659.53] sd 2:0:0:0: Device offlined - not ready after error recovery
[ 1659.533342] sd 2:0:0:0: [sdb] tag#101 FAILED Result: 
hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK cmd_age=35s
[ 1659.533345] sd 2:0:0:0: [sdb] tag#101 CDB: Read(10) 28 00 00 00 00 28 
00 00 10 00
[ 1659.533346] I/O error, dev sdb, sector 40 op 0x0:(READ) flags 0x80700 
phys_seg 1 prio class 0


=

This is on Vi

Re: [PATCH 5/6] scsi: remove stale BusLogic driver

[Khalid Aziz]

On 6/6/22 02:41, Arnd Bergmann wrote:

From: Arnd Bergmann

The BusLogic driver is the last remaining driver that relies on the
deprecated bus_to_virt() function, which in turn only works on a few
architectures, and is incompatible with both swiotlb and iommu support.

Before commit 391e2f25601e ("[SCSI] BusLogic: Port driver to 64-bit."),
the driver had a dependency on x86-32, presumably because of this
problem. However, the change introduced another bug that made it still
impossible to use the driver on any 64-bit machine.

This was in turn fixed in commit 56f396146af2 ("scsi: BusLogic: Fix
64-bit system enumeration error for Buslogic"), 8 years later.

The fact that this was found at all is an indication that there are
users, and it seems that Maciej, Matt and Khalid all have access to
this hardware, but if it took eight years to find the problem,
it's likely that nobody actually relies on it.

Remove it as part of the global virt_to_bus()/bus_to_virt() removal.
If anyone is still interested in keeping this driver, the alternative
is to stop it from using bus_to_virt(), possibly along the lines of
how dpt_i2o gets around the same issue.

Cc: Maciej W. Rozycki
Cc: Matt Wang
Cc: Khalid Aziz
Signed-off-by: Arnd Bergmann
---
  Documentation/scsi/BusLogic.rst   |  581 ---
  Documentation/scsi/FlashPoint.rst |  176 -
  MAINTAINERS   |7 -
  drivers/scsi/BusLogic.c   | 3727 --
  drivers/scsi/BusLogic.h   | 1284 -
  drivers/scsi/FlashPoint.c | 7560 -
  drivers/scsi/Kconfig  |   24 -
  7 files changed, 13359 deletions(-)
  delete mode 100644 Documentation/scsi/BusLogic.rst
  delete mode 100644 Documentation/scsi/FlashPoint.rst
  delete mode 100644 drivers/scsi/BusLogic.c
  delete mode 100644 drivers/scsi/BusLogic.h
  delete mode 100644 drivers/scsi/FlashPoint.c


I would say no to removing BusLogic driver. Virtualbox is another 
consumer of this driver. This driver is very old but I would rather fix 
the issues than remove it until we do not have any users.


Thanks,
Khalid

Re: [PATCH V6 4/7] sparc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT

[Khalid Aziz]

On 4/13/22 00:22, Anshuman Khandual wrote:



On 4/13/22 11:43, Christophe Leroy wrote:



Le 13/04/2022 à 07:58, Anshuman Khandual a écrit :

This defines and exports a platform specific custom vm_get_page_prot() via
subscribing ARCH_HAS_VM_GET_PAGE_PROT. It localizes arch_vm_get_page_prot()
as sparc_vm_get_page_prot() and moves near vm_get_page_prot().

Cc: "David S. Miller" 
Cc: Khalid Aziz 
Cc: sparcli...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Reviewed-by: Khalid Aziz 
Signed-off-by: Anshuman Khandual 
---
   arch/sparc/Kconfig|  1 +
   arch/sparc/include/asm/mman.h |  6 --
   arch/sparc/mm/init_64.c   | 13 +
   3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 9200bc04701c..85b573643af6 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -84,6 +84,7 @@ config SPARC64
select PERF_USE_VMALLOC
select ARCH_HAVE_NMI_SAFE_CMPXCHG
select HAVE_C_RECORDMCOUNT
+   select ARCH_HAS_VM_GET_PAGE_PROT
select HAVE_ARCH_AUDITSYSCALL
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_SUPPORTS_DEBUG_PAGEALLOC
diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
index 274217e7ed70..af9c10c83dc5 100644
--- a/arch/sparc/include/asm/mman.h
+++ b/arch/sparc/include/asm/mman.h
@@ -46,12 +46,6 @@ static inline unsigned long sparc_calc_vm_prot_bits(unsigned 
long prot)
}
   }
   
-#define arch_vm_get_page_prot(vm_flags) sparc_vm_get_page_prot(vm_flags)

-static inline pgprot_t sparc_vm_get_page_prot(unsigned long vm_flags)
-{
-   return (vm_flags & VM_SPARC_ADI) ? __pgprot(_PAGE_MCD_4V) : __pgprot(0);
-}
-
   #define arch_validate_prot(prot, addr) sparc_validate_prot(prot, addr)
   static inline int sparc_validate_prot(unsigned long prot, unsigned long addr)
   {
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 8b1911591581..dcb17763c1f2 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -3184,3 +3184,16 @@ void copy_highpage(struct page *to, struct page *from)
}
   }
   EXPORT_SYMBOL(copy_highpage);
+
+static pgprot_t sparc_vm_get_page_prot(unsigned long vm_flags)
+{
+   return (vm_flags & VM_SPARC_ADI) ? __pgprot(_PAGE_MCD_4V) : __pgprot(0);
+}
+
+pgprot_t vm_get_page_prot(unsigned long vm_flags)
+{
+   return __pgprot(pgprot_val(protection_map[vm_flags &
+   (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
+   pgprot_val(sparc_vm_get_page_prot(vm_flags)));
+}
+EXPORT_SYMBOL(vm_get_page_prot);



sparc is now the only one with two functions. You can most likely do
like you did for ARM and POWERPC: merge into a single function:


I was almost about to do this one as well but as this patch has already been
reviewed with a tag, just skipped it. I will respin the series once more :)

Khalid,

Could I keep your review tag after the following change ?


Hi Anshuman,

Yes, this change makes sense.

--
Khalid





pgprot_t vm_get_page_prot(unsigned long vm_flags)
{
unsigned long prot = pgprot_val(protection_map[vm_flags &
(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]);

if (vm_flags & VM_SPARC_ADI)
prot |= _PAGE_MCD_4V;

return __pgprot(prot);
}
EXPORT_SYMBOL(vm_get_page_prot);


- Anshuman

Re: [PATCH V4 4/7] sparc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT

[Khalid Aziz]

On 4/7/22 04:32, Anshuman Khandual wrote:

This defines and exports a platform specific custom vm_get_page_prot() via
subscribing ARCH_HAS_VM_GET_PAGE_PROT. It localizes arch_vm_get_page_prot()
as sparc_vm_get_page_prot() and moves near vm_get_page_prot().

Cc: "David S. Miller" 
Cc: Khalid Aziz 
Cc: sparcli...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Anshuman Khandual 
---
  arch/sparc/Kconfig|  1 +
  arch/sparc/include/asm/mman.h |  6 --
  arch/sparc/mm/init_64.c   | 13 +
  3 files changed, 14 insertions(+), 6 deletions(-)



Reviewed-by: Khalid Aziz 



diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 9200bc04701c..85b573643af6 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -84,6 +84,7 @@ config SPARC64
select PERF_USE_VMALLOC
select ARCH_HAVE_NMI_SAFE_CMPXCHG
select HAVE_C_RECORDMCOUNT
+   select ARCH_HAS_VM_GET_PAGE_PROT
select HAVE_ARCH_AUDITSYSCALL
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_SUPPORTS_DEBUG_PAGEALLOC
diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
index 274217e7ed70..af9c10c83dc5 100644
--- a/arch/sparc/include/asm/mman.h
+++ b/arch/sparc/include/asm/mman.h
@@ -46,12 +46,6 @@ static inline unsigned long sparc_calc_vm_prot_bits(unsigned 
long prot)
}
  }
  
-#define arch_vm_get_page_prot(vm_flags) sparc_vm_get_page_prot(vm_flags)

-static inline pgprot_t sparc_vm_get_page_prot(unsigned long vm_flags)
-{
-   return (vm_flags & VM_SPARC_ADI) ? __pgprot(_PAGE_MCD_4V) : __pgprot(0);
-}
-
  #define arch_validate_prot(prot, addr) sparc_validate_prot(prot, addr)
  static inline int sparc_validate_prot(unsigned long prot, unsigned long addr)
  {
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 8b1911591581..dcb17763c1f2 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -3184,3 +3184,16 @@ void copy_highpage(struct page *to, struct page *from)
}
  }
  EXPORT_SYMBOL(copy_highpage);
+
+static pgprot_t sparc_vm_get_page_prot(unsigned long vm_flags)
+{
+   return (vm_flags & VM_SPARC_ADI) ? __pgprot(_PAGE_MCD_4V) : __pgprot(0);
+}
+
+pgprot_t vm_get_page_prot(unsigned long vm_flags)
+{
+   return __pgprot(pgprot_val(protection_map[vm_flags &
+   (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
+   pgprot_val(sparc_vm_get_page_prot(vm_flags)));
+}
+EXPORT_SYMBOL(vm_get_page_prot);

Re: [PATCH 1/2] mm/mprotect: Call arch_validate_prot under mmap_lock and with length

[Khalid Aziz]

On 10/15/20 3:05 AM, Catalin Marinas wrote:
> On Wed, Oct 14, 2020 at 03:21:16PM -0600, Khalid Aziz wrote:
>> What FreeBSD does seems like a reasonable thing to do. Any way first
>> thing to do is to update sparc to use arch_validate_flags() and update
>> sparc_validate_prot() to not peek into vma without lock.
> 
> If you go for arch_validate_flags(), I think sparc_validate_prot()
> doesn't need the vma at all.

Yes, the plan is to move vma flag check from sparc_validate_prot() to
arch_validate_flags()..

> 
> BTW, on the ADI topic, I think you have a race in do_swap_page() since
> set_pte_at() is called before arch_do_swap_page(). So a thread in the
> same process would see the new mapping but the tags have not been
> updated yet. Unless sparc relies on the new user pte to be set, I think
> you can just swap the two calls.
> 

Thanks for pointing that out. I will take a look at it.

--
Khalid

Re: [PATCH 1/2] mm/mprotect: Call arch_validate_prot under mmap_lock and with length

[Khalid Aziz]

On 10/13/20 3:16 AM, Catalin Marinas wrote:
> On Mon, Oct 12, 2020 at 01:14:50PM -0600, Khalid Aziz wrote:
>> On 10/12/20 11:22 AM, Catalin Marinas wrote:
>>> On Mon, Oct 12, 2020 at 11:03:33AM -0600, Khalid Aziz wrote:
>>>> On 10/10/20 5:09 AM, Catalin Marinas wrote:
>>>>> On Wed, Oct 07, 2020 at 02:14:09PM -0600, Khalid Aziz wrote:
>>>>>> On 10/7/20 1:39 AM, Jann Horn wrote:
>>>>>>> arch_validate_prot() is a hook that can validate whether a given set of
>>>>>>> protection flags is valid in an mprotect() operation. It is given the 
>>>>>>> set
>>>>>>> of protection flags and the address being modified.
>>>>>>>
>>>>>>> However, the address being modified can currently not actually be used 
>>>>>>> in
>>>>>>> a meaningful way because:
>>>>>>>
>>>>>>> 1. Only the address is given, but not the length, and the operation can
>>>>>>>span multiple VMAs. Therefore, the callee can't actually tell which
>>>>>>>virtual address range, or which VMAs, are being targeted.
>>>>>>> 2. The mmap_lock is not held, meaning that if the callee were to check
>>>>>>>the VMA at @addr, that VMA would be unrelated to the one the
>>>>>>>operation is performed on.
>>>>>>>
>>>>>>> Currently, custom arch_validate_prot() handlers are defined by
>>>>>>> arm64, powerpc and sparc.
>>>>>>> arm64 and powerpc don't care about the address range, they just check 
>>>>>>> the
>>>>>>> flags against CPU support masks.
>>>>>>> sparc's arch_validate_prot() attempts to look at the VMA, but doesn't 
>>>>>>> take
>>>>>>> the mmap_lock.
>>>>>>>
>>>>>>> Change the function signature to also take a length, and move the
>>>>>>> arch_validate_prot() call in mm/mprotect.c down into the locked region.
>>>>> [...]
>>>>>> As Chris pointed out, the call to arch_validate_prot() from do_mmap2()
>>>>>> is made without holding mmap_lock. Lock is not acquired until
>>>>>> vm_mmap_pgoff(). This variance is uncomfortable but I am more
>>>>>> uncomfortable forcing all implementations of validate_prot to require
>>>>>> mmap_lock be held when non-sparc implementations do not have such need
>>>>>> yet. Since do_mmap2() is in powerpc specific code, for now this patch
>>>>>> solves a current problem.
>>>>>
>>>>> I still think sparc should avoid walking the vmas in
>>>>> arch_validate_prot(). The core code already has the vmas, though not
>>>>> when calling arch_validate_prot(). That's one of the reasons I added
>>>>> arch_validate_flags() with the MTE patches. For sparc, this could be
>>>>> (untested, just copied the arch_validate_prot() code):
>>>>
>>>> I am little uncomfortable with the idea of validating protection bits
>>>> inside the VMA walk loop in do_mprotect_pkey(). When ADI is being
>>>> enabled across multiple VMAs and arch_validate_flags() fails on a VMA
>>>> later, do_mprotect_pkey() will bail out with error leaving ADI enabled
>>>> on earlier VMAs. This will apply to protection bits other than ADI as
>>>> well of course. This becomes a partial failure of mprotect() call. I
>>>> think it should be all or nothing with mprotect() - when one calls
>>>> mprotect() from userspace, either the entire address range passed in
>>>> gets its protection bits updated or none of it does. That requires
>>>> validating protection bits upfront or undoing what earlier iterations of
>>>> VMA walk loop might have done.
>>>
>>> I thought the same initially but mprotect() already does this with the
>>> VM_MAY* flag checking. If you ask it for an mprotect() that crosses
>>> multiple vmas and one of them fails, it doesn't roll back the changes to
>>> the prior ones. I considered that a similar approach is fine for MTE
>>> (it's most likely a user error).
>>
>> You are right about the current behavior with VM_MAY* flags, but that is
>> not the right behavior. Adding more cases to this just perpetuates
>> incorrect behavior. It is not easy to roll back changes after VMAs ha

Re: [PATCH 1/2] mm/mprotect: Call arch_validate_prot under mmap_lock and with length

[Khalid Aziz]

On 10/12/20 11:22 AM, Catalin Marinas wrote:
> On Mon, Oct 12, 2020 at 11:03:33AM -0600, Khalid Aziz wrote:
>> On 10/10/20 5:09 AM, Catalin Marinas wrote:
>>> On Wed, Oct 07, 2020 at 02:14:09PM -0600, Khalid Aziz wrote:
>>>> On 10/7/20 1:39 AM, Jann Horn wrote:
>>>>> arch_validate_prot() is a hook that can validate whether a given set of
>>>>> protection flags is valid in an mprotect() operation. It is given the set
>>>>> of protection flags and the address being modified.
>>>>>
>>>>> However, the address being modified can currently not actually be used in
>>>>> a meaningful way because:
>>>>>
>>>>> 1. Only the address is given, but not the length, and the operation can
>>>>>span multiple VMAs. Therefore, the callee can't actually tell which
>>>>>virtual address range, or which VMAs, are being targeted.
>>>>> 2. The mmap_lock is not held, meaning that if the callee were to check
>>>>>the VMA at @addr, that VMA would be unrelated to the one the
>>>>>operation is performed on.
>>>>>
>>>>> Currently, custom arch_validate_prot() handlers are defined by
>>>>> arm64, powerpc and sparc.
>>>>> arm64 and powerpc don't care about the address range, they just check the
>>>>> flags against CPU support masks.
>>>>> sparc's arch_validate_prot() attempts to look at the VMA, but doesn't take
>>>>> the mmap_lock.
>>>>>
>>>>> Change the function signature to also take a length, and move the
>>>>> arch_validate_prot() call in mm/mprotect.c down into the locked region.
>>> [...]
>>>> As Chris pointed out, the call to arch_validate_prot() from do_mmap2()
>>>> is made without holding mmap_lock. Lock is not acquired until
>>>> vm_mmap_pgoff(). This variance is uncomfortable but I am more
>>>> uncomfortable forcing all implementations of validate_prot to require
>>>> mmap_lock be held when non-sparc implementations do not have such need
>>>> yet. Since do_mmap2() is in powerpc specific code, for now this patch
>>>> solves a current problem.
>>>
>>> I still think sparc should avoid walking the vmas in
>>> arch_validate_prot(). The core code already has the vmas, though not
>>> when calling arch_validate_prot(). That's one of the reasons I added
>>> arch_validate_flags() with the MTE patches. For sparc, this could be
>>> (untested, just copied the arch_validate_prot() code):
>>
>> I am little uncomfortable with the idea of validating protection bits
>> inside the VMA walk loop in do_mprotect_pkey(). When ADI is being
>> enabled across multiple VMAs and arch_validate_flags() fails on a VMA
>> later, do_mprotect_pkey() will bail out with error leaving ADI enabled
>> on earlier VMAs. This will apply to protection bits other than ADI as
>> well of course. This becomes a partial failure of mprotect() call. I
>> think it should be all or nothing with mprotect() - when one calls
>> mprotect() from userspace, either the entire address range passed in
>> gets its protection bits updated or none of it does. That requires
>> validating protection bits upfront or undoing what earlier iterations of
>> VMA walk loop might have done.
> 
> I thought the same initially but mprotect() already does this with the
> VM_MAY* flag checking. If you ask it for an mprotect() that crosses
> multiple vmas and one of them fails, it doesn't roll back the changes to
> the prior ones. I considered that a similar approach is fine for MTE
> (it's most likely a user error).
> 

You are right about the current behavior with VM_MAY* flags, but that is
not the right behavior. Adding more cases to this just perpetuates
incorrect behavior. It is not easy to roll back changes after VMAs have
potentially been split/merged which is probably why the current code
simply throws in the towel and returns with partially modified address
space. It is lot easier to do all the checks upfront and then proceed or
not proceed with modifying VMAs. One approach might be to call
arch_validate_flags() in a loop before modifying VMAs and walk all VMAs
with a read lock held. Current code also bails out with ENOMEM if it
finds a hole in the address range and leaves any modifications already
made in place. This is another case where a hole could have been
detected earlier.

--
Khalid

Re: [PATCH 1/2] mm/mprotect: Call arch_validate_prot under mmap_lock and with length

[Khalid Aziz]

On 10/10/20 5:09 AM, Catalin Marinas wrote:
> Hi Khalid,
> 
> On Wed, Oct 07, 2020 at 02:14:09PM -0600, Khalid Aziz wrote:
>> On 10/7/20 1:39 AM, Jann Horn wrote:
>>> arch_validate_prot() is a hook that can validate whether a given set of
>>> protection flags is valid in an mprotect() operation. It is given the set
>>> of protection flags and the address being modified.
>>>
>>> However, the address being modified can currently not actually be used in
>>> a meaningful way because:
>>>
>>> 1. Only the address is given, but not the length, and the operation can
>>>span multiple VMAs. Therefore, the callee can't actually tell which
>>>virtual address range, or which VMAs, are being targeted.
>>> 2. The mmap_lock is not held, meaning that if the callee were to check
>>>the VMA at @addr, that VMA would be unrelated to the one the
>>>operation is performed on.
>>>
>>> Currently, custom arch_validate_prot() handlers are defined by
>>> arm64, powerpc and sparc.
>>> arm64 and powerpc don't care about the address range, they just check the
>>> flags against CPU support masks.
>>> sparc's arch_validate_prot() attempts to look at the VMA, but doesn't take
>>> the mmap_lock.
>>>
>>> Change the function signature to also take a length, and move the
>>> arch_validate_prot() call in mm/mprotect.c down into the locked region.
> [...]
>> As Chris pointed out, the call to arch_validate_prot() from do_mmap2()
>> is made without holding mmap_lock. Lock is not acquired until
>> vm_mmap_pgoff(). This variance is uncomfortable but I am more
>> uncomfortable forcing all implementations of validate_prot to require
>> mmap_lock be held when non-sparc implementations do not have such need
>> yet. Since do_mmap2() is in powerpc specific code, for now this patch
>> solves a current problem.
> 
> I still think sparc should avoid walking the vmas in
> arch_validate_prot(). The core code already has the vmas, though not
> when calling arch_validate_prot(). That's one of the reasons I added
> arch_validate_flags() with the MTE patches. For sparc, this could be
> (untested, just copied the arch_validate_prot() code):

I am little uncomfortable with the idea of validating protection bits
inside the VMA walk loop in do_mprotect_pkey(). When ADI is being
enabled across multiple VMAs and arch_validate_flags() fails on a VMA
later, do_mprotect_pkey() will bail out with error leaving ADI enabled
on earlier VMAs. This will apply to protection bits other than ADI as
well of course. This becomes a partial failure of mprotect() call. I
think it should be all or nothing with mprotect() - when one calls
mprotect() from userspace, either the entire address range passed in
gets its protection bits updated or none of it does. That requires
validating protection bits upfront or undoing what earlier iterations of
VMA walk loop might have done.

--
Khalid

> 
> static inline bool arch_validate_flags(unsigned long vm_flags)
> {
>   if (!(vm_flags & VM_SPARC_ADI))
>   return true;
> 
>   if (!adi_capable())
>   return false;
> 
>   /* ADI can not be enabled on PFN mapped pages */
>   if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>   return false;
> 
>   /*
>* Mergeable pages can become unmergeable if ADI is enabled on
>* them even if they have identical data on them. This can be
>* because ADI enabled pages with identical data may still not
>* have identical ADI tags on them. Disallow ADI on mergeable
>* pages.
>*/
>   if (vma->vm_flags & VM_MERGEABLE)
>   return false;
> 
>   return true;
> }
> 
>> That leaves open the question of should
>> generic mmap call arch_validate_prot and return EINVAL for invalid
>> combination of protection bits, but that is better addressed in a
>> separate patch.
> 
> The above would cover mmap() as well.
> 
> The current sparc_validate_prot() relies on finding the vma for the
> corresponding address. However, if you call this early in the mmap()
> path, there's no such vma. It is only created later in mmap_region()
> which no longer has the original PROT_* flags (all converted to VM_*
> flags).
> 
> Calling arch_validate_flags() on mmap() has a small side-effect on the
> user ABI: if the CPU is not adi_capable(), PROT_ADI is currently ignored
> on mmap() but rejected by sparc_validate_prot(). Powerpc already does
> this already and I think it should be fine for arm64 (it needs checking
> though as we have another flag, PROT_BTI, hopefully dynamic loaders
> don't pass this flag unconditionally).
> 
> However, as I said above, it doesn't solve the mmap() PROT_ADI checking
> for sparc since there's no vma yet. I'd strongly recommend the
> arch_validate_flags() approach and reverting the "start" parameter added
> to arch_validate_prot() if you go for the flags route.
> 
> Thanks.
> 

Re: [PATCH 2/2] sparc: Check VMA range in sparc_validate_prot()

[Khalid Aziz]

On 10/7/20 1:39 AM, Jann Horn wrote:
> sparc_validate_prot() is called from do_mprotect_pkey() as
> arch_validate_prot(); it tries to ensure that an mprotect() call can't
> enable ADI on incompatible VMAs.
> The current implementation only checks that the VMA at the start address
> matches the rules for ADI mappings; instead, check all VMAs that will be
> affected by mprotect().
> 
> (This hook is called before mprotect() makes sure that the specified range
> is actually covered by VMAs, and mprotect() returns specific error codes
> when that's not the case. In order for mprotect() to still generate the
> same error codes for mprotect(, , ...|PROT_ADI), we need
> to *accept* cases where the range is not fully covered by VMAs.)
> 
> Cc: sta...@vger.kernel.org
> Fixes: 74a04967482f ("sparc64: Add support for ADI (Application Data 
> Integrity)")
> Signed-off-by: Jann Horn 
> ---
> compile-tested only, I don't have a Sparc ADI setup - might be nice if some
> Sparc person could test this?
> 
>  arch/sparc/include/asm/mman.h | 50 +------
>  1 file changed, 30 insertions(+), 20 deletions(-)


Looks good to me.

Reviewed-by: Khalid Aziz 


> 
> diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
> index e85222c76585..6dced75567c3 100644
> --- a/arch/sparc/include/asm/mman.h
> +++ b/arch/sparc/include/asm/mman.h
> @@ -60,31 +60,41 @@ static inline int sparc_validate_prot(unsigned long prot, 
> unsigned long addr,
>   if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_ADI))
>   return 0;
>   if (prot & PROT_ADI) {
> + struct vm_area_struct *vma, *next;
> +
>   if (!adi_capable())
>   return 0;
>  
> - if (addr) {
> - struct vm_area_struct *vma;
> + vma = find_vma(current->mm, addr);
> + /* if @addr is unmapped, let mprotect() deal with it */
> + if (!vma || vma->vm_start > addr)
> + return 1;
> + while (1) {
> + /* ADI can not be enabled on PFN
> +  * mapped pages
> +  */
> + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> + return 0;
>  
> - vma = find_vma(current->mm, addr);
> - if (vma) {
> - /* ADI can not be enabled on PFN
> -  * mapped pages
> -  */
> - if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> - return 0;
> + /* Mergeable pages can become unmergeable
> +  * if ADI is enabled on them even if they
> +  * have identical data on them. This can be
> +  * because ADI enabled pages with identical
> +  * data may still not have identical ADI
> +  * tags on them. Disallow ADI on mergeable
> +  * pages.
> +  */
> + if (vma->vm_flags & VM_MERGEABLE)
> + return 0;
>  
> - /* Mergeable pages can become unmergeable
> -  * if ADI is enabled on them even if they
> -  * have identical data on them. This can be
> -  * because ADI enabled pages with identical
> -  * data may still not have identical ADI
> -  * tags on them. Disallow ADI on mergeable
> -  * pages.
> -  */
> - if (vma->vm_flags & VM_MERGEABLE)
> - return 0;
> - }
> + /* reached the end of the range without errors? */
> + if (addr+len <= vma->vm_end)
> + return 1;
> + next = vma->vm_next;
> + /* if a VMA hole follows, let mprotect() deal with it */
> + if (!next || next->vm_start != vma->vm_end)
> + return 1;
> + vma = next;
>   }
>   }
>   return 1;
> 

Re: [PATCH 1/2] mm/mprotect: Call arch_validate_prot under mmap_lock and with length

[Khalid Aziz]

On 10/7/20 1:39 AM, Jann Horn wrote:
> arch_validate_prot() is a hook that can validate whether a given set of
> protection flags is valid in an mprotect() operation. It is given the set
> of protection flags and the address being modified.
> 
> However, the address being modified can currently not actually be used in
> a meaningful way because:
> 
> 1. Only the address is given, but not the length, and the operation can
>span multiple VMAs. Therefore, the callee can't actually tell which
>virtual address range, or which VMAs, are being targeted.
> 2. The mmap_lock is not held, meaning that if the callee were to check
>the VMA at @addr, that VMA would be unrelated to the one the
>operation is performed on.
> 
> Currently, custom arch_validate_prot() handlers are defined by
> arm64, powerpc and sparc.
> arm64 and powerpc don't care about the address range, they just check the
> flags against CPU support masks.
> sparc's arch_validate_prot() attempts to look at the VMA, but doesn't take
> the mmap_lock.
> 
> Change the function signature to also take a length, and move the
> arch_validate_prot() call in mm/mprotect.c down into the locked region.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 9035cf9a97e4 ("mm: Add address parameter to arch_validate_prot()")
> Suggested-by: Khalid Aziz 
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Jann Horn 
> ---
>  arch/arm64/include/asm/mman.h   | 4 ++--
>  arch/powerpc/include/asm/mman.h | 3 ++-
>  arch/powerpc/kernel/syscalls.c  | 2 +-
>  arch/sparc/include/asm/mman.h   | 6 --
>  include/linux/mman.h| 3 ++-
>  mm/mprotect.c   | 6 --
>  6 files changed, 15 insertions(+), 9 deletions(-)


This looks good to me.

As Chris pointed out, the call to arch_validate_prot() from do_mmap2()
is made without holding mmap_lock. Lock is not acquired until
vm_mmap_pgoff(). This variance is uncomfortable but I am more
uncomfortable forcing all implementations of validate_prot to require
mmap_lock be held when non-sparc implementations do not have such need
yet. Since do_mmap2() is in powerpc specific code, for now this patch
solves a current problem. That leaves open the question of should
generic mmap call arch_validate_prot and return EINVAL for invalid
combination of protection bits, but that is better addressed in a
separate patch.

Reviewed-by: Khalid Aziz 

> 
> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> index 081ec8de9ea6..0876a87986dc 100644
> --- a/arch/arm64/include/asm/mman.h
> +++ b/arch/arm64/include/asm/mman.h
> @@ -23,7 +23,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
> vm_flags)
>  #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
>  
>  static inline bool arch_validate_prot(unsigned long prot,
> - unsigned long addr __always_unused)
> + unsigned long addr __always_unused, unsigned long len __always_unused)
>  {
>   unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
>  
> @@ -32,6 +32,6 @@ static inline bool arch_validate_prot(unsigned long prot,
>  
>   return (prot & ~supported) == 0;
>  }
> -#define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr)
> +#define arch_validate_prot(prot, addr, len) arch_validate_prot(prot, addr, 
> len)
>  
>  #endif /* ! __ASM_MMAN_H__ */
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 7cb6d18f5cd6..65dd9b594985 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -36,7 +36,8 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
> vm_flags)
>  }
>  #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
>  
> -static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
> +static inline bool arch_validate_prot(unsigned long prot, unsigned long addr,
> +   unsigned long len)
>  {
>   if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>   return false;
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index 078608ec2e92..b1fabb97d138 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -43,7 +43,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
>  {
>   long ret = -EINVAL;
>  
> - if (!arch_validate_prot(prot, addr))
> + if (!arch_validate_prot(prot, addr, len))
>   goto out;
>  
>   if (shift) {
> diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
> index f94532f25db1..e85222c76585 100644
> --- a/arch/sparc/include/asm/mman.h
&g

Re: [PATCH 01/16] mm: use untagged_addr() for get_user_pages_fast addresses

[Khalid Aziz]

On 6/21/19 7:39 AM, Jason Gunthorpe wrote:
> On Tue, Jun 11, 2019 at 04:40:47PM +0200, Christoph Hellwig wrote:
>> This will allow sparc64 to override its ADI tags for
>> get_user_pages and get_user_pages_fast.
>>
>> Signed-off-by: Christoph Hellwig 
>>  mm/gup.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index ddde097cf9e4..6bb521db67ec 100644
>> +++ b/mm/gup.c
>> @@ -2146,7 +2146,7 @@ int __get_user_pages_fast(unsigned long start, int 
>> nr_pages, int write,
>>  unsigned long flags;
>>  int nr = 0;
>>  
>> -start &= PAGE_MASK;
>> +start = untagged_addr(start) & PAGE_MASK;
>>  len = (unsigned long) nr_pages << PAGE_SHIFT;
>>  end = start + len;
> 
> Hmm, this function, and the other, goes on to do:
> 
> if (unlikely(!access_ok((void __user *)start, len)))
> return 0;
> 
> and I thought that access_ok takes in the tagged pointer?
> 
> How about re-order it a bit?

access_ok() can handle tagged or untagged pointers. It just strips the
tag bits from the top bits. Current order doesn't really matter from
functionality point of view. There might be minor gain in delaying
untagging in __get_user_pages_fast() but I could go either way.

--
Khalid

> 
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e410..f48747ced4723b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2148,11 +2148,12 @@ int __get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
>  
>   start &= PAGE_MASK;
>   len = (unsigned long) nr_pages << PAGE_SHIFT;
> - end = start + len;
> -
>   if (unlikely(!access_ok((void __user *)start, len)))
>   return 0;
>  
> + start = untagged_ptr(start);
> + end = start + len;
> +
>   /*
>* Disable interrupts.  We use the nested form as we can already have
>* interrupts disabled by get_futex_key.
> 

Re: [PATCH 10/16] mm: rename CONFIG_HAVE_GENERIC_GUP to CONFIG_HAVE_FAST_GUP

[Khalid Aziz]

On 6/11/19 8:40 AM, Christoph Hellwig wrote:
> We only support the generic GUP now, so rename the config option to
> be more clear, and always use the mm/Kconfig definition of the
> symbol and select it from the arch Kconfigs.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  arch/arm/Kconfig | 5 +
>  arch/arm64/Kconfig   | 4 +---
>  arch/mips/Kconfig| 2 +-
>  arch/powerpc/Kconfig | 2 +-
>  arch/s390/Kconfig| 2 +-
>  arch/sh/Kconfig  | 2 +-
>  arch/sparc/Kconfig   | 2 +-
>  arch/x86/Kconfig | 4 +---
>  mm/Kconfig   | 2 +-
>  mm/gup.c | 4 ++--
>  10 files changed, 11 insertions(+), 18 deletions(-)
> 

Looks good.

Reviewed-by: Khalid Aziz 

Re: [PATCH 09/16] sparc64: use the generic get_user_pages_fast code

[Khalid Aziz]

On 6/11/19 8:40 AM, Christoph Hellwig wrote:
> The sparc64 code is mostly equivalent to the generic one, minus various
> bugfixes and two arch overrides that this patch adds to pgtable.h.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  arch/sparc/Kconfig  |   1 +
>  arch/sparc/include/asm/pgtable_64.h |  18 ++
>  arch/sparc/mm/Makefile  |   2 +-
>  arch/sparc/mm/gup.c | 340 
>  4 files changed, 20 insertions(+), 341 deletions(-)
>  delete mode 100644 arch/sparc/mm/gup.c
> 

Reviewed-by: Khalid Aziz 

Re: [PATCH 08/16] sparc64: define untagged_addr()

[Khalid Aziz]

On 6/11/19 8:40 AM, Christoph Hellwig wrote:
> Add a helper to untag a user pointer.  This is needed for ADI support
> in get_user_pages_fast.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  arch/sparc/include/asm/pgtable_64.h | 22 ++
>  1 file changed, 22 insertions(+)

Looks good to me.

Reviewed-by: Khalid Aziz 

> 
> diff --git a/arch/sparc/include/asm/pgtable_64.h 
> b/arch/sparc/include/asm/pgtable_64.h
> index f0dcf991d27f..1904782dcd39 100644
> --- a/arch/sparc/include/asm/pgtable_64.h
> +++ b/arch/sparc/include/asm/pgtable_64.h
> @@ -1076,6 +1076,28 @@ static inline int io_remap_pfn_range(struct 
> vm_area_struct *vma,
>  }
>  #define io_remap_pfn_range io_remap_pfn_range 
>  
> +static inline unsigned long untagged_addr(unsigned long start)
> +{
> + if (adi_capable()) {
> + long addr = start;
> +
> + /* If userspace has passed a versioned address, kernel
> +  * will not find it in the VMAs since it does not store
> +  * the version tags in the list of VMAs. Storing version
> +  * tags in list of VMAs is impractical since they can be
> +  * changed any time from userspace without dropping into
> +  * kernel. Any address search in VMAs will be done with
> +  * non-versioned addresses. Ensure the ADI version bits
> +  * are dropped here by sign extending the last bit before
> +  * ADI bits. IOMMU does not implement version tags.
> +  */
> + return (addr << (long)adi_nbits()) >> (long)adi_nbits();
> + }
> +
> + return start;
> +}
> +#define untagged_addr untagged_addr
> +
>  #include 
>  #include 
>  
> 

Re: [PATCH 01/16] mm: use untagged_addr() for get_user_pages_fast addresses

[Khalid Aziz]

On 6/11/19 8:40 AM, Christoph Hellwig wrote:
> This will allow sparc64 to override its ADI tags for
> get_user_pages and get_user_pages_fast.
> 
> Signed-off-by: Christoph Hellwig 
> ---

Commit message is sparc64 specific but the goal here is to allow any
architecture with memory tagging to use this. So I would suggest
rewording the commit log. Other than that:

Reviewed-by: Khalid Aziz 

>  mm/gup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e4..6bb521db67ec 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2146,7 +2146,7 @@ int __get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
>   unsigned long flags;
>   int nr = 0;
>  
> - start &= PAGE_MASK;
> + start = untagged_addr(start) & PAGE_MASK;
>   len = (unsigned long) nr_pages << PAGE_SHIFT;
>   end = start + len;
>  
> @@ -2219,7 +2219,7 @@ int get_user_pages_fast(unsigned long start, int 
> nr_pages,
>   unsigned long addr, len, end;
>   int nr = 0, ret = 0;
>  
> - start &= PAGE_MASK;
> + start = untagged_addr(start) & PAGE_MASK;
>   addr = start;
>   len = (unsigned long) nr_pages << PAGE_SHIFT;
>   end = start + len;
> 

Re: [PATCH 01/16] uaccess: add untagged_addr definition for other arches

[Khalid Aziz]

On 6/1/19 1:49 AM, Christoph Hellwig wrote:
> From: Andrey Konovalov 
> 
> To allow arm64 syscalls to accept tagged pointers from userspace, we must
> untag them when they are passed to the kernel. Since untagging is done in
> generic parts of the kernel, the untagged_addr macro needs to be defined
> for all architectures.
> 
> Define it as a noop for architectures other than arm64.

Could you reword above sentence? We are already starting off with
untagged_addr() not being no-op for arm64 and sparc64. It will expand
further potentially. So something more along the lines of "Define it as
noop for architectures that do not support memory tagging". The first
paragraph in the log can also be rewritten to be not specific to arm64.

--
Khalid

> 
> Acked-by: Catalin Marinas 
> Signed-off-by: Andrey Konovalov 
> Signed-off-by: Christoph Hellwig 
> ---
>  include/linux/mm.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..949d43e9c0b6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -99,6 +99,10 @@ extern int mmap_rnd_compat_bits __read_mostly;
>  #include 
>  #include 
>  
> +#ifndef untagged_addr
> +#define untagged_addr(addr) (addr)
> +#endif
> +
>  #ifndef __pa_symbol
>  #define __pa_symbol(x)  __pa(RELOC_HIDE((unsigned long)(x), 0))
>  #endif
> 

Re: [PATCH v12 00/11] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

On 03/18/2018 09:08 AM, David Miller wrote:

In uapi/asm/auxvec.h you conditionalize the ADI aux vectors on
CONFIG_SPARC64.

That's not correct, you should never control user facing definitions
based upon kernel configuration.

Also, both 32-bit and 64-bit applications running on ADI capable
machines want to compile against and use this informaiton.

So please remove these CPP checks.



Hi Dave,

That makes sense. I will send a patch to remove these.

Thanks,
Khalid

[PATCH v12 00/11] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

ff from patch 4/4 in v6
- Patch 5/9: new patch split off from patch 4/4 in v6
- Patch 6/9: new patch split off from patch 4/4 in v6
- Patch 7/9: new patch
- Patch 8/9: new patch
- Patch 9/9:
- Enhanced arch_validate_prot() to enable ADI only on
  writable addresses backed by physical RAM
- Added support for saving/restoring ADI tags for each
  ADI block size address range on a page on swap in/out
- copy ADI tags on COW
- Updated values for auxiliary vectors to not conflict
  with values on other architectures to avoid conflict
  in glibc
- Disable same page merging on ADI enabled pages
- Enable ADI only on writable addresses backed by
  physical RAM
- Split parts of patch off into separate patches

Changelog v6:
- Patch 1/4: No changes
- Patch 2/4: No changes
- Patch 3/4: Added missing nop in the delay slot in
  sun4v_mcd_detect_precise
- Patch 4/4: Eliminated instructions to read and write PSTATE
  as well as MCDPER and PMCDPER on every access to userspace
  addresses by setting PSTATE and PMCDPER correctly upon entry
  into kernel

Changelog v5:
- Patch 1/4: No changes
- Patch 2/4: Replaced set_swp_pte_at() with new architecture
  functions arch_do_swap_page() and arch_unmap_one() that
  suppoprt architecture specific actions to be taken on page
  swap and migration
- Patch 3/4: Fixed indentation issues in assembly code
- Patch 4/4:
- Fixed indentation issues and instrcuctions in assembly
  code
- Removed CONFIG_SPARC64 from mdesc.c
- Changed to maintain state of MCDPER register in thread
  info flags as opposed to in mm context. MCDPER is a
  per-thread state and belongs in thread info flag as
  opposed to mm context which is shared across threads.
  Added comments to clarify this is a lazily maintained
  state and must be updated on context switch and
  copy_process() 
- Updated code to use the new arch_do_swap_page() and
  arch_unmap_one() functions

Testing:

- All functionality was tested with 8K normal pages as well as hugepages
  using malloc, mmap and shm.
- Multiple long duration stress tests were run using hugepages over 2+
  months. Normal pages were tested with shorter duration stress tests.
- Tested swapping with malloc and shm by reducing max memory and
  allocating three times the available system memory by active processes
  using ADI on allocated memory. Ran through multiple hours long runs of
  this test.
- Tested page migration with malloc and shm by migrating data pages of
  active ADI test process using migratepages, back and forth between two
  nodes every few seconds over an hour long run. Verified page migration
  through /proc//numa_maps.
- Tested COW support using test that forks children that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-

Khalid Aziz (11):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  mm: Allow arch code to override copy_highpage()
  sparc64: Add support for ADI (Application Data Integrity)
  sparc64: Update signal delivery to use new helper functions

 Documentation/sparc/adi.txt | 278 ++
 arch/powerpc/include/asm/mman.h |   4 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  47 
 arch/sparc/include/asm/elf_64.h |   5 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  84 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  50 
 arch/sparc/include/asm/page_64.h|   6 +
 arch/sparc/include/asm/pgtable_64.h |  48 
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  11 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/ke

[PATCH v12 07/11] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
Reviewed-by: Anthony Yznaga 
---
v8:
- Added addr parameter to powerpc arch_validate_prot() (suggested
  by Michael Ellerman)
v9:
- new patch

 arch/powerpc/include/asm/mman.h | 4 ++--
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 07e3f54de9e3..e3f1b5ba5d5c 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -43,7 +43,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
vm_flags)
 }
 #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
 
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
return false;
@@ -51,7 +51,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot arch_validate_prot
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 6a4d1caaff5c..4b08e9c9c538 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -92,7 +92,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index e3309fcf586b..088ea9c08678 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -417,7 +417,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0

Re: [PATCH v11 00/10] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

On 02/07/2018 12:38 AM, ebied...@xmission.com wrote:

Khalid Aziz  writes:


On 02/01/2018 07:29 PM, ebied...@xmission.com wrote:

Khalid Aziz  writes:


V11 changes:
This series is same as v10 and was simply rebased on 4.15 kernel. Can
mm maintainers please review patches 2, 7, 8 and 9 which are arch
independent, and include/linux/mm.h and mm/ksm.c changes in patch 10
and ack these if everything looks good?


I am a bit puzzled how this differs from the pkey's that other
architectures are implementing to achieve a similar result.

I am a bit mystified why you don't store the tag in a vma
instead of inventing a new way to store data on page out.


Hello Eric,

As Steven pointed out, sparc sets tags per cacheline unlike pkey. This results
in much finer granularity for tags that pkey and hence requires larger tag
storage than what we can do in a vma.


*Nod*   I am a bit mystified where you keep the information in memory.
I would think the tags would need to be stored per cacheline or per
tlb entry, in some kind of cache that could overflow.  So I would be
surprised if swapping is the only time this information needs stored
in memory.  Which makes me wonder if you have the proper data
structures.

I would think an array per vma or something in the page tables would
tend to make sense.

But perhaps I am missing something.


The ADI tags are stored in spare bits in the RAM. ADI tag storage is 
managed entirely by memory controller which maintains these tags per ADI 
block. An ADI block is the same size as cacheline on M7. Tags for each 
ADI block are associated with the physical ADI block, not the virtual 
address. When a physical page is reused, the physical ADI tag storage 
for that page is overwritten with new ADI tags, hence we need to store 
away the tags when we swap out a page. Kernel updates the ADI tags for 
physical page when it swaps a new page in. Each vma can cover variable 
number of pages so it is best to store a pointer to the tag storage in 
vma as opposed to actual tags in an array. Each 8K page can have 128 
tags on it. Since each tag is 4 bits, we need 64 bytes per page to store 
the tags. That can add up for a large vma.





Can you please use force_sig_fault to send these signals instead
of force_sig_info.  Emperically I have found that it is very
error prone to generate siginfo's by hand, especially on code
paths where several different si_codes may apply.  So it helps
to go through a helper function to ensure the fiddly bits are
all correct.  AKA the unused bits all need to be set to zero before
struct siginfo is copied to userspace.



What you say makes sense. I followed the same code as other fault handlers for
sparc. I could change just the fault handlers for ADI related faults. Would it
make more sense to change all the fault handlers in a separate patch and keep
the code in arch/sparc/kernel/traps_64.c consistent? Dave M, do you have a
preference?


It is my intention post -rc1 to start sending out patches to get the
rest of not just sparc but all of the architectures using the new
helpers.  I have the code I just ran out of time befor the merge
window opened to ensure everything had a good thorough review.

So if you can handle the your new changes I expect I will handle the
rest.



I can add a patch at the end of my series to update all force_sig_info() 
in my patchset to force_sig_fault(). That will sync my patches up with 
your changes cleanly. Does that work for you? I can send an updated 
series with this change. Can you review and ack the patches after this 
change.


Thanks,
Khalid

Re: [PATCH v11 00/10] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

On 02/01/2018 07:29 PM, ebied...@xmission.com wrote:

Khalid Aziz  writes:


V11 changes:
This series is same as v10 and was simply rebased on 4.15 kernel. Can
mm maintainers please review patches 2, 7, 8 and 9 which are arch
independent, and include/linux/mm.h and mm/ksm.c changes in patch 10
and ack these if everything looks good?


I am a bit puzzled how this differs from the pkey's that other
architectures are implementing to achieve a similar result.

I am a bit mystified why you don't store the tag in a vma
instead of inventing a new way to store data on page out.


Hello Eric,

As Steven pointed out, sparc sets tags per cacheline unlike pkey. This 
results in much finer granularity for tags that pkey and hence requires 
larger tag storage than what we can do in a vma.




Can you please use force_sig_fault to send these signals instead
of force_sig_info.  Emperically I have found that it is very
error prone to generate siginfo's by hand, especially on code
paths where several different si_codes may apply.  So it helps
to go through a helper function to ensure the fiddly bits are
all correct.  AKA the unused bits all need to be set to zero before
struct siginfo is copied to userspace.



What you say makes sense. I followed the same code as other fault 
handlers for sparc. I could change just the fault handlers for ADI 
related faults. Would it make more sense to change all the fault 
handlers in a separate patch and keep the code in 
arch/sparc/kernel/traps_64.c consistent? Dave M, do you have a preference?


Thanks,
Khalid

[PATCH v11 00/10] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

ADI block size address range on a page on swap in/out
- copy ADI tags on COW
- Updated values for auxiliary vectors to not conflict
  with values on other architectures to avoid conflict
  in glibc
- Disable same page merging on ADI enabled pages
- Enable ADI only on writable addresses backed by
  physical RAM
- Split parts of patch off into separate patches

Changelog v6:
- Patch 1/4: No changes
- Patch 2/4: No changes
- Patch 3/4: Added missing nop in the delay slot in
  sun4v_mcd_detect_precise
- Patch 4/4: Eliminated instructions to read and write PSTATE
  as well as MCDPER and PMCDPER on every access to userspace
  addresses by setting PSTATE and PMCDPER correctly upon entry
  into kernel

Changelog v5:
- Patch 1/4: No changes
- Patch 2/4: Replaced set_swp_pte_at() with new architecture
  functions arch_do_swap_page() and arch_unmap_one() that
  suppoprt architecture specific actions to be taken on page
  swap and migration
- Patch 3/4: Fixed indentation issues in assembly code
- Patch 4/4:
- Fixed indentation issues and instrcuctions in assembly
  code
- Removed CONFIG_SPARC64 from mdesc.c
- Changed to maintain state of MCDPER register in thread
  info flags as opposed to in mm context. MCDPER is a
  per-thread state and belongs in thread info flag as
  opposed to mm context which is shared across threads.
  Added comments to clarify this is a lazily maintained
  state and must be updated on context switch and
  copy_process() 
- Updated code to use the new arch_do_swap_page() and
  arch_unmap_one() functions

Testing:

- All functionality was tested with 8K normal pages as well as hugepages
  using malloc, mmap and shm.
- Multiple long duration stress tests were run using hugepages over 2+
  months. Normal pages were tested with shorter duration stress tests.
- Tested swapping with malloc and shm by reducing max memory and
  allocating three times the available system memory by active processes
  using ADI on allocated memory. Ran through multiple hours long runs of
  this test.
- Tested page migration with malloc and shm by migrating data pages of
  active ADI test process using migratepages, back and forth between two
  nodes every few seconds over an hour long run. Verified page migration
  through /proc//numa_maps.
- Tested COW support using test that forks children that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-

Khalid Aziz (10):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  mm: Allow arch code to override copy_highpage()
  sparc64: Add support for ADI (Application Data Integrity)

 Documentation/sparc/adi.txt | 278 ++
 arch/powerpc/include/asm/mman.h |   4 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  47 
 arch/sparc/include/asm/elf_64.h |   5 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  84 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  50 
 arch/sparc/include/asm/page_64.h|   6 +
 arch/sparc/include/asm/pgtable_64.h |  48 
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  11 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/kernel/Makefile  |   1 +
 arch/sparc/kernel/adi_64.c  | 397 
 arch/sparc/kernel/entry.h   |   3 +
 arch/sparc/kernel/etrap_64.S|  27 ++-
 arch/sparc/kernel/head_64.S |   1 +
 arch/sparc/kernel/mdesc.c   |   2 +
 arch/sparc/kernel/process_64.c  |  25 ++
 arch/sparc/kernel/rtrap_64.S|  33 ++-
 arch/sparc/kernel/setup_64.c|   2 +
 arch/sparc/kernel/sun4v_mcd.S  

[PATCH v11 07/10] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
Reviewed-by: Anthony Yznaga 
---
v8:
- Added addr parameter to powerpc arch_validate_prot() (suggested
  by Michael Ellerman)
v7:
- new patch

 arch/powerpc/include/asm/mman.h | 4 ++--
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..1d129f4521ac 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -32,7 +32,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
vm_flags)
 }
 #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
 
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
return false;
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot arch_validate_prot
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 6a4d1caaff5c..4b08e9c9c538 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -92,7 +92,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 58b629bb70de..80243e0166a7 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -412,7 +412,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0

Re: [PATCH v1] mm: relax deferred struct page requirements

[Khalid Aziz]

On Thu, 2017-11-16 at 20:46 -0500, Pavel Tatashin wrote:
> There is no need to have ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT,
> as all the page initialization code is in common code.
> 
> Also, there is no need to depend on MEMORY_HOTPLUG, as initialization
> code
> does not really use hotplug memory functionality. So, we can remove
> this
> requirement as well.
> 
> This patch allows to use deferred struct page initialization on all
> platforms with memblock allocator.
> 
> Tested on x86, arm64, and sparc. Also, verified that code compiles on
> PPC with CONFIG_MEMORY_HOTPLUG disabled.
> 
> Signed-off-by: Pavel Tatashin 
> ---
>  arch/powerpc/Kconfig | 1 -
>  arch/s390/Kconfig| 1 -
>  arch/x86/Kconfig | 1 -
>  mm/Kconfig   | 7 +--
>  4 files changed, 1 insertion(+), 9 deletions(-)
> 
> 

Looks reasonable to me.

Reviewed-by: Khalid Aziz 

[PATCH v10 00/10] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

page merging on ADI enabled pages
- Enable ADI only on writable addresses backed by
  physical RAM
- Split parts of patch off into separate patches

Changelog v6:
- Patch 1/4: No changes
- Patch 2/4: No changes
- Patch 3/4: Added missing nop in the delay slot in
  sun4v_mcd_detect_precise
- Patch 4/4: Eliminated instructions to read and write PSTATE
  as well as MCDPER and PMCDPER on every access to userspace
  addresses by setting PSTATE and PMCDPER correctly upon entry
  into kernel

Changelog v5:
- Patch 1/4: No changes
- Patch 2/4: Replaced set_swp_pte_at() with new architecture
  functions arch_do_swap_page() and arch_unmap_one() that
  suppoprt architecture specific actions to be taken on page
  swap and migration
- Patch 3/4: Fixed indentation issues in assembly code
- Patch 4/4:
- Fixed indentation issues and instrcuctions in assembly
  code
- Removed CONFIG_SPARC64 from mdesc.c
- Changed to maintain state of MCDPER register in thread
  info flags as opposed to in mm context. MCDPER is a
  per-thread state and belongs in thread info flag as
  opposed to mm context which is shared across threads.
  Added comments to clarify this is a lazily maintained
  state and must be updated on context switch and
  copy_process() 
- Updated code to use the new arch_do_swap_page() and
  arch_unmap_one() functions

Testing:

- All functionality was tested with 8K normal pages as well as hugepages
  using malloc, mmap and shm.
- Multiple long duration stress tests were run using hugepages over 2+
  months. Normal pages were tested with shorter duration stress tests.
- Tested swapping with malloc and shm by reducing max memory and
  allocating three times the available system memory by active processes
  using ADI on allocated memory. Ran through multiple hours long runs of
  this test.
- Tested page migration with malloc and shm by migrating data pages of
  active ADI test process using migratepages, back and forth between two
  nodes every few seconds over an hour long run. Verified page migration
  through /proc//numa_maps.
- Tested COW support using test that forks children that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-
Khalid Aziz (10):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata as well on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  mm: Allow arch code to override copy_highpage()
  sparc64: Add support for ADI (Application Data Integrity)

 Documentation/sparc/adi.txt | 278 ++
 arch/powerpc/include/asm/mman.h |   4 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  47 
 arch/sparc/include/asm/elf_64.h |   9 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  84 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  50 
 arch/sparc/include/asm/page_64.h|   6 +
 arch/sparc/include/asm/pgtable_64.h |  48 
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  11 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/kernel/Makefile  |   1 +
 arch/sparc/kernel/adi_64.c  | 397 
 arch/sparc/kernel/entry.h   |   3 +
 arch/sparc/kernel/etrap_64.S|  27 ++-
 arch/sparc/kernel/head_64.S |   1 +
 arch/sparc/kernel/mdesc.c   |   2 +
 arch/sparc/kernel/process_64.c  |  25 ++
 arch/sparc/kernel/rtrap_64.S|  33 ++-
 arch/sparc/kernel/setup_64.c|   2 +
 arch/sparc/kernel/sun4v_mcd.S   |  18 ++
 arch/sparc/kernel/traps_64.c| 142 +++-
 arch/sparc/kernel/ttable_64.S   |   6 +-
 arch/sparc/kernel/urtt_fill.S   |   7 +-
 arch/sparc/kernel/vmlinux.lds.S |   5 +
 arch/sparc/mm/gup.c |  37 +++
 arch/sparc/mm/hugetlbpage.c   

[PATCH v10 07/10] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v8:
- Added addr parameter to powerpc arch_validate_prot() (suggested
  by Michael Ellerman)
v7:
- new patch

 arch/powerpc/include/asm/mman.h | 4 ++--
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..1d129f4521ac 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -32,7 +32,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
vm_flags)
 }
 #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
 
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
return false;
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot arch_validate_prot
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 7c87b6652244..4d3395e41268 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -50,7 +50,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ec39f730a0bf..1e0d9cb024c8 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -410,7 +410,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0

Re: linux-next: Tree for Nov 7

[Khalid Aziz]

On Tue, 2017-11-14 at 10:04 +0100, Michal Hocko wrote:
> If there is a general consensus that this is the preferred way to go,
> I
> will post the patch as an RFC to linux-api
> 
> [1] http://lkml.kernel.org/r/20171113160637.jhekbdyfpccme3be@dhcp22.s
> use.cz

I prefer the new flag. It is cleaner and avoids unintended breakage for
existing flag.

--
Khalid

Re: linux-next: Tree for Nov 7

[Khalid Aziz]

On 11/13/2017 09:06 AM, Michal Hocko wrote:

OK, so this one should take care of the backward compatibility while
still not touching the arch code
---
commit 39ff9bf8597e79a032da0954aea1f0d77d137765
Author: Michal Hocko 
Date:   Mon Nov 13 17:06:24 2017 +0100

 mm: introduce MAP_FIXED_SAFE
 
 MAP_FIXED is used quite often but it is inherently dangerous because it

 unmaps an existing mapping covered by the requested range. While this
 might be might be really desidered behavior in many cases there are
 others which would rather see a failure than a silent memory corruption.
 Introduce a new MAP_FIXED_SAFE flag for mmap to achive this behavior.
 It is a MAP_FIXED extension with a single exception that it fails with
 ENOMEM if the requested address is already covered by an existing
 mapping. We still do rely on get_unmaped_area to handle all the arch
 specific MAP_FIXED treatment and check for a conflicting vma after it
 returns.
 
 Signed-off-by: Michal Hocko 


.. deleted ...
diff --git a/mm/mmap.c b/mm/mmap.c
index 680506faceae..aad8d37f0205 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1358,6 +1358,10 @@ unsigned long do_mmap(struct file *file, unsigned long 
addr,
if (mm->map_count > sysctl_max_map_count)
return -ENOMEM;
  
+	/* force arch specific MAP_FIXED handling in get_unmapped_area */

+   if (flags & MAP_FIXED_SAFE)
+   flags |= MAP_FIXED;
+
/* Obtain the address to map to. we verify (or select) it and ensure
 * that it represents a valid section of the address space.
 */


Do you need to move this code above:

if (!(flags & MAP_FIXED))
addr = round_hint_to_min(addr);

/* Careful about overflows.. */
len = PAGE_ALIGN(len);
if (!len)
return -ENOMEM;

Not doing that might mean the hint address will end up being rounded for 
MAP_FIXED_SAFE which would change the behavior from MAP_FIXED.


--
Khalid

[PATCH v9 00/10] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

 and PMCDPER correctly upon entry
  into kernel

Changelog v5:
- Patch 1/4: No changes
- Patch 2/4: Replaced set_swp_pte_at() with new architecture
  functions arch_do_swap_page() and arch_unmap_one() that
  suppoprt architecture specific actions to be taken on page
  swap and migration
- Patch 3/4: Fixed indentation issues in assembly code
- Patch 4/4:
- Fixed indentation issues and instrcuctions in assembly
  code
- Removed CONFIG_SPARC64 from mdesc.c
- Changed to maintain state of MCDPER register in thread
  info flags as opposed to in mm context. MCDPER is a
  per-thread state and belongs in thread info flag as
  opposed to mm context which is shared across threads.
  Added comments to clarify this is a lazily maintained
  state and must be updated on context switch and
  copy_process() 
- Updated code to use the new arch_do_swap_page() and
  arch_unmap_one() functions

Testing:

- All functionality was tested with 8K normal pages as well as hugepages
  using malloc, mmap and shm.
- Multiple long duration stress tests were run using hugepages over 2+
  months. Normal pages were tested with shorter duration stress tests.
- Tested swapping with malloc and shm by reducing max memory and
  allocating three times the available system memory by active processes
  using ADI on allocated memory. Ran through multiple hours long runs of
  this test.
- Tested page migration with malloc and shm by migrating data pages of
  active ADI test process using migratepages, back and forth between two
  nodes every few seconds over an hour long run. Verified page migration
  through /proc//numa_maps.
- Tested COW support using test that forks children that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-
Khalid Aziz (10):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata as well on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  mm: Allow arch code to override copy_highpage()
  sparc64: Add support for ADI (Application Data Integrity)

 Documentation/sparc/adi.txt | 278 ++
 arch/powerpc/include/asm/mman.h |   4 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  46 
 arch/sparc/include/asm/elf_64.h |   9 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  84 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  50 
 arch/sparc/include/asm/page_64.h|   6 +
 arch/sparc/include/asm/pgtable_64.h |  48 
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  11 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/kernel/Makefile  |   1 +
 arch/sparc/kernel/adi_64.c  | 396 
 arch/sparc/kernel/entry.h   |   3 +
 arch/sparc/kernel/etrap_64.S|  28 ++-
 arch/sparc/kernel/head_64.S |   1 +
 arch/sparc/kernel/mdesc.c   |   2 +
 arch/sparc/kernel/process_64.c  |  25 ++
 arch/sparc/kernel/setup_64.c|   2 +
 arch/sparc/kernel/sun4v_mcd.S   |  17 ++
 arch/sparc/kernel/traps_64.c| 142 +++-
 arch/sparc/kernel/ttable_64.S   |   6 +-
 arch/sparc/kernel/vmlinux.lds.S |   5 +
 arch/sparc/mm/gup.c |  37 +++
 arch/sparc/mm/hugetlbpage.c |  14 +-
 arch/sparc/mm/init_64.c |  69 ++
 arch/sparc/mm/tsb.c |  21 ++
 arch/x86/kernel/signal_compat.c |   2 +-
 include/asm-generic/pgtable.h   |  36 +++
 include/linux/highmem.h |   4 +
 include/linux/mm.h  |   9 +
 include/linux/mman.h|   2 +-
 include/uapi/asm-generic/siginfo.h  |   5 +-
 mm/ksm.c|   4 +
 mm/memory.c |   1 +
 mm/mprotect.c   |   4 +-
 mm/rmap.c   |  14 ++
 45 files changed, 1427 

[PATCH v9 07/10] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v8:
- Added addr parameter to powerpc arch_validate_prot() (suggested
  by Michael Ellerman)
v7:
- new patch

 arch/powerpc/include/asm/mman.h | 4 ++--
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..1d129f4521ac 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -32,7 +32,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
vm_flags)
 }
 #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
 
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
return false;
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot arch_validate_prot
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index c8367041fafd..b42ad5c9d6a2 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -49,7 +49,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index bd0f409922cb..4f0e46bb1797 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -395,7 +395,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0

[PATCH v8 0/9] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

ments to clarify this is a lazily maintained
  state and must be updated on context switch and
  copy_process() 
- Updated code to use the new arch_do_swap_page() and
  arch_unmap_one() functions

Testing:

- All functionality was tested with 8K normal pages as well as hugepages
  using malloc, mmap and shm.
- Multiple long duration stress tests were run using hugepages over 2+
  months. Normal pages were tested with shorter duration stress tests.
- Tested swapping with malloc and shm by reducing max memory and
  allocating three times the available system memory by active processes
  using ADI on allocated memory. Ran through multiple hours long runs of
  this test.
- Tested page migration with malloc and shm by migrating data pages of
  active ADI test process using migratepages, back and forth between two
  nodes every few seconds over an hour long run. Verified page migration
  through /proc//numa_maps.
- Tested COW support using test that forks children that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-
Khalid Aziz (9):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata as well on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  sparc64: Add support for ADI (Application Data Integrity)

 Documentation/sparc/adi.txt | 278 +++
 arch/powerpc/include/asm/mman.h |   4 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  46 
 arch/sparc/include/asm/elf_64.h |   9 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  84 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  50 +
 arch/sparc/include/asm/page_64.h|   4 +
 arch/sparc/include/asm/pgtable_64.h |  48 
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  11 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/kernel/Makefile  |   1 +
 arch/sparc/kernel/adi_64.c  | 384 
 arch/sparc/kernel/entry.h   |   3 +
 arch/sparc/kernel/etrap_64.S|  28 ++-
 arch/sparc/kernel/head_64.S |   1 +
 arch/sparc/kernel/mdesc.c   |   2 +
 arch/sparc/kernel/process_64.c  |  25 +++
 arch/sparc/kernel/setup_64.c|   2 +
 arch/sparc/kernel/sun4v_mcd.S   |  17 ++
 arch/sparc/kernel/traps_64.c| 142 +++-
 arch/sparc/kernel/ttable_64.S   |   6 +-
 arch/sparc/kernel/vmlinux.lds.S |   5 +
 arch/sparc/mm/gup.c |  37 +++
 arch/sparc/mm/hugetlbpage.c |  14 +-
 arch/sparc/mm/init_64.c |  34 +++
 arch/sparc/mm/tsb.c |  21 ++
 arch/x86/kernel/signal_compat.c |   2 +-
 include/asm-generic/pgtable.h   |  36 +++
 include/linux/mm.h  |   9 +
 include/linux/mman.h|   2 +-
 include/uapi/asm-generic/siginfo.h  |   5 +-
 mm/ksm.c|   4 +
 mm/memory.c |   1 +
 mm/mprotect.c   |   4 +-
 mm/rmap.c   |  14 ++
 44 files changed, 1374 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/sparc/adi.txt
 create mode 100644 arch/sparc/include/asm/adi.h
 create mode 100644 arch/sparc/include/asm/adi_64.h
 create mode 100644 arch/sparc/kernel/adi_64.c
 create mode 100644 arch/sparc/kernel/sun4v_mcd.S

-- 
2.11.0

[PATCH v8 7/9] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v8:
- Added addr parameter to powerpc arch_validate_prot() (suggested
  by Michael Ellerman)
v7:
- new patch

 arch/powerpc/include/asm/mman.h | 4 ++--
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..1d129f4521ac 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -32,7 +32,7 @@ static inline pgprot_t arch_vm_get_page_prot(unsigned long 
vm_flags)
 }
 #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
 
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
return false;
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot arch_validate_prot
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index c8367041fafd..b42ad5c9d6a2 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -49,7 +49,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index bd0f409922cb..4f0e46bb1797 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -395,7 +395,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0

Re: [PATCH v7 7/9] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

On 08/14/2017 11:02 PM, Michael Ellerman wrote:

Khalid Aziz  writes:


On 08/10/2017 07:20 AM, Michael Ellerman wrote:

Khalid Aziz  writes:


A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v7:
- new patch

   arch/powerpc/include/asm/mman.h | 2 +-
   arch/powerpc/kernel/syscalls.c  | 2 +-
   include/linux/mman.h| 2 +-
   mm/mprotect.c   | 2 +-
   4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..bc74074304a2 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
   }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot(prot, addr) arch_validate_prot(prot)


This can be simpler, as just:

#define arch_validate_prot arch_validate_prot



Hi Michael,

Thanks for reviewing!

My patch expands parameter list for arch_validate_prot() from one to two
parameters. Existing powerpc version of arch_validate_prot() is written
with one parameter. If I use the above #define, compilation fails with:

mm/mprotect.c: In function ‘do_mprotect_pkey’:
mm/mprotect.c:399: error: too many arguments to function
‘arch_validate_prot’

Another way to solve it would be to add the new addr parameter to
powerpc version of arch_validate_prot() but I chose the less disruptive
solution of tackling it through #define and expanded the existing
#define to include the new parameter. Make sense?


Yes, it makes sense. But it's a bit gross.

At first glance it looks like our arch_validate_prot() has an incorrect
signature.

I'd prefer you just updated it to have the correct signature, I think
you'll have to change one more line in do_mmap2(). So it's not very
intrusive.


Thanks, Michael. I can do that.

--
Khalid

Re: [PATCH v7 7/9] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

On 08/10/2017 07:20 AM, Michael Ellerman wrote:

Khalid Aziz  writes:


A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v7:
- new patch

  arch/powerpc/include/asm/mman.h | 2 +-
  arch/powerpc/kernel/syscalls.c  | 2 +-
  include/linux/mman.h| 2 +-
  mm/mprotect.c   | 2 +-
  4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..bc74074304a2 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
  }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot(prot, addr) arch_validate_prot(prot)


This can be simpler, as just:

#define arch_validate_prot arch_validate_prot



Hi Michael,

Thanks for reviewing!

My patch expands parameter list for arch_validate_prot() from one to two 
parameters. Existing powerpc version of arch_validate_prot() is written 
with one parameter. If I use the above #define, compilation fails with:


mm/mprotect.c: In function ‘do_mprotect_pkey’:
mm/mprotect.c:399: error: too many arguments to function 
‘arch_validate_prot’


Another way to solve it would be to add the new addr parameter to 
powerpc version of arch_validate_prot() but I chose the less disruptive 
solution of tackling it through #define and expanded the existing 
#define to include the new parameter. Make sense?


Thanks,
Khalid

[PATCH v7 0/9] Application Data Integrity feature introduced by SPARC M7

[Khalid Aziz]

en that read from
  ADI enabled pages shared with parent and other children and write to
  them as well forcing COW.


-
Khalid Aziz (9):
  signals, sparc: Add signal codes for ADI violations
  mm, swap: Add infrastructure for saving page metadata as well on swap
  sparc64: Add support for ADI register fields, ASIs and traps
  sparc64: Add HV fault type handlers for ADI related faults
  sparc64: Add handler for "Memory Corruption Detected" trap
  sparc64: Add auxiliary vectors to report platform ADI properties
  mm: Add address parameter to arch_validate_prot()
  mm: Clear arch specific VM flags on protection change
  sparc64: Add support for ADI (Application Data Integrity)

 Documentation/sparc/adi.txt | 272 +++
 arch/powerpc/include/asm/mman.h |   2 +-
 arch/powerpc/kernel/syscalls.c  |   2 +-
 arch/sparc/include/asm/adi.h|   6 +
 arch/sparc/include/asm/adi_64.h |  45 
 arch/sparc/include/asm/elf_64.h |   8 +
 arch/sparc/include/asm/hypervisor.h |   2 +
 arch/sparc/include/asm/mman.h   |  72 ++-
 arch/sparc/include/asm/mmu_64.h |  17 ++
 arch/sparc/include/asm/mmu_context_64.h |  43 
 arch/sparc/include/asm/page_64.h|   4 +
 arch/sparc/include/asm/pgtable_64.h |  48 +
 arch/sparc/include/asm/thread_info_64.h |   2 +-
 arch/sparc/include/asm/trap_block.h |   2 +
 arch/sparc/include/asm/ttable.h |  10 +
 arch/sparc/include/uapi/asm/asi.h   |   5 +
 arch/sparc/include/uapi/asm/auxvec.h|  10 +
 arch/sparc/include/uapi/asm/mman.h  |   2 +
 arch/sparc/include/uapi/asm/pstate.h|  10 +
 arch/sparc/kernel/Makefile  |   1 +
 arch/sparc/kernel/adi_64.c  | 367 
 arch/sparc/kernel/entry.h   |   3 +
 arch/sparc/kernel/etrap_64.S|  28 ++-
 arch/sparc/kernel/head_64.S |   1 +
 arch/sparc/kernel/mdesc.c   |   2 +
 arch/sparc/kernel/process_64.c  |  25 +++
 arch/sparc/kernel/setup_64.c|  11 +-
 arch/sparc/kernel/sun4v_mcd.S   |  17 ++
 arch/sparc/kernel/traps_64.c| 142 +++-
 arch/sparc/kernel/ttable_64.S   |   6 +-
 arch/sparc/kernel/vmlinux.lds.S |   5 +
 arch/sparc/mm/gup.c |  37 
 arch/sparc/mm/hugetlbpage.c |  14 +-
 arch/sparc/mm/init_64.c |  33 +++
 arch/sparc/mm/tsb.c |  21 ++
 arch/x86/kernel/signal_compat.c |   2 +-
 include/asm-generic/pgtable.h   |  36 
 include/linux/mm.h  |   9 +
 include/linux/mman.h|   2 +-
 include/uapi/asm-generic/siginfo.h  |   5 +-
 mm/ksm.c|   4 +
 mm/memory.c |   1 +
 mm/mprotect.c   |   4 +-
 mm/rmap.c   |  13 ++
 44 files changed, 1334 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/sparc/adi.txt
 create mode 100644 arch/sparc/include/asm/adi.h
 create mode 100644 arch/sparc/include/asm/adi_64.h
 create mode 100644 arch/sparc/kernel/adi_64.c
 create mode 100644 arch/sparc/kernel/sun4v_mcd.S

-- 
2.11.0

[PATCH v7 7/9] mm: Add address parameter to arch_validate_prot()

[Khalid Aziz]

A protection flag may not be valid across entire address space and
hence arch_validate_prot() might need the address a protection bit is
being set on to ensure it is a valid protection flag. For example, sparc
processors support memory corruption detection (as part of ADI feature)
flag on memory addresses mapped on to physical RAM but not on PFN mapped
pages or addresses mapped on to devices. This patch adds address to the
parameters being passed to arch_validate_prot() so protection bits can
be validated in the relevant context.

Signed-off-by: Khalid Aziz 
Cc: Khalid Aziz 
---
v7:
- new patch

 arch/powerpc/include/asm/mman.h | 2 +-
 arch/powerpc/kernel/syscalls.c  | 2 +-
 include/linux/mman.h| 2 +-
 mm/mprotect.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
index 30922f699341..bc74074304a2 100644
--- a/arch/powerpc/include/asm/mman.h
+++ b/arch/powerpc/include/asm/mman.h
@@ -40,7 +40,7 @@ static inline bool arch_validate_prot(unsigned long prot)
return false;
return true;
 }
-#define arch_validate_prot(prot) arch_validate_prot(prot)
+#define arch_validate_prot(prot, addr) arch_validate_prot(prot)
 
 #endif /* CONFIG_PPC64 */
 #endif /* _ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index a877bf8269fe..6d90ddbd2d11 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -48,7 +48,7 @@ static inline long do_mmap2(unsigned long addr, size_t len,
 {
long ret = -EINVAL;
 
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, addr))
goto out;
 
if (shift) {
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c51fe3a..1693d95a88ee 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -49,7 +49,7 @@ static inline void vm_unacct_memory(long pages)
  *
  * Returns true if the prot flags are valid
  */
-static inline bool arch_validate_prot(unsigned long prot)
+static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
 {
return (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM)) == 0;
 }
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 8edd0d576254..beac2dfbb5fa 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -396,7 +396,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
end = start + len;
if (end <= start)
return -ENOMEM;
-   if (!arch_validate_prot(prot))
+   if (!arch_validate_prot(prot, start))
return -EINVAL;
 
reqprot = prot;
-- 
2.11.0