Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 22:13 schrieb James Bottomley: On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote: Am 28.02.22 um 21:42 schrieb James Bottomley: On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: Am 28.02.22 um 20:56 schrieb Linus Torvalds: On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: [SNIP] Anybody have any ideas? I think we should look at the use cases why code is touching (pos) after the loop. Just from skimming over the patches to change this and experience with the drivers/subsystems I help to maintain I think the primary pattern looks something like this: list_for_each_entry(entry, head, member) { if (some_condition_checking(entry)) break; } do_something_with(entry); Actually, we usually have a check to see if the loop found anything, but in that case it should something like if (list_entry_is_head(entry, head, member)) { return with error; } do_somethin_with(entry); Suffice? The list_entry_is_head() macro is designed to cope with the bogus entry on head problem. That will work and is also what people already do. The key problem is that we let people do the same thing over and over again with slightly different implementations. Out in the wild I've seen at least using a separate variable, using a bool to indicate that something was found and just assuming that the list has an entry. The last case is bogus and basically what can break badly. Yes, I understand that. I'm saying we should replace that bogus checks of entry->something against some_value loop termination condition with the list_entry_is_head() macro. That should be a one line and fairly mechanical change rather than the explosion of code changes we seem to have in the patch series. Yes, exactly that's my thinking as well. Christian. James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 28 Feb 2022 16:41:04 -0800 Linus Torvalds wrote: > So yes, initially my idea had been to just move the iterator entirely > inside the macro. But specifying the type got so ugly that I think > that > > typeof (pos) pos > > trick inside the macro really ends up giving us the best of all worlds: > > (a) let's us keep the existing syntax and code for all the nice cases > that did everything inside the loop anyway > > (b) gives us a nice warning for any normal use-after-loop case > (unless you explicitly initialized it like that > sgx_mmu_notifier_release() function did for no good reason > > (c) also guarantees that even if you don't get a warning, > non-converted (or newly written) bad code won't actually _work_ > > so you end up getting the new rules without any ambiguity or mistaken I presume the goal is that we can do this without changing existing code? Otherwise actually moving the iterator into the loop body would be an option, by creating a different hidden variable: #define list_iter(head) \ for (struct list head *_l = (head)->next; _l != (head); _l = _l->next) #define list_iter_entry(var, member)\ list_entry(_l, typeof(*var), member) list_iter(>a_head) { struct entry *e = list_iter_entry(e, a_member); /* use e->... */ } Or we can slide into soft insanity and exploit one of Kees'es tricks to encode the type of the entries "next to" the head: #define LIST_HEAD_MEM(name, type) \ union { \ struct list_head name; \ type *name ## _entry; \ } struct entry { struct list_head a_member; }; struct parent { LIST_HEAD_MEM(a_head, struct entry); }; #define list_for_each_magic(pos, head, member) \ for (typeof(**(head ## _entry)) *pos = list_first_entry(head, typeof(**(head ## _entry)), member); \ >member != (head);\ pos = list_next_entry(pos, member)) list_for_each_magic(e, >a_head, a_member) { /* use e->... */ } I'll show myself out...
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 10:20:28AM -0800, Joe Perches wrote: > On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > > > a multi-line indent gets curly braces for readability even though > > it's not required by C. And then both sides would get curly braces. > > That's more your personal preference than a coding style guideline. > It's a USB patch. I thought Greg prefered it that way. Greg has some specific style things which he likes and I have adopted and some I pretend not to see. regards, dan carpenter
[powerpc:next-test] BUILD SUCCESS 689576dd98c57e6b35277c361af898a5be401dcc
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next-test branch HEAD: 689576dd98c57e6b35277c361af898a5be401dcc powerpc: lib: sstep: fix build errors elapsed time: 761m configs tested: 196 configs skipped: 5 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm64allyesconfig arm64 defconfig arm allyesconfig arm allmodconfig i386 randconfig-c001 i386 randconfig-c001-20220228 armlart_defconfig arm assabet_defconfig arm simpad_defconfig sh se7780_defconfig arc nsimosci_hs_smp_defconfig powerpccell_defconfig s390defconfig mips db1xxx_defconfig shecovec24-romimage_defconfig m68kmac_defconfig sh sh7710voipgw_defconfig powerpc bamboo_defconfig pariscgeneric-32bit_defconfig powerpc tqm8xx_defconfig xtensaxip_kc705_defconfig sh alldefconfig mips tb0226_defconfig powerpc iss476-smp_defconfig m68k atari_defconfig s390 allyesconfig arm omap2plus_defconfig powerpc tqm8555_defconfig mips mpc30x_defconfig armcerfcube_defconfig mips bmips_be_defconfig mips rt305x_defconfig sh rsk7269_defconfig shsh7785lcr_defconfig sh ecovec24_defconfig armmulti_v7_defconfig arc allyesconfig arm badge4_defconfig arc nsimosci_hs_defconfig sh r7780mp_defconfig sh se7724_defconfig sh sdk7780_defconfig armpleb_defconfig microblaze mmu_defconfig arm pxa3xx_defconfig m68km5307c3_defconfig m68k multi_defconfig arm gemini_defconfig powerpc makalu_defconfig m68kmvme16x_defconfig sh se7619_defconfig shsh7757lcr_defconfig powerpc motionpro_defconfig um x86_64_defconfig s390 zfcpdump_defconfig ia64defconfig powerpc taishan_defconfig openriscor1ksim_defconfig arm axm55xx_defconfig powerpc sequoia_defconfig m68k m5275evb_defconfig armmini2440_defconfig sparc64 alldefconfig sh landisk_defconfig powerpc ppc6xx_defconfig arc axs103_defconfig openrisc alldefconfig sh se7751_defconfig powerpc linkstation_defconfig armqcom_defconfig mips loongson1b_defconfig arm aspeed_g5_defconfig arcnsimosci_defconfig ia64 allmodconfig arcvdk_hs38_defconfig m68k apollo_defconfig armspear6xx_defconfig m68k amiga_defconfig arm viper_defconfig arm randconfig-c002-20220228 arm randconfig-c002-20220227 arm randconfig-c002-20220301 ia64 allyesconfig m68k allmodconfig m68kdefconfig m68k allyesconfig nios2 defconfig nds32 allnoconfig nds32 defconfig nios2allyesconfig cskydefconfig alpha defconfig alphaallyesconfig xtensa allyesconfig h8300allyesconfig arc defconfig sh allmodconfig parisc defconfig s390 allmodconfig parisc64
[powerpc:next] BUILD SUCCESS 8b91cee5eadd2021f55e6775f2d50bd56d00c217
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next branch HEAD: 8b91cee5eadd2021f55e6775f2d50bd56d00c217 powerpc/64s/hash: Make hash faults work in NMI context elapsed time: 761m configs tested: 193 configs skipped: 5 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm64allyesconfig arm64 defconfig arm allyesconfig arm allmodconfig i386 randconfig-c001 i386 randconfig-c001-20220228 armlart_defconfig arm assabet_defconfig arm simpad_defconfig sh se7780_defconfig arc nsimosci_hs_smp_defconfig powerpccell_defconfig s390defconfig mips db1xxx_defconfig shecovec24-romimage_defconfig m68kmac_defconfig sh sh7710voipgw_defconfig powerpc bamboo_defconfig pariscgeneric-32bit_defconfig powerpc tqm8xx_defconfig xtensaxip_kc705_defconfig sh alldefconfig mips tb0226_defconfig powerpc iss476-smp_defconfig m68k atari_defconfig s390 allyesconfig arm omap2plus_defconfig powerpc tqm8555_defconfig mips mpc30x_defconfig armcerfcube_defconfig mips bmips_be_defconfig mips rt305x_defconfig sh rsk7269_defconfig shsh7785lcr_defconfig sh ecovec24_defconfig armmulti_v7_defconfig arc allyesconfig arm badge4_defconfig arc nsimosci_hs_defconfig sh r7780mp_defconfig sh se7724_defconfig sh sdk7780_defconfig armpleb_defconfig microblaze mmu_defconfig arm pxa3xx_defconfig m68km5307c3_defconfig m68k multi_defconfig arm gemini_defconfig powerpc makalu_defconfig m68kmvme16x_defconfig sh se7619_defconfig shsh7757lcr_defconfig powerpc motionpro_defconfig um x86_64_defconfig s390 zfcpdump_defconfig ia64defconfig powerpc taishan_defconfig openriscor1ksim_defconfig arm axm55xx_defconfig powerpc sequoia_defconfig m68k m5275evb_defconfig armmini2440_defconfig sparc64 alldefconfig sh landisk_defconfig powerpc ppc6xx_defconfig arc axs103_defconfig openrisc alldefconfig sh se7751_defconfig powerpc linkstation_defconfig armqcom_defconfig mips loongson1b_defconfig arm aspeed_g5_defconfig arcnsimosci_defconfig ia64 allmodconfig arcvdk_hs38_defconfig m68k apollo_defconfig armspear6xx_defconfig m68k amiga_defconfig arm viper_defconfig arm randconfig-c002-20220228 arm randconfig-c002-20220227 arm randconfig-c002-20220301 ia64 allyesconfig m68k allmodconfig m68kdefconfig m68k allyesconfig nios2 defconfig nds32 allnoconfig cskydefconfig alpha defconfig nds32 defconfig alphaallyesconfig nios2allyesconfig xtensa allyesconfig h8300allyesconfig arc defconfig sh allmodconfig parisc defconfig s390 allmodconfig parisc64
[powerpc:merge] BUILD SUCCESS e314fe9c2ad65adcb62fa98376a5f35502e4f4dd
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: e314fe9c2ad65adcb62fa98376a5f35502e4f4dd Automatic merge of 'next' into merge (2022-03-01 00:35) elapsed time: 761m configs tested: 149 configs skipped: 4 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm allmodconfig arm allyesconfig arm64 defconfig arm64allyesconfig i386 randconfig-c001 armlart_defconfig arm assabet_defconfig arm simpad_defconfig sh se7780_defconfig powerpccell_defconfig mips rt305x_defconfig sh ecovec24_defconfig armmulti_v7_defconfig shsh7785lcr_defconfig sh rsk7269_defconfig shsh7757lcr_defconfig arm axm55xx_defconfig powerpc sequoia_defconfig m68k m5275evb_defconfig armmini2440_defconfig sparc64 alldefconfig sh se7619_defconfig powerpc ppc6xx_defconfig arc axs103_defconfig sh landisk_defconfig armqcom_defconfig arcnsimosci_defconfig mips loongson1b_defconfig arm aspeed_g5_defconfig xtensaxip_kc705_defconfig arcvdk_hs38_defconfig m68k apollo_defconfig armspear6xx_defconfig m68k amiga_defconfig arm viper_defconfig arm randconfig-c002-20220228 arm randconfig-c002-20220227 ia64 allmodconfig ia64defconfig ia64 allyesconfig m68kdefconfig m68k allyesconfig m68k allmodconfig nios2 defconfig arc allyesconfig nds32 allnoconfig nds32 defconfig nios2allyesconfig cskydefconfig alpha defconfig alphaallyesconfig xtensa allyesconfig h8300allyesconfig arc defconfig sh allmodconfig parisc defconfig parisc64defconfig s390 allmodconfig parisc allyesconfig s390defconfig s390 allyesconfig i386 allyesconfig i386 debian-10.3 i386 debian-10.3-kselftests i386defconfig sparc defconfig sparcallyesconfig mips allyesconfig mips allmodconfig powerpc allyesconfig powerpc allmodconfig powerpc allnoconfig x86_64randconfig-a006 x86_64randconfig-a002 x86_64randconfig-a004 x86_64 randconfig-a011-20220228 x86_64 randconfig-a015-20220228 x86_64 randconfig-a014-20220228 x86_64 randconfig-a013-20220228 x86_64 randconfig-a016-20220228 x86_64 randconfig-a012-20220228 x86_64randconfig-a013 x86_64randconfig-a011 x86_64randconfig-a015 i386 randconfig-a016-20220228 i386 randconfig-a012-20220228 i386 randconfig-a015-20220228 i386 randconfig-a011-20220228 i386 randconfig-a013-20220228 i386 randconfig-a014-20220228 s390 randconfig-r044-20220228 arc randconfig-r043-20220228 riscvrandconfig-r042-20220228 arc randconfig-r043-20220227 riscvnommu_k210_defconfig riscvallyesconfig riscvnommu_virt_defconfig riscv allnoconfig riscv defconfig riscv
RE: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Matthew Wilcox > Sent: 28 February 2022 20:16 > > On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > > > +#define list_for_each_entry(pos, head, member) > > \ > > + for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); > > \ > > +!list_entry_is_head(pos, head, member);\ > > pos = list_next_entry(pos, member)) Actually can't you use 'pos' to temporarily hold the address of 'member'. Something like: for (pos = (void *)head; \ pos ? ((pos = (void *)pos - offsetof(member)), 1) : 0; \ pos = (void *)pos->next) So that 'pos' is NULL if the loop terminates. No pointers outside structures are generated. Probably need to kill list_entry_is_head() - or it just checks for NULL. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
From: Linus Torvalds > Sent: 28 February 2022 19:56 > On Mon, Feb 28, 2022 at 4:19 AM Christian König > wrote: > > > > I don't think that using the extra variable makes the code in any way > > more reliable or easier to read. > > So I think the next step is to do the attached patch (which requires > that "-std=gnu11" that was discussed in the original thread). > > That will guarantee that the 'pos' parameter of list_for_each_entry() > is only updated INSIDE the for_each_list_entry() loop, and can never > point to the (wrongly typed) head entry. > > And I would actually hope that it should actually cause compiler > warnings about possibly uninitialized variables if people then use the > 'pos' pointer outside the loop. Except > > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for > inexplicable reasons - possibly because it already expected this > behavior > > (b) when I remove that NULL initializer, I still don't get a warning, > because we've disabled -Wno-maybe-uninitialized since it results in so > many false positives. > > Oh well. > > Anyway, give this patch a look, and at least if it's expanded to do > "(pos) = NULL" in the entry statement for the for-loop, it will avoid > the HEAD type confusion that Jakob is working on. And I think in a > cleaner way than the horrid games he plays. > > (But it won't avoid possible CPU speculation of such type confusion. > That, in my opinion, is a completely different issue) > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. > > Anybody have any ideas? > > Linus diff --git a/include/linux/list.h b/include/linux/list.h index dd6c2041d09c..bab995596aaa 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -634,10 +634,10 @@ static inline void list_splice_tail_init(struct list_head *list, * @head: the head for your list. * @member:the name of the list_head within the struct. */ -#define list_for_each_entry(pos, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member);\ -!list_entry_is_head(pos, head, member);\ -pos = list_next_entry(pos, member)) +#define list_for_each_entry(pos, head, member) \ + for (typeof(pos) __iter = list_first_entry(head, typeof(*pos), member); \ +!list_entry_is_head(__iter, head, member) && (((pos)=__iter),1); \ +__iter = list_next_entry(__iter, member)) /** * list_for_each_entry_reverse - iterate backwards over list of given type. I think you actually want: !list_entry_is_head(__iter, head, member) ? (((pos)=__iter),1) : (((pos) = NULL),0); Which can be done in the original by: !list_entry_is_head(pos, head, member) ? 1 : (((pos) = NULL), 0); Although it would be safer to have (without looking up the actual name): for (item *__item = head; \ __item ? (((pos) = list_item(__item, member)), 1) : (((pos) = NULL), 0); __item = (pos)->member) The local does need 'fixing' to avoid shadowing for nested loops. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[PATCH v6 9/9] powerpc/pseries/vas: Add 'update_total_credits' entry for QoS capabilities
pseries supports two types of credits - Default (uses normal priority FIFO) and Qality of service (QoS uses high priority FIFO). The user decides the number of QoS credits and sets this value with HMC interface. The total credits for QoS capabilities can be changed dynamically with HMC interface which invokes drmgr to communicate to the kernel. This patch creats 'update_total_credits' entry for QoS capabilities so that drmgr command can write the new target QoS credits in sysfs. Instead of using this value, the kernel gets the new QoS capabilities from the hypervisor whenever update_total_credits is updated to make sure sync with the QoS target credits in the hypervisor. Signed-off-by: Haren Myneni --- arch/powerpc/platforms/pseries/vas-sysfs.c | 54 +++--- arch/powerpc/platforms/pseries/vas.c | 2 +- arch/powerpc/platforms/pseries/vas.h | 1 + 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/platforms/pseries/vas-sysfs.c b/arch/powerpc/platforms/pseries/vas-sysfs.c index e24d3edb3021..4a7fcde5afc0 100644 --- a/arch/powerpc/platforms/pseries/vas-sysfs.c +++ b/arch/powerpc/platforms/pseries/vas-sysfs.c @@ -25,6 +25,27 @@ struct vas_caps_entry { #define to_caps_entry(entry) container_of(entry, struct vas_caps_entry, kobj) +/* + * This function is used to get the notification from the drmgr when + * QoS credits are changed. Though receiving the target total QoS + * credits here, get the official QoS capabilities from the hypervisor. + */ +static ssize_t update_total_credits_trigger(struct vas_cop_feat_caps *caps, + const char *buf, size_t count) +{ + int err; + u16 creds; + + err = kstrtou16(buf, 0, ); + if (!err) + err = vas_reconfig_capabilties(caps->win_type); + + if (err) + return -EINVAL; + + return count; +} + #define sysfs_caps_entry_read(_name) \ static ssize_t _name##_show(struct vas_cop_feat_caps *caps, char *buf) \ { \ @@ -63,17 +84,29 @@ struct vas_sysfs_entry { * changed dynamically by the user. * /sys/devices/vas/vas0/gzip/qos_capabilities/nr_used_credits * Number of credits used by the user space. + * /sys/devices/vas/vas0/gzip/qos_capabilities/update_total_credits + * Update total QoS credits dynamically */ VAS_ATTR_RO(nr_total_credits); VAS_ATTR_RO(nr_used_credits); -static struct attribute *vas_capab_attrs[] = { +static struct vas_sysfs_entry update_total_credits_attribute = + __ATTR(update_total_credits, 0200, NULL, update_total_credits_trigger); + +static struct attribute *vas_def_capab_attrs[] = { _total_credits_attribute.attr, _used_credits_attribute.attr, NULL, }; +static struct attribute *vas_qos_capab_attrs[] = { + _total_credits_attribute.attr, + _used_credits_attribute.attr, + _total_credits_attribute.attr, + NULL, +}; + static ssize_t vas_type_show(struct kobject *kobj, struct attribute *attr, char *buf) { @@ -118,19 +151,29 @@ static const struct sysfs_ops vas_sysfs_ops = { .store = vas_type_store, }; -static struct kobj_type vas_attr_type = { +static struct kobj_type vas_def_attr_type = { .release= vas_type_release, .sysfs_ops = _sysfs_ops, - .default_attrs = vas_capab_attrs, + .default_attrs = vas_def_capab_attrs, }; -static char *vas_caps_kobj_name(struct vas_cop_feat_caps *caps, +static struct kobj_type vas_qos_attr_type = { + .release= vas_type_release, + .sysfs_ops = _sysfs_ops, + .default_attrs = vas_qos_capab_attrs, +}; + +static char *vas_caps_kobj_name(struct vas_caps_entry *centry, struct kobject **kobj) { + struct vas_cop_feat_caps *caps = centry->caps; + if (caps->descriptor == VAS_GZIP_QOS_CAPABILITIES) { + kobject_init(>kobj, _qos_attr_type); *kobj = gzip_caps_kobj; return "qos_capabilities"; } else if (caps->descriptor == VAS_GZIP_DEFAULT_CAPABILITIES) { + kobject_init(>kobj, _def_attr_type); *kobj = gzip_caps_kobj; return "default_capabilities"; } else @@ -152,9 +195,8 @@ int sysfs_add_vas_caps(struct vas_cop_feat_caps *caps) if (!centry) return -ENOMEM; - kobject_init(>kobj, _attr_type); centry->caps = caps; - name = vas_caps_kobj_name(caps, ); + name = vas_caps_kobj_name(centry, ); if (kobj) { ret = kobject_add(>kobj, kobj, "%s", name); diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c index
[PATCH v6 8/9] powerpc/pseries/vas: sysfs interface to export capabilities
The hypervisor provides the available VAS GZIP capabilities such as default or QoS window type and the target available credits in each type. This patch creates sysfs entries and exports the target, used and the available credits for each feature. This interface can be used by the user space to determine the credits usage or to set the target credits in the case of QoS type (for DLPAR). /sys/devices/vas/vas0/gzip/default_capabilities (default GZIP capabilities) nr_total_credits /* Total credits available. Can be /* changed with DLPAR operation */ nr_used_credits /* Used credits */ /sys/devices/vas/vas0/gzip/qos_capabilities (QoS GZIP capabilities) nr_total_credits nr_used_credits Signed-off-by: Haren Myneni Reviewed-by: Nicholas Piggin --- arch/powerpc/platforms/pseries/Makefile| 2 +- arch/powerpc/platforms/pseries/vas-sysfs.c | 226 + arch/powerpc/platforms/pseries/vas.c | 6 + arch/powerpc/platforms/pseries/vas.h | 6 + 4 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/platforms/pseries/vas-sysfs.c diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile index ee60b59024b4..29b522d2c755 100644 --- a/arch/powerpc/platforms/pseries/Makefile +++ b/arch/powerpc/platforms/pseries/Makefile @@ -29,6 +29,6 @@ obj-$(CONFIG_PPC_SVM) += svm.o obj-$(CONFIG_FA_DUMP) += rtas-fadump.o obj-$(CONFIG_SUSPEND) += suspend.o -obj-$(CONFIG_PPC_VAS) += vas.o +obj-$(CONFIG_PPC_VAS) += vas.o vas-sysfs.o obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += cc_platform.o diff --git a/arch/powerpc/platforms/pseries/vas-sysfs.c b/arch/powerpc/platforms/pseries/vas-sysfs.c new file mode 100644 index ..e24d3edb3021 --- /dev/null +++ b/arch/powerpc/platforms/pseries/vas-sysfs.c @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright 2022-23 IBM Corp. + */ + +#define pr_fmt(fmt) "vas: " fmt + +#include +#include +#include +#include +#include +#include + +#include "vas.h" + +#ifdef CONFIG_SYSFS +static struct kobject *pseries_vas_kobj; +static struct kobject *gzip_caps_kobj; + +struct vas_caps_entry { + struct kobject kobj; + struct vas_cop_feat_caps *caps; +}; + +#define to_caps_entry(entry) container_of(entry, struct vas_caps_entry, kobj) + +#define sysfs_caps_entry_read(_name) \ +static ssize_t _name##_show(struct vas_cop_feat_caps *caps, char *buf) \ +{ \ + return sprintf(buf, "%d\n", atomic_read(>_name)); \ +} + +struct vas_sysfs_entry { + struct attribute attr; + ssize_t (*show)(struct vas_cop_feat_caps *, char *); + ssize_t (*store)(struct vas_cop_feat_caps *, const char *, size_t); +}; + +#define VAS_ATTR_RO(_name) \ + sysfs_caps_entry_read(_name); \ + static struct vas_sysfs_entry _name##_attribute = __ATTR(_name, \ + 0444, _name##_show, NULL); + +/* + * Create sysfs interface: + * /sys/devices/vas/vas0/gzip/default_capabilities + * This directory contains the following VAS GZIP capabilities + * for the defaule credit type. + * /sys/devices/vas/vas0/gzip/default_capabilities/nr_total_credits + * Total number of default credits assigned to the LPAR which + * can be changed with DLPAR operation. + * /sys/devices/vas/vas0/gzip/default_capabilities/nr_used_credits + * Number of credits used by the user space. One credit will + * be assigned for each window open. + * + * /sys/devices/vas/vas0/gzip/qos_capabilities + * This directory contains the following VAS GZIP capabilities + * for the Quality of Service (QoS) credit type. + * /sys/devices/vas/vas0/gzip/qos_capabilities/nr_total_credits + * Total number of QoS credits assigned to the LPAR. The user + * has to define this value using HMC interface. It can be + * changed dynamically by the user. + * /sys/devices/vas/vas0/gzip/qos_capabilities/nr_used_credits + * Number of credits used by the user space. + */ + +VAS_ATTR_RO(nr_total_credits); +VAS_ATTR_RO(nr_used_credits); + +static struct attribute *vas_capab_attrs[] = { + _total_credits_attribute.attr, + _used_credits_attribute.attr, + NULL, +}; + +static ssize_t vas_type_show(struct kobject *kobj, struct attribute *attr, +char *buf) +{ + struct vas_caps_entry *centry; + struct vas_cop_feat_caps *caps; + struct vas_sysfs_entry *entry; + + centry = to_caps_entry(kobj); + caps = centry->caps; + entry = container_of(attr, struct vas_sysfs_entry, attr); + + if (!entry->show) + return -EIO; + + return entry->show(caps, buf); +} + +static ssize_t vas_type_store(struct kobject *kobj, struct attribute *attr, +
[PATCH v6 7/9] powerpc/pseries/vas: Reopen windows with DLPAR core add
VAS windows can be closed in the hypervisor due to lost credits when the core is removed and the kernel gets fault for NX requests on these inactive windows. If the NX requests are issued on these inactive windows, OS gets page faults and the paste failure will be returned to the user space. If the lost credits are available later with core add, reopen these windows and set them active. Later when the OS sees page faults on these active windows, it creates mapping on the new paste address. Then the user space can continue to use these windows and send HW compression requests to NX successfully. Signed-off-by: Haren Myneni --- arch/powerpc/platforms/pseries/vas.c | 91 +++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c index a297720bcdae..96178dd58adf 100644 --- a/arch/powerpc/platforms/pseries/vas.c +++ b/arch/powerpc/platforms/pseries/vas.c @@ -565,6 +565,88 @@ static int __init get_vas_capabilities(u8 feat, enum vas_cop_feat_type type, return 0; } +/* + * VAS windows can be closed due to lost credits when the core is + * removed. So reopen them if credits are available due to DLPAR + * core add and set the window active status. When NX sees the page + * fault on the unmapped paste address, the kernel handles the fault + * by setting the remapping to new paste address if the window is + * active. + */ +static int reconfig_open_windows(struct vas_caps *vcaps, int creds) +{ + long domain[PLPAR_HCALL9_BUFSIZE] = {VAS_DEFAULT_DOMAIN_ID}; + struct vas_cop_feat_caps *caps = >caps; + struct pseries_vas_window *win = NULL, *tmp; + int rc, mv_ents = 0; + + /* +* Nothing to do if there are no closed windows. +*/ + if (!vcaps->nr_close_wins) + return 0; + + /* +* For the core removal, the hypervisor reduces the credits +* assigned to the LPAR and the kernel closes VAS windows +* in the hypervisor depends on reduced credits. The kernel +* uses LIFO (the last windows that are opened will be closed +* first) and expects to open in the same order when credits +* are available. +* For example, 40 windows are closed when the LPAR lost 2 cores +* (dedicated). If 1 core is added, this LPAR can have 20 more +* credits. It means the kernel can reopen 20 windows. So move +* 20 entries in the VAS windows lost and reopen next 20 windows. +*/ + if (vcaps->nr_close_wins > creds) + mv_ents = vcaps->nr_close_wins - creds; + + list_for_each_entry_safe(win, tmp, >list, win_list) { + if (!mv_ents) + break; + + mv_ents--; + } + + list_for_each_entry_safe_from(win, tmp, >list, win_list) { + /* +* Nothing to do on this window if it is not closed +* with VAS_WIN_NO_CRED_CLOSE +*/ + if (!(win->vas_win.status & VAS_WIN_NO_CRED_CLOSE)) + continue; + + rc = allocate_setup_window(win, (u64 *)[0], + caps->win_type); + if (rc) + return rc; + + rc = h_modify_vas_window(win); + if (rc) + goto out; + + mutex_lock(>vas_win.task_ref.mmap_mutex); + /* +* Set window status to active +*/ + win->vas_win.status &= ~VAS_WIN_NO_CRED_CLOSE; + mutex_unlock(>vas_win.task_ref.mmap_mutex); + win->win_type = caps->win_type; + if (!--vcaps->nr_close_wins) + break; + } + + return 0; +out: + /* +* Window modify HCALL failed. So close the window to the +* hypervisor and return. +*/ + free_irq_setup(win); + h_deallocate_vas_window(win->vas_win.winid); + return rc; +} + /* * The hypervisor reduces the available credits if the LPAR lost core. It * means the excessive windows should not be active and the user space @@ -673,7 +755,14 @@ static int vas_reconfig_capabilties(u8 type) * closed / reopened. Hold the vas_pseries_mutex so that the * the user space can not open new windows. */ - if (old_nr_creds > new_nr_creds) { + if (old_nr_creds < new_nr_creds) { + /* +* If the existing target credits is less than the new +* target, reopen windows if they are closed due to +* the previous DLPAR (core removal). +*/ + rc = reconfig_open_windows(vcaps, new_nr_creds - old_nr_creds); + } else { /* * # active windows is more than new LPAR available * credits. So close the excessive windows.
[PATCH v6 6/9] powerpc/pseries/vas: Close windows with DLPAR core removal
The hypervisor assigns vas credits (windows) for each LPAR based on the number of cores configured in that system. The OS is expected to release credits when cores are removed, and may allocate more when cores are added. So there is a possibility of using excessive credits (windows) in the LPAR and the hypervisor expects the system to close the excessive windows so that NX load can be equally distributed across all LPARs in the system. When the OS closes the excessive windows in the hypervisor, it sets the window status inactive and invalidates window virtual address mapping. The user space receives paste instruction failure if any NX requests are issued on the inactive window. Then the user space can use with the available open windows or retry NX requests until this window active again. This patch also adds the notifier for core removal/add to close windows in the hypervisor if the system lost credits (core removal) and reopen windows in the hypervisor when the previously lost credits are available. Signed-off-by: Haren Myneni --- arch/powerpc/include/asm/vas.h | 2 + arch/powerpc/platforms/pseries/vas.c | 207 +-- arch/powerpc/platforms/pseries/vas.h | 3 + 3 files changed, 204 insertions(+), 8 deletions(-) diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h index 27251af18c65..6baf7b9ffed4 100644 --- a/arch/powerpc/include/asm/vas.h +++ b/arch/powerpc/include/asm/vas.h @@ -34,6 +34,8 @@ */ #define VAS_WIN_ACTIVE 0x0 /* Used in platform independent */ /* vas mmap() */ +/* Window is closed in the hypervisor due to lost credit */ +#define VAS_WIN_NO_CRED_CLOSE 0x0001 /* * Get/Set bit fields diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c index 1035446f985b..a297720bcdae 100644 --- a/arch/powerpc/platforms/pseries/vas.c +++ b/arch/powerpc/platforms/pseries/vas.c @@ -370,13 +370,28 @@ static struct vas_window *vas_allocate_window(int vas_id, u64 flags, if (rc) goto out_free; - vas_user_win_add_mm_context(>vas_win.task_ref); txwin->win_type = cop_feat_caps->win_type; mutex_lock(_pseries_mutex); - list_add(>win_list, >list); + /* +* Possible to lose the acquired credit with DLPAR core +* removal after the window is opened. So if there are any +* closed windows (means with lost credits), do not give new +* window to user space. New windows will be opened only +* after the existing windows are reopened when credits are +* available. +*/ + if (!caps->nr_close_wins) { + list_add(>win_list, >list); + caps->nr_open_windows++; + mutex_unlock(_pseries_mutex); + vas_user_win_add_mm_context(>vas_win.task_ref); + return >vas_win; + } mutex_unlock(_pseries_mutex); - return >vas_win; + put_vas_user_win_ref(>vas_win.task_ref); + rc = -EBUSY; + pr_err("No credit is available to allocate window\n"); out_free: /* @@ -439,14 +454,24 @@ static int vas_deallocate_window(struct vas_window *vwin) caps = [win->win_type].caps; mutex_lock(_pseries_mutex); - rc = deallocate_free_window(win); - if (rc) { - mutex_unlock(_pseries_mutex); - return rc; - } + /* +* VAS window is already closed in the hypervisor when +* lost the credit. So just remove the entry from +* the list, remove task references and free vas_window +* struct. +*/ + if (win->vas_win.status & VAS_WIN_NO_CRED_CLOSE) { + rc = deallocate_free_window(win); + if (rc) { + mutex_unlock(_pseries_mutex); + return rc; + } + } else + vascaps[win->win_type].nr_close_wins--; list_del(>win_list); atomic_dec(>nr_used_credits); + vascaps[win->win_type].nr_open_windows--; mutex_unlock(_pseries_mutex); put_vas_user_win_ref(>task_ref); @@ -501,6 +526,7 @@ static int __init get_vas_capabilities(u8 feat, enum vas_cop_feat_type type, memset(vcaps, 0, sizeof(*vcaps)); INIT_LIST_HEAD(>list); + vcaps->feat = feat; caps = >caps; rc = h_query_vas_capabilities(H_QUERY_VAS_CAPABILITIES, feat, @@ -539,6 +565,168 @@ static int __init get_vas_capabilities(u8 feat, enum vas_cop_feat_type type, return 0; } +/* + * The hypervisor reduces the available credits if the LPAR lost core. It + * means the excessive windows should not be active and the user space + * should not be using these windows to send compression requests to NX. + * So the kernel closes the excessive windows and unmap the paste address + * such that the user space receives paste instruction failure. Then up to + *
[PATCH v6 5/9] powerpc/vas: Map paste address only if window is active
The paste address mapping is done with mmap() after the window is opened with ioctl. The partition has to close VAS windows in the hypervisor if it lost credits due to DLPAR core removal. But the kernel marks these windows inactive until the previously lost credits are available later. If the window is inactive due to DLPAR after this mmap(), the paste instruction returns failure until the the OS reopens this window again. Before the user space issuing mmap(), there is a possibility of happening DLPAR core removal event which causes the corresponding window inactive. So if the window is not active, return mmap() failure with -EACCES and expects the user space reissue mmap() when the window is active or open a new window when the credit is available. Signed-off-by: Haren Myneni --- arch/powerpc/platforms/book3s/vas-api.c | 23 ++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c index 82f32781c5d2..f9a1615b74da 100644 --- a/arch/powerpc/platforms/book3s/vas-api.c +++ b/arch/powerpc/platforms/book3s/vas-api.c @@ -497,10 +497,29 @@ static int coproc_mmap(struct file *fp, struct vm_area_struct *vma) return -EACCES; } + /* +* The initial mmap is done after the window is opened +* with ioctl. But before mmap(), this window can be closed in +* the hypervisor due to lost credit (core removal on pseries). +* So if the window is not active, return mmap() failure with +* -EACCES and expects the user space reissue mmap() when it +* is active again or open new window when the credit is available. +* mmap_mutex protects the paste address mmap() with DLPAR +* close/open event and allows mmap() only when the window is +* active. +*/ + mutex_lock(>task_ref.mmap_mutex); + if (txwin->status != VAS_WIN_ACTIVE) { + pr_err("%s(): Window is not active\n", __func__); + rc = -EACCES; + goto out; + } + paste_addr = cp_inst->coproc->vops->paste_addr(txwin); if (!paste_addr) { pr_err("%s(): Window paste address failed\n", __func__); - return -EINVAL; + rc = -EINVAL; + goto out; } pfn = paste_addr >> PAGE_SHIFT; @@ -520,6 +539,8 @@ static int coproc_mmap(struct file *fp, struct vm_area_struct *vma) txwin->task_ref.vma = vma; vma->vm_ops = _vm_ops; +out: + mutex_unlock(>task_ref.mmap_mutex); return rc; } -- 2.27.0
[PATCH v6 4/9] powerpc/vas: Return paste instruction failure if no active window
The VAS window may not be active if the system looses credits and the NX generates page fault when it receives request on unmap paste address. The kernel handles the fault by remap new paste address if the window is active again, Otherwise return the paste instruction failure if the executed instruction that caused the fault was a paste. Signed-off-by: Nicholas Piggin Signed-off-by: Haren Myneni --- arch/powerpc/include/asm/ppc-opcode.h | 2 + arch/powerpc/platforms/book3s/vas-api.c | 54 + 2 files changed, 56 insertions(+) diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h index 9675303b724e..82f1f0041c6f 100644 --- a/arch/powerpc/include/asm/ppc-opcode.h +++ b/arch/powerpc/include/asm/ppc-opcode.h @@ -262,6 +262,8 @@ #define PPC_INST_MFSPR_PVR 0x7c1f42a6 #define PPC_INST_MFSPR_PVR_MASK0xfc1e #define PPC_INST_MTMSRD0x7c000164 +#define PPC_INST_PASTE 0x7c20070d +#define PPC_INST_PASTE_MASK0xfc2007ff #define PPC_INST_POPCNTB 0x7cf4 #define PPC_INST_POPCNTB_MASK 0xfc0007fe #define PPC_INST_RFEBB 0x4c000124 diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c index 217b4a624d09..82f32781c5d2 100644 --- a/arch/powerpc/platforms/book3s/vas-api.c +++ b/arch/powerpc/platforms/book3s/vas-api.c @@ -351,6 +351,41 @@ static int coproc_release(struct inode *inode, struct file *fp) return 0; } +/* + * If the executed instruction that caused the fault was a paste, then + * clear regs CR0[EQ], advance NIP, and return 0. Else return error code. + */ +static int do_fail_paste(void) +{ + struct pt_regs *regs = current->thread.regs; + u32 instword; + + if (WARN_ON_ONCE(!regs)) + return -EINVAL; + + if (WARN_ON_ONCE(!user_mode(regs))) + return -EINVAL; + + /* +* If we couldn't translate the instruction, the driver should +* return success without handling the fault, it will be retried +* or the instruction fetch will fault. +*/ + if (get_user(instword, (u32 __user *)(regs->nip))) + return -EAGAIN; + + /* +* Not a paste instruction, driver may fail the fault. +*/ + if ((instword & PPC_INST_PASTE_MASK) != PPC_INST_PASTE) + return -ENOENT; + + regs->ccr &= ~0xe000; /* Clear CR0[0-2] to fail paste */ + regs_add_return_ip(regs, 4);/* Emulate the paste */ + + return 0; +} + /* * This fault handler is invoked when the core generates page fault on * the paste address. Happens if the kernel closes window in hypervisor @@ -364,6 +399,7 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf) struct vas_window *txwin; vm_fault_t fault; u64 paste_addr; + int ret; /* * window is not opened. Shouldn't expect this error. @@ -408,6 +444,24 @@ static vm_fault_t vas_mmap_fault(struct vm_fault *vmf) } mutex_unlock(>task_ref.mmap_mutex); + /* +* Received this fault due to closing the actual window. +* It can happen during migration or lost credits. +* Since no mapping, return the paste instruction failure +* to the user space. +*/ + ret = do_fail_paste(); + /* +* The user space can retry several times until success (needed +* for migration) or should fallback to SW compression or +* manage with the existing open windows if available. +* Looking at sysfs interface, it can determine whether these +* failures are coming during migration or core removal: +* nr_used_credits > nr_total_credits when lost credits +*/ + if (!ret || (ret == -EAGAIN)) + return VM_FAULT_NOPAGE; + return VM_FAULT_SIGBUS; } -- 2.27.0
[PATCH v6 3/9] powerpc/vas: Add paste address mmap fault handler
The user space opens VAS windows and issues NX requests by pasting CRB on the corresponding paste address mmap. When the system lost credits due to core removal, the kernel has to close the window in the hypervisor and make the window inactive by unmapping this paste address. Also the OS has to handle NX request page faults if the user space issue NX requests. This handler maps the new paste address with the same VMA when the window is active again (due to core add with DLPAR). Otherwise returns paste failure. Signed-off-by: Haren Myneni Reviewed-by: Nicholas Piggin --- arch/powerpc/include/asm/vas.h | 10 arch/powerpc/platforms/book3s/vas-api.c | 68 + 2 files changed, 78 insertions(+) diff --git a/arch/powerpc/include/asm/vas.h b/arch/powerpc/include/asm/vas.h index 57573d9c1e09..27251af18c65 100644 --- a/arch/powerpc/include/asm/vas.h +++ b/arch/powerpc/include/asm/vas.h @@ -29,6 +29,12 @@ #define VAS_THRESH_FIFO_GT_QTR_FULL2 #define VAS_THRESH_FIFO_GT_EIGHTH_FULL 3 +/* + * VAS window Linux status bits + */ +#define VAS_WIN_ACTIVE 0x0 /* Used in platform independent */ + /* vas mmap() */ + /* * Get/Set bit fields */ @@ -59,6 +65,9 @@ struct vas_user_win_ref { struct pid *pid;/* PID of owner */ struct pid *tgid; /* Thread group ID of owner */ struct mm_struct *mm; /* Linux process mm_struct */ + struct mutex mmap_mutex;/* protects paste address mmap() */ + /* with DLPAR close/open windows */ + struct vm_area_struct *vma; /* Save VMA and used in DLPAR ops */ }; /* @@ -67,6 +76,7 @@ struct vas_user_win_ref { struct vas_window { u32 winid; u32 wcreds_max; /* Window credits */ + u32 status; /* Window status used in OS */ enum vas_cop_type cop; struct vas_user_win_ref task_ref; char *dbgname; diff --git a/arch/powerpc/platforms/book3s/vas-api.c b/arch/powerpc/platforms/book3s/vas-api.c index 4d82c92ddd52..217b4a624d09 100644 --- a/arch/powerpc/platforms/book3s/vas-api.c +++ b/arch/powerpc/platforms/book3s/vas-api.c @@ -316,6 +316,7 @@ static int coproc_ioc_tx_win_open(struct file *fp, unsigned long arg) return PTR_ERR(txwin); } + mutex_init(>task_ref.mmap_mutex); cp_inst->txwin = txwin; return 0; @@ -350,6 +351,70 @@ static int coproc_release(struct inode *inode, struct file *fp) return 0; } +/* + * This fault handler is invoked when the core generates page fault on + * the paste address. Happens if the kernel closes window in hypervisor + * (on pseries) due to lost credit or the paste address is not mapped. + */ +static vm_fault_t vas_mmap_fault(struct vm_fault *vmf) +{ + struct vm_area_struct *vma = vmf->vma; + struct file *fp = vma->vm_file; + struct coproc_instance *cp_inst = fp->private_data; + struct vas_window *txwin; + vm_fault_t fault; + u64 paste_addr; + + /* +* window is not opened. Shouldn't expect this error. +*/ + if (!cp_inst || !cp_inst->txwin) { + pr_err("%s(): Unexpected fault on paste address with TX window closed\n", + __func__); + return VM_FAULT_SIGBUS; + } + + txwin = cp_inst->txwin; + /* +* When the LPAR lost credits due to core removal or during +* migration, invalidate the existing mapping for the current +* paste addresses and set windows in-active (zap_page_range in +* reconfig_close_windows()). +* New mapping will be done later after migration or new credits +* available. So continue to receive faults if the user space +* issue NX request. +*/ + if (txwin->task_ref.vma != vmf->vma) { + pr_err("%s(): No previous mapping with paste address\n", + __func__); + return VM_FAULT_SIGBUS; + } + + mutex_lock(>task_ref.mmap_mutex); + /* +* The window may be inactive due to lost credit (Ex: core +* removal with DLPAR). If the window is active again when +* the credit is available, map the new paste address at the +* the window virtual address. +*/ + if (txwin->status == VAS_WIN_ACTIVE) { + paste_addr = cp_inst->coproc->vops->paste_addr(txwin); + if (paste_addr) { + fault = vmf_insert_pfn(vma, vma->vm_start, + (paste_addr >> PAGE_SHIFT)); + mutex_unlock(>task_ref.mmap_mutex); + return fault; + } + } + mutex_unlock(>task_ref.mmap_mutex); + + return VM_FAULT_SIGBUS; +} + +static const struct vm_operations_struct vas_vm_ops = { + .fault = vas_mmap_fault, +}; + static int
[PATCH v6 2/9] powerpc/pseries/vas: Save PID in pseries_vas_window struct
The kernel sets the VAS window with PID when it is opened in the hypervisor. During DLPAR operation, windows can be closed and reopened in the hypervisor when the credit is available. So saves this PID in pseries_vas_window struct when the window is opened initially and reuse it later during DLPAR operation. Signed-off-by: Haren Myneni Reviewed-by: Nicholas Piggin --- arch/powerpc/platforms/pseries/vas.c | 9 + arch/powerpc/platforms/pseries/vas.h | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c index 18aae037ffe9..1035446f985b 100644 --- a/arch/powerpc/platforms/pseries/vas.c +++ b/arch/powerpc/platforms/pseries/vas.c @@ -107,7 +107,6 @@ static int h_deallocate_vas_window(u64 winid) static int h_modify_vas_window(struct pseries_vas_window *win) { long rc; - u32 lpid = mfspr(SPRN_PID); /* * AMR value is not supported in Linux VAS implementation. @@ -115,7 +114,7 @@ static int h_modify_vas_window(struct pseries_vas_window *win) */ do { rc = plpar_hcall_norets(H_MODIFY_VAS_WINDOW, - win->vas_win.winid, lpid, 0, + win->vas_win.winid, win->pid, 0, VAS_MOD_WIN_FLAGS, 0); rc = hcall_return_busy_check(rc); @@ -124,8 +123,8 @@ static int h_modify_vas_window(struct pseries_vas_window *win) if (rc == H_SUCCESS) return 0; - pr_err("H_MODIFY_VAS_WINDOW error: %ld, winid %u lpid %u\n", - rc, win->vas_win.winid, lpid); + pr_err("H_MODIFY_VAS_WINDOW error: %ld, winid %u pid %u\n", + rc, win->vas_win.winid, win->pid); return -EIO; } @@ -338,6 +337,8 @@ static struct vas_window *vas_allocate_window(int vas_id, u64 flags, } } + txwin->pid = mfspr(SPRN_PID); + /* * Allocate / Deallocate window hcalls and setup / free IRQs * have to be protected with mutex. diff --git a/arch/powerpc/platforms/pseries/vas.h b/arch/powerpc/platforms/pseries/vas.h index d6ea8ab8b07a..2872532ed72a 100644 --- a/arch/powerpc/platforms/pseries/vas.h +++ b/arch/powerpc/platforms/pseries/vas.h @@ -114,6 +114,7 @@ struct pseries_vas_window { u64 domain[6]; /* Associativity domain Ids */ /* this window is allocated */ u64 util; + u32 pid;/* PID associated with this window */ /* List of windows opened which is used for LPM */ struct list_head win_list; -- 2.27.0
[PATCH v6 1/9] powerpc/pseries/vas: Use common names in VAS capability structure
nr_total/nr_used_credits provides credits usage to user space via sysfs and the same interface can be used on PowerNV in future. Changed with proper naming so that applicable on both pseries and PowerNV. Signed-off-by: Haren Myneni Reviewed-by: Nicholas Piggin --- arch/powerpc/platforms/pseries/vas.c | 10 +- arch/powerpc/platforms/pseries/vas.h | 5 ++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c index d243ddc58827..18aae037ffe9 100644 --- a/arch/powerpc/platforms/pseries/vas.c +++ b/arch/powerpc/platforms/pseries/vas.c @@ -310,8 +310,8 @@ static struct vas_window *vas_allocate_window(int vas_id, u64 flags, cop_feat_caps = >caps; - if (atomic_inc_return(_feat_caps->used_lpar_creds) > - atomic_read(_feat_caps->target_lpar_creds)) { + if (atomic_inc_return(_feat_caps->nr_used_credits) > + atomic_read(_feat_caps->nr_total_credits)) { pr_err("Credits are not available to allocate window\n"); rc = -EINVAL; goto out; @@ -385,7 +385,7 @@ static struct vas_window *vas_allocate_window(int vas_id, u64 flags, free_irq_setup(txwin); h_deallocate_vas_window(txwin->vas_win.winid); out: - atomic_dec(_feat_caps->used_lpar_creds); + atomic_dec(_feat_caps->nr_used_credits); kfree(txwin); return ERR_PTR(rc); } @@ -445,7 +445,7 @@ static int vas_deallocate_window(struct vas_window *vwin) } list_del(>win_list); - atomic_dec(>used_lpar_creds); + atomic_dec(>nr_used_credits); mutex_unlock(_pseries_mutex); put_vas_user_win_ref(>task_ref); @@ -521,7 +521,7 @@ static int __init get_vas_capabilities(u8 feat, enum vas_cop_feat_type type, } caps->max_lpar_creds = be16_to_cpu(hv_caps->max_lpar_creds); caps->max_win_creds = be16_to_cpu(hv_caps->max_win_creds); - atomic_set(>target_lpar_creds, + atomic_set(>nr_total_credits, be16_to_cpu(hv_caps->target_lpar_creds)); if (feat == VAS_GZIP_DEF_FEAT) { caps->def_lpar_creds = be16_to_cpu(hv_caps->def_lpar_creds); diff --git a/arch/powerpc/platforms/pseries/vas.h b/arch/powerpc/platforms/pseries/vas.h index 4ecb3fcabd10..d6ea8ab8b07a 100644 --- a/arch/powerpc/platforms/pseries/vas.h +++ b/arch/powerpc/platforms/pseries/vas.h @@ -72,9 +72,8 @@ struct vas_cop_feat_caps { }; /* Total LPAR available credits. Can be different from max LPAR */ /* credits due to DLPAR operation */ - atomic_ttarget_lpar_creds; - atomic_tused_lpar_creds; /* Used credits so far */ - u16 avail_lpar_creds; /* Remaining available credits */ + atomic_tnr_total_credits; /* Total credits assigned to LPAR */ + atomic_tnr_used_credits;/* Used credits so far */ }; /* -- 2.27.0
[PATCH v6 0/9] powerpc/pseries/vas: NXGZIP support with DLPAR
PowerPC provides HW compression with NX coprocessor. This feature is available on both PowerNV and PowerVM and included in Linux. Since each powerpc chip has one NX coprocessor, the VAS introduces the concept of windows / credits to manage access to this hardware resource. On powerVM, these limited resources should be available across all LPARs. So the hypervisor assigns the specific credits to each LPAR based on processor entitlement so that one LPAR does not overload NX. The hypervisor can reject the window open request to a partition if exceeds its credit limit (1 credit per window). So the total number of target credits in a partition can be changed if the core configuration is modified. The hypervisor expects the partition to modify its window usage depends on new target credits. For example, if the partition uses more credits than the new target credits, it should close the excessive windows so that the NX resource will be available to other partitions. This patch series enables OS to support this dynamic credit management with DLPAR core removal/add. Core removal operation: - Get new VAS capabilities from the hypervisor when the DLPAR notifier is received. This capabilities provides the new target credits based on new processor entitlement. In the case of QoS credit changes, the notification will be issued by updating the target_creds via sysfs. - If the partition is already used more than the new target credits, the kernel selects windows, unmap the current paste address and close them in the hypervisor, It uses FIFO to identify these windows - last windows that are opened are the first ones to be closed. - When the user space issue requests on these windows, NX generates page fault on the unmap paste address. The kernel handles the fault by returning the paste instruction failure if the window is not active (means unmap paste). Then up to the library / user space to fall back to SW compression or manage with the current windows. Core add operation: - The kernel can see increased target credits from the new VAS capabilities. - Scans the window list for the closed windows in the hypervisor due to lost credit before and selects windows based on same FIFO. - Make these corresponding windows active and create remap with the same VMA on the new paste address in the fault handler. - Then the user space should expect paste successful later. Patch 1: Define common names for sysfs target/used/avail_creds so that same sysfs entries can be used even on PowerNV later. Patch 2: Save PID in the vas window struct during initial window open and use it when reopen later. Patch 3: Add new mmap fault handler which handles the page fault from NX on paste address. Patch 4: Return the paste instruction failure if the window is not active. Patch 5: If the window is closed in the hypervisor before the user space issue the initial mmap(), return -EACCES failure. Patch 6: Close windows in the hypervisor when the partition exceeds its usage than the new target credits. Patch 7: When credits are available, reopen windows that are closed before with core removal. Patch 8 & 9: The user space determines the credit usage with sysfs nr_total/nr_used_credits interfaces. drmgr uses update_total_credits to notify OS for QoS credit changes. Thanks to Nicholas Piggin and Aneesh Kumar for the valuable suggestions on the NXGZIP design to support DLPAR operations. Changes in v2: - Rebase on 5.16-rc5 - Use list safe functions to iterate windows list - Changes to show the actual value in sysfs used_credits even though some windows are inactive with core removal. Reflects -ve value in sysfs avail_creds to let userspace know that it opened more windows than the current maximum LPAR credits. Changes in v3: - Rebase on 5.16 - Reconfigure VAS windows only for CPU hotplug events. Changes in v4: - Rebase on 5.17-rc4 - Changes based on comments from Nicholas Piggin - Included VAS DLPAR notifer code in 'Close windows with DLPAR' patch instead of as a separate patch - Patches reordering and other changes Changes in v5: - Rebase on 5.17-rc5 - Add update_total_credits sysfs entry to update QoS target credits and other commit descriptions as suggested by Nicholas Piggin Changed in v6: - Build fix in "Add paste address mmap fault handler" patch as reported by kernel test robot Haren Myneni (9): powerpc/pseries/vas: Use common names in VAS capability structure powerpc/pseries/vas: Save PID in pseries_vas_window struct powerpc/vas: Add paste address mmap fault handler powerpc/vas: Return paste instruction failure if no active window powerpc/vas: Map paste address only if window is active powerpc/pseries/vas: Close windows with DLPAR core removal powerpc/pseries/vas: Reopen windows with DLPAR core add powerpc/pseries/vas: sysfs interface to export capabilities powerpc/pseries/vas: Add
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds wrote: > > Yeah, except that's ugly beyond belief, plus it's literally not what > we do in the kernel. (Of course, I probably shouldn't have used 'min()' as an example, because that is actually one of the few places where we do exactly that, using our __UNIQUE_ID() macros. Exactly because people _have_ tried to do -Wshadow when doing W=2). Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:38 PM Segher Boessenkool wrote: > > In C its scope is the rest of the declaration and the entire loop, not > anything after it. This was the same in C++98 already, btw (but in > pre-standard versions of C++ things were like you remember, yes, and it > was painful). Yeah, the original C++ model was just unadulterated garbage, with no excuse for it, and the scope was not the loop, but the block the loop existed in. That would never have been acceptable for the kernel - it's basically just an even uglier version of "put variable declarations in the middle of code" (and we use "-Wdeclaration-after-statement" to disallow that for kernel code, although apparently some of our user space tooling code doesn't enforce or follow that rule). The actual C99 version is the sane one which actually makes it easier and clearer to have loop iterators that are clearly just in loop scope. That's a good idea in general, and I have wanted to start using that in the kernel even aside from some of the loop construct macros. Because putting variables in natural minimal scope is a GoodThing(tm). Of course, we shouldn't go crazy with it. Even after we do that -std=gnu11 thing, we'll have backports to worry about. And it's not clear that we necessarily want to backport that gnu11 thing - since people who run old stable kernels also may be still using those really old compilers... Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox wrote: > > #define ___PASTE(a, b) a##b > #define __PASTE(a, b) ___PASTE(a, b) > #define _min(a, b, u) ({ \ Yeah, except that's ugly beyond belief, plus it's literally not what we do in the kernel. Really. The "-Wshadow doesn't work on the kernel" is not some new issue, because you have to do completely insane things to the source code to enable it. Just compare your uglier-than-sin version to my straightforward one. One does the usual and obvious "use a private variable to avoid the classic multi-use of a macro argument". And the other one is an abomination. Linus
[PATCH] powerpc: declare unmodified attribute_group usages const
>From ec1a16a15a86c6224cc0129ab3c2ae9f69f2c7c5 Mon Sep 17 00:00:00 2001 From: Rohan McLure Date: Mon, 28 Feb 2022 10:19:19 +1100 Subject: [PATCH] powerpc: declare unmodified attribute_group usages const To: linuxppc-dev@lists.ozlabs.org Inspired by (bd75b4ef4977: Constify static attribute_group structs), accepted by linux-next, reported: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220210202805.7750-4-rikard.falkeb...@gmail.com/ Nearly all singletons of type struct attribute_group are never modified, and so are candidates for being const. Declare them as const. Signed-off-by: Rohan McLure --- arch/powerpc/perf/generic-compat-pmu.c | 4 ++-- arch/powerpc/perf/hv-24x7.c | 6 +++--- arch/powerpc/perf/hv-gpci.c | 8 arch/powerpc/perf/imc-pmu.c | 6 +++--- arch/powerpc/perf/isa207-common.c | 2 +- arch/powerpc/perf/power10-pmu.c | 6 +++--- arch/powerpc/perf/power7-pmu.c | 4 ++-- arch/powerpc/perf/power8-pmu.c | 4 ++-- arch/powerpc/perf/power9-pmu.c | 6 +++--- arch/powerpc/platforms/cell/cbe_thermal.c | 4 ++-- arch/powerpc/platforms/powernv/opal-core.c | 2 +- arch/powerpc/platforms/powernv/opal-dump.c | 2 +- arch/powerpc/platforms/powernv/opal-flash.c | 2 +- arch/powerpc/platforms/pseries/papr_scm.c | 2 +- arch/powerpc/platforms/pseries/power.c | 2 +- 15 files changed, 30 insertions(+), 30 deletions(-) diff --git a/arch/powerpc/perf/generic-compat-pmu.c b/arch/powerpc/perf/generic-compat-pmu.c index b6e25f75109d..f3db88aee4dd 100644 --- a/arch/powerpc/perf/generic-compat-pmu.c +++ b/arch/powerpc/perf/generic-compat-pmu.c @@ -130,7 +130,7 @@ static struct attribute *generic_compat_events_attr[] = { NULL }; -static struct attribute_group generic_compat_pmu_events_group = { +static const struct attribute_group generic_compat_pmu_events_group = { .name = "events", .attrs = generic_compat_events_attr, }; @@ -146,7 +146,7 @@ static struct attribute *generic_compat_pmu_format_attr[] = { NULL, }; -static struct attribute_group generic_compat_pmu_format_group = { +static const struct attribute_group generic_compat_pmu_format_group = { .name = "format", .attrs = generic_compat_pmu_format_attr, }; diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c index 1e8aa934e37e..12c1777187fc 100644 --- a/arch/powerpc/perf/hv-24x7.c +++ b/arch/powerpc/perf/hv-24x7.c @@ -204,7 +204,7 @@ static struct attribute *format_attrs[] = { NULL, }; -static struct attribute_group format_group = { +static const struct attribute_group format_group = { .name = "format", .attrs = format_attrs, }; @@ -1148,7 +1148,7 @@ static struct attribute *cpumask_attrs[] = { NULL, }; -static struct attribute_group cpumask_attr_group = { +static const struct attribute_group cpumask_attr_group = { .attrs = cpumask_attrs, }; @@ -1162,7 +1162,7 @@ static struct attribute *if_attrs[] = { NULL, }; -static struct attribute_group if_group = { +static const struct attribute_group if_group = { .name = "interface", .bin_attrs = if_bin_attrs, .attrs = if_attrs, diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c index c756228a081f..5eb60ed5b5e8 100644 --- a/arch/powerpc/perf/hv-gpci.c +++ b/arch/powerpc/perf/hv-gpci.c @@ -65,12 +65,12 @@ static struct attribute *format_attrs[] = { NULL, }; -static struct attribute_group format_group = { +static const struct attribute_group format_group = { .name = "format", .attrs = format_attrs, }; -static struct attribute_group event_group = { +static const struct attribute_group event_group = { .name = "events", .attrs = hv_gpci_event_attrs, }; @@ -126,11 +126,11 @@ static struct attribute *cpumask_attrs[] = { NULL, }; -static struct attribute_group cpumask_attr_group = { +static const struct attribute_group cpumask_attr_group = { .attrs = cpumask_attrs, }; -static struct attribute_group interface_group = { +static const struct attribute_group interface_group = { .name = "interface", .attrs = interface_attrs, }; diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c index e106909ff9c3..70981a321036 100644 --- a/arch/powerpc/perf/imc-pmu.c +++ b/arch/powerpc/perf/imc-pmu.c @@ -71,7 +71,7 @@ static struct attribute *imc_format_attrs[] = { NULL, }; -static struct attribute_group imc_format_group = { +static const struct attribute_group imc_format_group = { .name = "format", .attrs = imc_format_attrs, }; @@ -90,7 +90,7 @@ static struct attribute *trace_imc_format_attrs[] = { NULL, }; -static struct attribute_group trace_imc_format_group = { +static const struct attribute_group trace_imc_format_group = { .name = "format", .attrs = trace_imc_format_attrs, }; @@
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 05:28:58PM -0500, James Bottomley wrote: > On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote: > > > > On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley < > > james.bottom...@hansenpartnership.com> wrote: > > > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: > [...] > > > > > I do wish we could actually poison the 'pos' value after the > > > > > loop somehow - but clearly the "might be uninitialized" I was > > > > > hoping for isn't the way to do it. > > > > > > > > > > Anybody have any ideas? > > > > > > > > I think we should look at the use cases why code is touching > > > > (pos) after the loop. > > > > > > > > Just from skimming over the patches to change this and experience > > > > with the drivers/subsystems I help to maintain I think the > > > > primary pattern looks something like this: > > > > > > > > list_for_each_entry(entry, head, member) { > > > > if (some_condition_checking(entry)) > > > > break; > > > > } > > > > do_something_with(entry); > > > > > > Actually, we usually have a check to see if the loop found > > > anything, but in that case it should something like > > > > > > if (list_entry_is_head(entry, head, member)) { > > >return with error; > > > } > > > do_somethin_with(entry); > > > > > > Suffice? The list_entry_is_head() macro is designed to cope with > > > the bogus entry on head problem. > > > > Won't suffice because the end goal of this work is to limit scope of > > entry only to loop. Hence the need for additional variable. > > Well, yes, but my objection is more to the size of churn than the > desire to do loop local. I'm not even sure loop local is possible, > because it's always annoyed me that for (int i = 0; ... in C++ defines > i in the outer scope not the loop scope, which is why I never use it. In C its scope is the rest of the declaration and the entire loop, not anything after it. This was the same in C++98 already, btw (but in pre-standard versions of C++ things were like you remember, yes, and it was painful). Segher
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel wrote: > > The goal of this is to get compiler warnings right? This would indeed be > great. Yes, so I don't mind having a one-time patch that has been gathered using some automated checker tool, but I don't think that works from a long-term maintenance perspective. So if we have the basic rule being "don't use the loop iterator after the loop has finished, because it can cause all kinds of subtle issues", then in _addition_ to fixing the existing code paths that have this issue, I really would want to (a) get a compiler warning for future cases and (b) make it not actually _work_ for future cases. Because otherwise it will just happen again. > Changing the list_for_each_entry() macro first will break all of those cases > (e.g. the ones using 'list_entry_is_head()). So I have no problems with breaking cases that we basically already have a patch for due to your automated tool. There were certainly more than a handful, but it didn't look _too_ bad to just make the rule be "don't use the iterator after the loop". Of course, that's just based on that patch of yours. Maybe there are a ton of other cases that your patch didn't change, because they didn't match your trigger case, so I may just be overly optimistic here. But basically to _me_, the important part is that the end result is maintainable longer-term. I'm more than happy to have a one-time patch to fix a lot of dubious cases if we can then have clean rules going forward. > I assumed it is better to fix those cases first and then have a simple > coccinelle script changing the macro + moving the iterator into the scope > of the macro. So that had been another plan of mine, until I actually looked at changing the macro. In the one case I looked at, it was ugly beyond belief. It turns out that just syntactically, it's really nice to give the type of the iterator from outside the way we do now. Yeah, it may be a bit odd, and maybe it's partly because I'm so used to the "list_for_each_list_entry()" syntax, but moving the type into the loop construct really made it nasty - either one very complex line, or having to split it over two lines which was even worse. Maybe the place I looked at just happened to have a long typename, but it's basically always going to be a struct, so it's never a _simple_ type. And it just looked very odd adn unnatural to have the type as one of the "arguments" to that list_for_each_entry() macro. So yes, initially my idea had been to just move the iterator entirely inside the macro. But specifying the type got so ugly that I think that typeof (pos) pos trick inside the macro really ends up giving us the best of all worlds: (a) let's us keep the existing syntax and code for all the nice cases that did everything inside the loop anyway (b) gives us a nice warning for any normal use-after-loop case (unless you explicitly initialized it like that sgx_mmu_notifier_release() function did for no good reason (c) also guarantees that even if you don't get a warning, non-converted (or newly written) bad code won't actually _work_ so you end up getting the new rules without any ambiguity or mistaken > With this you are no longer able to set the 'outer' pos within the list > iterator loop body or am I missing something? Correct. Any assignment inside the loop will be entirely just to the local loop case. So any "break;" out of the loop will have to set another variable - like your updated patch did. > I fail to see how this will make most of the changes in this > patch obsolete (if that was the intention). I hope my explanation above clarifies my thinking: I do not dislike your patch, and in fact your patch is indeed required to make the new semantics work. What I disliked was always the maintainability of your patch - making the rules be something that isn't actually visible in the source code, and letting the old semantics still work as well as they ever did, and having to basically run some verification pass to find bad users. (I also disliked your original patch that mixed up the "CPU speculation type safety" with the actual non-speculative problems, but that was another issue). Linus
Re: [PATCH V3 09/30] arm/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
On Tue, Mar 01, 2022 at 05:30:41AM +0530, Anshuman Khandual wrote: > On 2/28/22 4:27 PM, Russell King (Oracle) wrote: > > On Mon, Feb 28, 2022 at 04:17:32PM +0530, Anshuman Khandual wrote: > >> This defines and exports a platform specific custom vm_get_page_prot() via > >> subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX > >> macros can be dropped which are no longer needed. > > > > What I would really like to know is why having to run _code_ to work out > > what the page protections need to be is better than looking it up in a > > table. > > > > Not only is this more expensive in terms of CPU cycles, it also brings > > additional code size with it. > > > > I'm struggling to see what the benefit is. > > Currently vm_get_page_prot() is also being _run_ to fetch required page > protection values. Although that is being run in the core MM and from a > platform perspective __SXXX, __PXXX are just being exported for a table. > Looking it up in a table (and applying more constructs there after) is > not much different than a clean switch case statement in terms of CPU > usage. So this is not more expensive in terms of CPU cycles. I disagree. However, let's base this disagreement on some evidence. Here is the present 32-bit ARM implementation: 0048 : 48: e20fand r0, r0, #15 4c: e3003000movwr3, #0 4c: R_ARM_MOVW_ABS_NC .LANCHOR1 50: e3403000movtr3, #0 50: R_ARM_MOVT_ABS .LANCHOR1 54: e7930100ldr r0, [r3, r0, lsl #2] 58: e12fff1ebx lr That is five instructions long. Please show that your new implementation is not more expensive on 32-bit ARM. Please do so by building a 32-bit kernel, and providing the disassembly. I think you will find way more than five instructions in your version - the compiler will have to issue code to decode the protection bits, probably using a table of branches or absolute PC values, or possibly the worst case of using multiple comparisons and branches. It then has to load constants that may be moved using movw on ARMv7, but on older architectures would have to be created from multiple instructions or loaded from the literal pool. Then there'll be instructions to load the address of "user_pgprot", retrieve its value, and bitwise or that. Therefore, I fail to see how your approach of getting rid of the table is somehow "better" than what we currently have in terms of the effect on the resulting code. If you don't like the __P and __S stuff and two arch_* hooks, you could move the table into arch code along with vm_get_page_prot() without the additional unnecessary hooks, while keeping all the benefits of the table lookup. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH V3 09/30] arm/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
On 2/28/22 4:27 PM, Russell King (Oracle) wrote: > On Mon, Feb 28, 2022 at 04:17:32PM +0530, Anshuman Khandual wrote: >> This defines and exports a platform specific custom vm_get_page_prot() via >> subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX >> macros can be dropped which are no longer needed. > > What I would really like to know is why having to run _code_ to work out > what the page protections need to be is better than looking it up in a > table. > > Not only is this more expensive in terms of CPU cycles, it also brings > additional code size with it. > > I'm struggling to see what the benefit is. > Currently vm_get_page_prot() is also being _run_ to fetch required page protection values. Although that is being run in the core MM and from a platform perspective __SXXX, __PXXX are just being exported for a table. Looking it up in a table (and applying more constructs there after) is not much different than a clean switch case statement in terms of CPU usage. So this is not more expensive in terms of CPU cycles. -- pgprot_t protection_map[16] __ro_after_init = { __P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111, __S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111 }; #ifndef CONFIG_ARCH_HAS_FILTER_PGPROT static inline pgprot_t arch_filter_pgprot(pgprot_t prot) { return prot; } #endif pgprot_t vm_get_page_prot(unsigned long vm_flags) { pgprot_t ret = __pgprot(pgprot_val(protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) | pgprot_val(arch_vm_get_page_prot(vm_flags))); return arch_filter_pgprot(ret); } EXPORT_SYMBOL(vm_get_page_prot) There will be a single vm_get_page_prot() instance on a given platform just like before. So this also does not bring any additional code size with it. As mentioned earlier on a previous version. Remove multiple 'core MM <--> platform' abstraction layers to map vm_flags access permission combination into page protection. >From the cover letter .. -- Currently there are multiple layers of abstraction i.e __SXXX/__PXXX macros , protection_map[], arch_vm_get_page_prot() and arch_filter_pgprot() built between the platform and generic MM, finally defining vm_get_page_prot(). Hence this series proposes to drop all these abstraction levels and instead just move the responsibility of defining vm_get_page_prot() to the platform itself making it clean and simple. -- Benefits 1. For platforms using arch_vm_get_page_prot() and/or arch_filter_pgprot() - A simplified vm_get_page_prot() - Dropped arch_vm_get_page_prot() and arch_filter_pgprot() - Dropped __SXXX, __PXXX macros 2. For platforms which just exported __SXXX, __PXXX - A simplified vm_get_page_prot() - Dropped __SXXX, __PXXX macros 3. For core MM - Dropped a complex vm_get_page_prot() with multiple layers of abstraction i.e __SXXX/__PXXX macros, protection_map[], arch_vm_get_page_prot(), arch_filter_pgprot() etc. - Anshuman
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:37:15PM -0800, Linus Torvalds wrote: > On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox wrote: > > > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > > it catches real bugs. > > Oh, we already can never use -Wshadow regardless of things like this. > That bridge hasn't just been burned, it never existed in the first > place. > > The whole '-Wshadow' thing simply cannot work with local variables in > macros - something that we've used since day 1. > > Try this (as a "p.c" file): > > #define min(a,b) ({ \ > typeof(a) __a = (a);\ > typeof(b) __b = (b);\ > __a < __b ? __a : __b; }) > > int min3(int a, int b, int c) > { > return min(a,min(b,c)); > } > > and now do "gcc -O2 -S t.c". > > Then try it with -Wshadow. #define ___PASTE(a, b) a##b #define __PASTE(a, b) ___PASTE(a, b) #define _min(a, b, u) ({ \ typeof(a) __PASTE(__a,u) = (a);\ typeof(b) __PASTE(__b,u) = (b);\ __PASTE(__a,u) < __PASTE(__b,u) ? __PASTE(__a,u) : __PASTE(__b,u); }) #define min(a, b) _min(a, b, __COUNTER__) int min3(int a, int b, int c) { return min(a,min(b,c)); } (probably there's a more elegant way to do this)
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote: > > On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley < > james.bottom...@hansenpartnership.com> wrote: > > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: [...] > > > > I do wish we could actually poison the 'pos' value after the > > > > loop somehow - but clearly the "might be uninitialized" I was > > > > hoping for isn't the way to do it. > > > > > > > > Anybody have any ideas? > > > > > > I think we should look at the use cases why code is touching > > > (pos) after the loop. > > > > > > Just from skimming over the patches to change this and experience > > > with the drivers/subsystems I help to maintain I think the > > > primary pattern looks something like this: > > > > > > list_for_each_entry(entry, head, member) { > > > if (some_condition_checking(entry)) > > > break; > > > } > > > do_something_with(entry); > > > > Actually, we usually have a check to see if the loop found > > anything, but in that case it should something like > > > > if (list_entry_is_head(entry, head, member)) { > >return with error; > > } > > do_somethin_with(entry); > > > > Suffice? The list_entry_is_head() macro is designed to cope with > > the bogus entry on head problem. > > Won't suffice because the end goal of this work is to limit scope of > entry only to loop. Hence the need for additional variable. Well, yes, but my objection is more to the size of churn than the desire to do loop local. I'm not even sure loop local is possible, because it's always annoyed me that for (int i = 0; ... in C++ defines i in the outer scope not the loop scope, which is why I never use it. However, if the desire is really to poison the loop variable then we can do #define list_for_each_entry(pos, head, member) \ for (pos = list_first_entry(head, typeof(*pos), member);\ !list_entry_is_head(pos, head, member) && ((pos = NULL) == NULL; \ pos = list_next_entry(pos, member)) Which would at least set pos to NULL when the loop completes. > Besides, there are no guarantees that people won't > do_something_with(entry) without the check or won't compare entry to > NULL to check if the loop finished with break or not. I get the wider goal, but we have to patch the problem cases now and a simple one-liner is better than a larger patch that may or may not work if we ever achieve the local definition or value poisoning idea. I'm also fairly certain coccinelle can come up with a use without checking for loop completion semantic patch which we can add to 0day. James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:56, Christian König wrote: > > > > Am 28.02.22 um 21:42 schrieb James Bottomley: >> On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: >>> Am 28.02.22 um 20:56 schrieb Linus Torvalds: On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: [SNIP] Anybody have any ideas? >>> I think we should look at the use cases why code is touching (pos) >>> after the loop. >>> >>> Just from skimming over the patches to change this and experience >>> with the drivers/subsystems I help to maintain I think the primary >>> pattern looks something like this: >>> >>> list_for_each_entry(entry, head, member) { >>> if (some_condition_checking(entry)) >>> break; >>> } >>> do_something_with(entry); There are other cases where the list iterator variable is used after the loop Some examples: - list_for_each_entry_continue() and list_for_each_entry_from(). - (although very rare) the head is actually of the correct struct type. (ppc440spe_get_group_entry(): drivers/dma/ppc4xx/adma.c:1436) - to use pos->list for example for list_add_tail(): (add_static_vm_early(): arch/arm/mm/ioremap.c:107) If the scope of the list iterator is limited those still need fixing in a different way. >> >> Actually, we usually have a check to see if the loop found anything, >> but in that case it should something like >> >> if (list_entry_is_head(entry, head, member)) { >> return with error; >> } >> do_somethin_with(entry); >> >> Suffice? The list_entry_is_head() macro is designed to cope with the >> bogus entry on head problem. > > That will work and is also what people already do. > > The key problem is that we let people do the same thing over and over again > with slightly different implementations. > > Out in the wild I've seen at least using a separate variable, using a bool to > indicate that something was found and just assuming that the list has an > entry. > > The last case is bogus and basically what can break badly. > > If we would have an unified macro which search for an entry combined with > automated reporting on patches to use that macro I think the potential to > introduce such issues will already go down massively without auditing tons of > existing code. Having a unified way to do the same thing would indeed be great. > > Regards, > Christian. > >> >> James >> >> > - Jakob
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley wrote: >On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: >> Am 28.02.22 um 20:56 schrieb Linus Torvalds: >> > On Mon, Feb 28, 2022 at 4:19 AM Christian König >> > wrote: >> > > I don't think that using the extra variable makes the code in any >> > > way >> > > more reliable or easier to read. >> > So I think the next step is to do the attached patch (which >> > requires >> > that "-std=gnu11" that was discussed in the original thread). >> > >> > That will guarantee that the 'pos' parameter of >> > list_for_each_entry() >> > is only updated INSIDE the for_each_list_entry() loop, and can >> > never >> > point to the (wrongly typed) head entry. >> > >> > And I would actually hope that it should actually cause compiler >> > warnings about possibly uninitialized variables if people then use >> > the >> > 'pos' pointer outside the loop. Except >> > >> > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL >> > for >> > inexplicable reasons - possibly because it already expected this >> > behavior >> > >> > (b) when I remove that NULL initializer, I still don't get a >> > warning, >> > because we've disabled -Wno-maybe-uninitialized since it results in >> > so >> > many false positives. >> > >> > Oh well. >> > >> > Anyway, give this patch a look, and at least if it's expanded to do >> > "(pos) = NULL" in the entry statement for the for-loop, it will >> > avoid the HEAD type confusion that Jakob is working on. And I think >> > in a cleaner way than the horrid games he plays. >> > >> > (But it won't avoid possible CPU speculation of such type >> > confusion. That, in my opinion, is a completely different issue) >> >> Yes, completely agree. >> >> > I do wish we could actually poison the 'pos' value after the loop >> > somehow - but clearly the "might be uninitialized" I was hoping for >> > isn't the way to do it. >> > >> > Anybody have any ideas? >> >> I think we should look at the use cases why code is touching (pos) >> after the loop. >> >> Just from skimming over the patches to change this and experience >> with the drivers/subsystems I help to maintain I think the primary >> pattern looks something like this: >> >> list_for_each_entry(entry, head, member) { >> if (some_condition_checking(entry)) >> break; >> } >> do_something_with(entry); > > >Actually, we usually have a check to see if the loop found anything, >but in that case it should something like > >if (list_entry_is_head(entry, head, member)) { >return with error; >} >do_somethin_with(entry); > >Suffice? The list_entry_is_head() macro is designed to cope with the >bogus entry on head problem. Won't suffice because the end goal of this work is to limit scope of entry only to loop. Hence the need for additional variable. Besides, there are no guarantees that people won't do_something_with(entry) without the check or won't compare entry to NULL to check if the loop finished with break or not. >James -- Sincerely yours, Mike
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:10, Linus Torvalds > wrote: > > On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds > wrote: >> >> Side note: we do need *some* way to do it. > > Ooh. > > This patch is a work of art. > > And I mean that in the worst possible way. > > We can do > >typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. > > And then the compiler will not see some "might be uninitialized", but > the outer 'pos' *will* be uninitialized. > > Unless, of course, the outer 'pos' had that pointless explicit initializer. The goal of this is to get compiler warnings right? This would indeed be great. Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()). I assumed it is better to fix those cases first and then have a simple coccinelle script changing the macro + moving the iterator into the scope of the macro. > > Here - can somebody poke holes in this "work of art" patch? With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something? Like this it stays uninitialized but you'll probably want to set it from within the loop. You would then yet again need a variable with another name to use after the loop. I fail to see how this will make most of the changes in this patch obsolete (if that was the intention). > > Linus > - Jakob
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:45 PM James Bottomley wrote: > ... > > Just from skimming over the patches to change this and experience > > with the drivers/subsystems I help to maintain I think the primary > > pattern looks something like this: > > > > list_for_each_entry(entry, head, member) { > > if (some_condition_checking(entry)) > > break; > > } > > do_something_with(entry); > > > Actually, we usually have a check to see if the loop found anything, > but in that case it should something like > > if (list_entry_is_head(entry, head, member)) { > return with error; > } > do_somethin_with(entry); Borrowing from c++, perhaps an explicit end should be used: if (list_entry_not_end(entry)) do_somethin_with(entry) It is modelled after c++ and the 'while(begin != end) {}' pattern. Jeff
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 20:16 +, Matthew Wilcox wrote: > On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > I was just going to say the same thing... If we're willing to change the API for the macro, we could do list_for_each_entry(type, pos, head, member) and then actually take advantage of -Wshadow? johannes
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote: > > Am 28.02.22 um 21:42 schrieb James Bottomley: > > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: > > > Am 28.02.22 um 20:56 schrieb Linus Torvalds: > > > > On Mon, Feb 28, 2022 at 4:19 AM Christian König > > > > wrote: > > > > [SNIP] > > > > Anybody have any ideas? > > > I think we should look at the use cases why code is touching > > > (pos) > > > after the loop. > > > > > > Just from skimming over the patches to change this and experience > > > with the drivers/subsystems I help to maintain I think the > > > primary pattern looks something like this: > > > > > > list_for_each_entry(entry, head, member) { > > > if (some_condition_checking(entry)) > > > break; > > > } > > > do_something_with(entry); > > > > Actually, we usually have a check to see if the loop found > > anything, but in that case it should something like > > > > if (list_entry_is_head(entry, head, member)) { > > return with error; > > } > > do_somethin_with(entry); > > > > Suffice? The list_entry_is_head() macro is designed to cope with > > the bogus entry on head problem. > > That will work and is also what people already do. > > The key problem is that we let people do the same thing over and > over again with slightly different implementations. > > Out in the wild I've seen at least using a separate variable, using > a bool to indicate that something was found and just assuming that > the list has an entry. > > The last case is bogus and basically what can break badly. Yes, I understand that. I'm saying we should replace that bogus checks of entry->something against some_value loop termination condition with the list_entry_is_head() macro. That should be a one line and fairly mechanical change rather than the explosion of code changes we seem to have in the patch series. James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:14:44PM -0800, Linus Torvalds wrote: > On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds > wrote: > > > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > The thing that makes me throw up in my mouth a bit is that in that > > typeof(pos) pos > > the first 'pos' (that we use for just the typeof) is that outer-level > 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same > declaration that declares the inner level shadowing new 'pos' > variable. The new "pos" has not yet been declared, so this has to refer to the outer "pos", it cannot be the inner one. Because it hasn't been declared yet :-) Compare this to typeof (pos) pos = pos; where that last "pos" *does* refer to the newly declared one: that declaration has already been done! (So this code is UB btw, 6.3.2.1/2). > If I was a compiler person, I would say "Linus, that thing is too ugly > to live", and I would hate it. I'm just hoping that even compiler > people say "that's *so* ugly it's almost beautiful". It is perfectly well-defined. Well, it would be good if we (GCC) would document it does work, and if someone tested it on LLVM as well. But it is really hard to implement it to *not* work :-) > Because it does seem to work. It's not pretty, but hey, it's not like > our headers are really ever be winning any beauty contests... It is very pretty! Needs a comment though :-) Segher
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 21:42 schrieb James Bottomley: On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: Am 28.02.22 um 20:56 schrieb Linus Torvalds: On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: [SNIP] Anybody have any ideas? I think we should look at the use cases why code is touching (pos) after the loop. Just from skimming over the patches to change this and experience with the drivers/subsystems I help to maintain I think the primary pattern looks something like this: list_for_each_entry(entry, head, member) { if (some_condition_checking(entry)) break; } do_something_with(entry); Actually, we usually have a check to see if the loop found anything, but in that case it should something like if (list_entry_is_head(entry, head, member)) { return with error; } do_somethin_with(entry); Suffice? The list_entry_is_head() macro is designed to cope with the bogus entry on head problem. That will work and is also what people already do. The key problem is that we let people do the same thing over and over again with slightly different implementations. Out in the wild I've seen at least using a separate variable, using a bool to indicate that something was found and just assuming that the list has an entry. The last case is bogus and basically what can break badly. If we would have an unified macro which search for an entry combined with automated reporting on patches to use that macro I think the potential to introduce such issues will already go down massively without auditing tons of existing code. Regards, Christian. James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote: > Am 28.02.22 um 20:56 schrieb Linus Torvalds: > > On Mon, Feb 28, 2022 at 4:19 AM Christian König > > wrote: > > > I don't think that using the extra variable makes the code in any > > > way > > > more reliable or easier to read. > > So I think the next step is to do the attached patch (which > > requires > > that "-std=gnu11" that was discussed in the original thread). > > > > That will guarantee that the 'pos' parameter of > > list_for_each_entry() > > is only updated INSIDE the for_each_list_entry() loop, and can > > never > > point to the (wrongly typed) head entry. > > > > And I would actually hope that it should actually cause compiler > > warnings about possibly uninitialized variables if people then use > > the > > 'pos' pointer outside the loop. Except > > > > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL > > for > > inexplicable reasons - possibly because it already expected this > > behavior > > > > (b) when I remove that NULL initializer, I still don't get a > > warning, > > because we've disabled -Wno-maybe-uninitialized since it results in > > so > > many false positives. > > > > Oh well. > > > > Anyway, give this patch a look, and at least if it's expanded to do > > "(pos) = NULL" in the entry statement for the for-loop, it will > > avoid the HEAD type confusion that Jakob is working on. And I think > > in a cleaner way than the horrid games he plays. > > > > (But it won't avoid possible CPU speculation of such type > > confusion. That, in my opinion, is a completely different issue) > > Yes, completely agree. > > > I do wish we could actually poison the 'pos' value after the loop > > somehow - but clearly the "might be uninitialized" I was hoping for > > isn't the way to do it. > > > > Anybody have any ideas? > > I think we should look at the use cases why code is touching (pos) > after the loop. > > Just from skimming over the patches to change this and experience > with the drivers/subsystems I help to maintain I think the primary > pattern looks something like this: > > list_for_each_entry(entry, head, member) { > if (some_condition_checking(entry)) > break; > } > do_something_with(entry); Actually, we usually have a check to see if the loop found anything, but in that case it should something like if (list_entry_is_head(entry, head, member)) { return with error; } do_somethin_with(entry); Suffice? The list_entry_is_head() macro is designed to cope with the bogus entry on head problem. James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:29 PM Johannes Berg wrote: > > If we're willing to change the API for the macro, we could do > > list_for_each_entry(type, pos, head, member) > > and then actually take advantage of -Wshadow? See my reply to Willy. There is no way -Wshadow will ever happen. I considered that (type, pos, head, member) kind of thing, to the point of trying it for one file, but it ends up as horrendous syntax. It turns out that declaring the type separately really helps, and avoids crazy long lines among other things. It would be unacceptable for another reason too - the amount of churn would just be immense. Every single use of that macro (and related macros) would change, even the ones that really don't need it or want it (ie the good kinds that already only use the variable inside the loop). So "typeof(pos) pos" may be ugly - but it's a very localized ugly. Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox wrote: > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. Oh, we already can never use -Wshadow regardless of things like this. That bridge hasn't just been burned, it never existed in the first place. The whole '-Wshadow' thing simply cannot work with local variables in macros - something that we've used since day 1. Try this (as a "p.c" file): #define min(a,b) ({ \ typeof(a) __a = (a);\ typeof(b) __b = (b);\ __a < __b ? __a : __b; }) int min3(int a, int b, int c) { return min(a,min(b,c)); } and now do "gcc -O2 -S t.c". Then try it with -Wshadow. In other words, -Wshadow is simply not acceptable. Never has been, never will be, and that has nothing to do with the typeof(pos) pos kind of thing. Your argument just isn't an argument. Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds wrote: > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. Side note: we do need *some* way to do it. Because if we make that change, and only set it to another pointer when not the head, then we very much change the semantics of "list_for_each_head()". The "list was empty" case would now exit with 'pos' not set at all (or set to NULL if we add that). Or it would be set to the last entry. And regardless, that means that all the if (pos == head) kinds of checks after the loop would be fundamentally broken. Darn. I really hoped for (and naively expected) that we could actually have the compiler warn about the use-after-loop case. That whole "compiler will now complain about bad use" was integral to my clever plan to use the C99 feature of declaring the iterator inside the loop. But my "clever plan" was apparently some ACME-level Wile E. Coyote sh*t. Darn. Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds wrote: > > We can do > > typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. The thing that makes me throw up in my mouth a bit is that in that typeof(pos) pos the first 'pos' (that we use for just the typeof) is that outer-level 'pos', IOW it's a *different* 'pos' than the second 'pos' in that same declaration that declares the inner level shadowing new 'pos' variable. If I was a compiler person, I would say "Linus, that thing is too ugly to live", and I would hate it. I'm just hoping that even compiler people say "that's *so* ugly it's almost beautiful". Because it does seem to work. It's not pretty, but hey, it's not like our headers are really ever be winning any beauty contests... Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > We can do > > typeof(pos) pos > > in the 'for ()' loop, and never use __iter at all. > > That means that inside the for-loop, we use a _different_ 'pos' than outside. Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs. > +#define list_for_each_entry(pos, head, member) > \ > + for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); > \ > + !list_entry_is_head(pos, head, member);\ >pos = list_next_entry(pos, member))
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds wrote: > > Side note: we do need *some* way to do it. Ooh. This patch is a work of art. And I mean that in the worst possible way. We can do typeof(pos) pos in the 'for ()' loop, and never use __iter at all. That means that inside the for-loop, we use a _different_ 'pos' than outside. And then the compiler will not see some "might be uninitialized", but the outer 'pos' *will* be uninitialized. Unless, of course, the outer 'pos' had that pointless explicit initializer. Here - can somebody poke holes in this "work of art" patch? Linus Makefile | 2 +- arch/x86/kernel/cpu/sgx/encl.c | 2 +- include/linux/list.h | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index daeb5c88b50b..cc4b0a266af0 100644 --- a/Makefile +++ b/Makefile @@ -515,7 +515,7 @@ KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \ -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \ -Werror=implicit-function-declaration -Werror=implicit-int \ -Werror=return-type -Wno-format-security \ - -std=gnu89 + -std=gnu11 KBUILD_CPPFLAGS := -D__KERNEL__ KBUILD_AFLAGS_KERNEL := KBUILD_CFLAGS_KERNEL := diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 48afe96ae0f0..87db2f3936b0 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -450,7 +450,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, struct mm_struct *mm) { struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); - struct sgx_encl_mm *tmp = NULL; + struct sgx_encl_mm *tmp; /* * The enclave itself can remove encl_mm. Note, objects can't be moved diff --git a/include/linux/list.h b/include/linux/list.h index dd6c2041d09c..708078b2f24d 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -634,9 +634,9 @@ static inline void list_splice_tail_init(struct list_head *list, * @head: the head for your list. * @member: the name of the list_head within the struct. */ -#define list_for_each_entry(pos, head, member)\ - for (pos = list_first_entry(head, typeof(*pos), member); \ - !list_entry_is_head(pos, head, member); \ +#define list_for_each_entry(pos, head, member) \ + for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member); \ + !list_entry_is_head(pos, head, member); \ pos = list_next_entry(pos, member)) /**
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 20:56 schrieb Linus Torvalds: On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: I don't think that using the extra variable makes the code in any way more reliable or easier to read. So I think the next step is to do the attached patch (which requires that "-std=gnu11" that was discussed in the original thread). That will guarantee that the 'pos' parameter of list_for_each_entry() is only updated INSIDE the for_each_list_entry() loop, and can never point to the (wrongly typed) head entry. And I would actually hope that it should actually cause compiler warnings about possibly uninitialized variables if people then use the 'pos' pointer outside the loop. Except (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for inexplicable reasons - possibly because it already expected this behavior (b) when I remove that NULL initializer, I still don't get a warning, because we've disabled -Wno-maybe-uninitialized since it results in so many false positives. Oh well. Anyway, give this patch a look, and at least if it's expanded to do "(pos) = NULL" in the entry statement for the for-loop, it will avoid the HEAD type confusion that Jakob is working on. And I think in a cleaner way than the horrid games he plays. (But it won't avoid possible CPU speculation of such type confusion. That, in my opinion, is a completely different issue) Yes, completely agree. I do wish we could actually poison the 'pos' value after the loop somehow - but clearly the "might be uninitialized" I was hoping for isn't the way to do it. Anybody have any ideas? I think we should look at the use cases why code is touching (pos) after the loop. Just from skimming over the patches to change this and experience with the drivers/subsystems I help to maintain I think the primary pattern looks something like this: list_for_each_entry(entry, head, member) { if (some_condition_checking(entry)) break; } do_something_with(entry); So the solution should probably not be to change all those use cases to use more temporary variables, but rather to add a list_find_entry(..., condition) macro and consistently use that one instead. Regards, Christian. Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:19 AM Christian König wrote: > > I don't think that using the extra variable makes the code in any way > more reliable or easier to read. So I think the next step is to do the attached patch (which requires that "-std=gnu11" that was discussed in the original thread). That will guarantee that the 'pos' parameter of list_for_each_entry() is only updated INSIDE the for_each_list_entry() loop, and can never point to the (wrongly typed) head entry. And I would actually hope that it should actually cause compiler warnings about possibly uninitialized variables if people then use the 'pos' pointer outside the loop. Except (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for inexplicable reasons - possibly because it already expected this behavior (b) when I remove that NULL initializer, I still don't get a warning, because we've disabled -Wno-maybe-uninitialized since it results in so many false positives. Oh well. Anyway, give this patch a look, and at least if it's expanded to do "(pos) = NULL" in the entry statement for the for-loop, it will avoid the HEAD type confusion that Jakob is working on. And I think in a cleaner way than the horrid games he plays. (But it won't avoid possible CPU speculation of such type confusion. That, in my opinion, is a completely different issue) I do wish we could actually poison the 'pos' value after the loop somehow - but clearly the "might be uninitialized" I was hoping for isn't the way to do it. Anybody have any ideas? Linus p Description: Binary data
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > a multi-line indent gets curly braces for readability even though > it's not required by C. And then both sides would get curly braces. That's more your personal preference than a coding style guideline.
Re: [PATCH v5 3/9] powerpc/vas: Add paste address mmap fault handler
Hi Haren, I love your patch! Perhaps something to improve: [auto build test WARNING on powerpc/next] [also build test WARNING on v5.17-rc6 next-20220225] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Haren-Myneni/powerpc-pseries-vas-NXGZIP-support-with-DLPAR/20220228-154814 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc64-randconfig-s031-20220227 (https://download.01.org/0day-ci/archive/20220228/202202281913.4n2af10e-...@intel.com/config) compiler: powerpc64-linux-gcc (GCC) 11.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-dirty # https://github.com/0day-ci/linux/commit/336150efb288e945786ca6d54b0eb7d9c956aad8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Haren-Myneni/powerpc-pseries-vas-NXGZIP-support-with-DLPAR/20220228-154814 git checkout 336150efb288e945786ca6d54b0eb7d9c956aad8 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash arch/powerpc/platforms/book3s/ arch/powerpc/platforms/pseries/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot sparse warnings: (new ones prefixed by >>) >> arch/powerpc/platforms/book3s/vas-api.c:403:29: sparse: sparse: incorrect >> type in assignment (different base types) @@ expected int ret @@ got >> restricted vm_fault_t @@ arch/powerpc/platforms/book3s/vas-api.c:403:29: sparse: expected int ret arch/powerpc/platforms/book3s/vas-api.c:403:29: sparse: got restricted vm_fault_t >> arch/powerpc/platforms/book3s/vas-api.c:406:32: sparse: sparse: incorrect >> type in return expression (different base types) @@ expected restricted >> vm_fault_t @@ got int ret @@ arch/powerpc/platforms/book3s/vas-api.c:406:32: sparse: expected restricted vm_fault_t arch/powerpc/platforms/book3s/vas-api.c:406:32: sparse: got int ret vim +403 arch/powerpc/platforms/book3s/vas-api.c 353 354 /* 355 * This fault handler is invoked when the core generates page fault on 356 * the paste address. Happens if the kernel closes window in hypervisor 357 * (on pseries) due to lost credit or the paste address is not mapped. 358 */ 359 static vm_fault_t vas_mmap_fault(struct vm_fault *vmf) 360 { 361 struct vm_area_struct *vma = vmf->vma; 362 struct file *fp = vma->vm_file; 363 struct coproc_instance *cp_inst = fp->private_data; 364 struct vas_window *txwin; 365 u64 paste_addr; 366 int ret; 367 368 /* 369 * window is not opened. Shouldn't expect this error. 370 */ 371 if (!cp_inst || !cp_inst->txwin) { 372 pr_err("%s(): Unexpected fault on paste address with TX window closed\n", 373 __func__); 374 return VM_FAULT_SIGBUS; 375 } 376 377 txwin = cp_inst->txwin; 378 /* 379 * When the LPAR lost credits due to core removal or during 380 * migration, invalidate the existing mapping for the current 381 * paste addresses and set windows in-active (zap_page_range in 382 * reconfig_close_windows()). 383 * New mapping will be done later after migration or new credits 384 * available. So continue to receive faults if the user space 385 * issue NX request. 386 */ 387 if (txwin->task_ref.vma != vmf->vma) { 388 pr_err("%s(): No previous mapping with paste address\n", 389 __func__); 390 return VM_FAULT_SIGBUS; 391 } 392 393 mutex_lock(>task_ref.mmap_mutex); 394 /* 395 * The window may be inactive due to lost credit (Ex: core 396 * removal with DLPAR). If the window is active again when 397 * the credit is available, map the new paste address at the 398 * the window virtual address. 399 */ 400 if (txwin->status == VAS_WIN_ACTIVE) { 401 paste_addr = cp_inst->coproc->vops->paste_addr(txwin); 402 if (paste_addr) { > 403
RE: [PATCH 08/11] swiotlb: make the swiotlb_init interface more useful
From: Christoph Hellwig Sent: Monday, February 28, 2022 3:31 AM > > On Mon, Feb 28, 2022 at 02:53:39AM +, Michael Kelley (LINUX) wrote: > > From: Christoph Hellwig Sent: Sunday, February 27, 2022 6:31 > > AM > > > > > > Pass a bool to pass if swiotlb needs to be enabled based on the > > > addressing needs and replace the verbose argument with a set of > > > flags, including one to force enable bounce buffering. > > > > > > Note that this patch removes the possibility to force xen-swiotlb > > > use using swiotlb=force on the command line on x86 (arm and arm64 > > > never supported that), but this interface will be restored shortly. > > > > > > Signed-off-by: Christoph Hellwig > > > --- > > > arch/arm/mm/init.c | 6 + > > > arch/arm64/mm/init.c | 6 + > > > arch/ia64/mm/init.c| 4 +-- > > > arch/mips/cavium-octeon/dma-octeon.c | 2 +- > > > arch/mips/loongson64/dma.c | 2 +- > > > arch/mips/sibyte/common/dma.c | 2 +- > > > arch/powerpc/include/asm/swiotlb.h | 1 + > > > arch/powerpc/mm/mem.c | 3 ++- > > > arch/powerpc/platforms/pseries/setup.c | 3 --- > > > arch/riscv/mm/init.c | 8 +- > > > arch/s390/mm/init.c| 3 +-- > > > arch/x86/kernel/cpu/mshyperv.c | 8 -- > > > arch/x86/kernel/pci-dma.c | 15 ++- > > > arch/x86/mm/mem_encrypt_amd.c | 3 --- > > > drivers/xen/swiotlb-xen.c | 4 +-- > > > include/linux/swiotlb.h| 15 ++- > > > include/trace/events/swiotlb.h | 29 - > > > kernel/dma/swiotlb.c | 35 ++ > > > 18 files changed, 56 insertions(+), 93 deletions(-) > > > > [snip] > > > > > > > > diff --git a/arch/x86/kernel/cpu/mshyperv.c > > > b/arch/x86/kernel/cpu/mshyperv.c > > > index 5a99f993e6392..568274917f1cd 100644 > > > --- a/arch/x86/kernel/cpu/mshyperv.c > > > +++ b/arch/x86/kernel/cpu/mshyperv.c > > > @@ -336,14 +336,6 @@ static void __init ms_hyperv_init_platform(void) > > > swiotlb_unencrypted_base = > ms_hyperv.shared_gpa_boundary; > > > #endif > > > } > > > - > > > -#ifdef CONFIG_SWIOTLB > > > - /* > > > - * Enable swiotlb force mode in Isolation VM to > > > - * use swiotlb bounce buffer for dma transaction. > > > - */ > > > - swiotlb_force = SWIOTLB_FORCE; > > > -#endif > > > > With this code removed, it's not clear to me what forces the use of the > > swiotlb in a Hyper-V isolated VM. The code in pci_swiotlb_detect_4g() > > doesn't > > catch this case because cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) > > returns "false" in a Hyper-V guest. In the Hyper-V guest, it's only > > cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) that returns "true". I'm > > looking more closely at the meaning of the CC_ATTR_* values, and it may > > be that Hyper-V should also return "true" for CC_ATTR_MEM_ENCRYPT, > > but I don't think CC_ATTR_HOST_MEM_ENCRYPT should return "true". > > Ok, I assumed that CC_ATTR_HOST_MEM_ENCRYPT returned true in this case. > I guess we just need to check for CC_ATTR_GUEST_MEM_ENCRYPT as well > there? I'm unsure. The comments for CC_ATTR_HOST_MEM_ENCRYPT indicates that it is for SME. The comments for both CC_ATTR_MEM_ENCRYPT and CC_ATTR_GUEST_MEM_ENCRYPT mention SEV and SEV-ES (and presumably SEV-SNP). But I haven't looked at the details of the core SNP patches from the AMD folks. I'd say that they need to weigh in on the right approach here that will work for both SME and the various SEV flavors, and then hopefully the Hyper-V case will fit in. Michael
[PATCH] powerpc/sysdev: fix incorrect use to determine if list is empty
'gtm' will *always* be set by list_for_each_entry(). It is incorrect to assume that the iterator value will be NULL if the list is empty. Instead of checking the pointer it should be checked if the list is empty. Fixes: 83ff9dcf375c ("powerpc/sysdev: implement FSL GTM support") Signed-off-by: Jakob Koschel --- arch/powerpc/sysdev/fsl_gtm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c index 8963eaffb1b7..39186ad6b3c3 100644 --- a/arch/powerpc/sysdev/fsl_gtm.c +++ b/arch/powerpc/sysdev/fsl_gtm.c @@ -86,7 +86,7 @@ static LIST_HEAD(gtms); */ struct gtm_timer *gtm_get_timer16(void) { - struct gtm *gtm = NULL; + struct gtm *gtm; int i; list_for_each_entry(gtm, , list_node) { @@ -103,7 +103,7 @@ struct gtm_timer *gtm_get_timer16(void) spin_unlock_irq(>lock); } - if (gtm) + if (!list_empty()) return ERR_PTR(-EBUSY); return ERR_PTR(-ENODEV); } base-commit: 7ee022567bf9e2e0b3cd92461a2f4986ecc99673 -- 2.25.1
Re: [PATCH V3 09/30] arm/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
Hi Russell, On Mon, Feb 28, 2022 at 11:57 AM Russell King (Oracle) wrote: > On Mon, Feb 28, 2022 at 04:17:32PM +0530, Anshuman Khandual wrote: > > This defines and exports a platform specific custom vm_get_page_prot() via > > subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX > > macros can be dropped which are no longer needed. > > What I would really like to know is why having to run _code_ to work out > what the page protections need to be is better than looking it up in a > table. > > Not only is this more expensive in terms of CPU cycles, it also brings > additional code size with it. > > I'm struggling to see what the benefit is. I was wondering about that as well. But at least for code size on m68k, this didn't have much impact. Looking at the generated code, the increase due to using code for the (few different) cases is offset by a 16-bit jump table (which is to be credited to the compiler). In terms of CPU cycles, it's indeed worse than before. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote: > >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > >> *_req) > >>dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); > >>net2272_done(ep, req, -ECONNRESET); > >>} > >> - req = NULL; > > > > Another unrelated change. These are all good changes but send them as > > separate patches. > > You are referring to the req = NULL, right? Yes. > > I've changed the use of 'req' in the same function and assumed that I can > just remove the unnecessary statement. But if it's better to do separately > I'll do that. > These are all changes which made me pause during my review to figure out why they were necessary. The line between what is a related part of a patch is a bit vague and some maintainers will ask you to add or subtract from a patch depending on their individual tastes. I don't really have an exact answer, but I felt like this patch needs to be subtracted from. Especially if there is a whole chunk of the patch which can be removed, then to me, that obviously should be in a different patch. regards, dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > diff --git a/drivers/scsi/scsi_transport_sas.c > b/drivers/scsi/scsi_transport_sas.c > index 4ee578b181da..a8cbd90db9d2 100644 > --- a/drivers/scsi/scsi_transport_sas.c > +++ b/drivers/scsi/scsi_transport_sas.c > @@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy); > * connected to a remote device is a port, so ports must be formed on > * all devices with phys if they're connected to anything. > */ > -void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy) > +void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy) _phy is an unfortunate name. > { > mutex_lock(>phy_list_mutex); > - if (unlikely(!list_empty(>port_siblings))) { > + if (unlikely(!list_empty(&_phy->port_siblings))) { > /* make sure we're already on this port */ > + struct sas_phy *phy = NULL; Maybe call this port_phy? > struct sas_phy *tmp; > > list_for_each_entry(tmp, >phy_list, port_siblings) > - if (tmp == phy) > + if (tmp == _phy) { > + phy = tmp; > break; > + } > /* If this trips, you added a phy that was already >* part of a different port */ > - if (unlikely(tmp != phy)) { > + if (unlikely(!phy)) { > dev_printk(KERN_ERR, >dev, "trying to add phy %s > fails: it's already part of another port\n", > -dev_name(>dev)); > +dev_name(&_phy->dev)); > BUG(); > } > } else { > - sas_port_create_link(port, phy); > - list_add_tail(>port_siblings, >phy_list); > + sas_port_create_link(port, _phy); > + list_add_tail(&_phy->port_siblings, >phy_list); > port->num_phys++; > } > mutex_unlock(>phy_list_mutex); regards, dan carpenter
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote: > diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c > b/drivers/infiniband/hw/hfi1/tid_rdma.c > index 2a7abf7a1f7f..a069847b56aa 100644 > --- a/drivers/infiniband/hw/hfi1/tid_rdma.c > +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c > @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow) > struct hfi1_ctxtdata *rcd = flow->req->rcd; > struct hfi1_devdata *dd = rcd->dd; > u32 ngroups, pageidx = 0; > - struct tid_group *group = NULL, *used; > + struct tid_group *group = NULL, *used, *tmp; > u8 use; > > flow->tnode_cnt = 0; > @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow) > goto used_list; > > /* First look at complete groups */ > - list_for_each_entry(group, >tid_group_list.list, list) { > - kern_add_tid_node(flow, rcd, "complete groups", group, > - group->size); > + list_for_each_entry(tmp, >tid_group_list.list, list) { > + kern_add_tid_node(flow, rcd, "complete groups", tmp, > + tmp->size); > > - pageidx += group->size; > - if (!--ngroups) > + pageidx += tmp->size; > + if (!--ngroups) { > + group = tmp; > break; > + } > } > > if (pageidx >= flow->npagesets) > @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow) >* However, if we are at the head, we have reached the end of the ^ >* complete groups list from the first loop above ^^ >*/ Originally this code tested for an open code list_is_head() so the comment made sense, but it's out of date now. Just delete it. > - if (group && >list == >tid_group_list.list) > + if (!group) > goto bail_eagain; > group = list_prepare_entry(group, >tid_group_list.list, > list); regards, dan carpenter
Re: [PATCH V7 03/20] compat: consolidate the compat_flock{, 64} definition
On Mon, Feb 28, 2022 at 1:13 PM Guo Ren wrote: > On Mon, Feb 28, 2022 at 8:02 PM David Laight wrote: > > From: Guo Ren Sent: 28 February 2022 11:52 > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight > > > wrote: > > > > ... > > > > > +struct compat_flock64 { > > > > > + short l_type; > > > > > + short l_whence; > > > > > + compat_loff_t l_start; > > > > > + compat_loff_t l_len; > > > > > + compat_pid_tl_pid; > > > > > +#ifdef __ARCH_COMPAT_FLOCK64_PAD > > > > > + __ARCH_COMPAT_FLOCK64_PAD > > > > > +#endif > > > > > +} __ARCH_COMPAT_FLOCK64_PACK; > > > > > + > > > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > > See include/asm-generic/compat.h > > > > > > typedef s64 compat_loff_t; > > > > > > Only: > > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > > > compat_loff_t should be compat_s64 not s64. > > > > The same should be done for all 64bit 'compat' types. > Changing > typedef s64 compat_loff_t; > to > typedef compat_s64 compat_loff_t; > > should be another patch and it affects all architectures, I don't > think we should involve it in this series. Agreed, your patch (originally from Christoph) looks fine, it correctly transforms the seven copies of the structure into a shared version. There is always more that can be improved, but for this series, I think you have already done enough. > look at kernel/power/user.c: > struct compat_resume_swap_area { > compat_loff_t offset; > u32 dev; > } __packed; > > I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for > COMPAT support patchset series. The only references to compat_loff_t that we have in the kernel could all be simplified by defining compat_loff_t as compat_s64 instead of s64, but it has no impact on correctness here. Let's make sure you get your series into 5.18, and then David can follow-up with any further cleanups after that. Arnd
RE: [PATCH V7 03/20] compat: consolidate the compat_flock{,64} definition
From: Guo Ren > Sent: 28 February 2022 12:13 > > On Mon, Feb 28, 2022 at 8:02 PM David Laight wrote: > > > > From: Guo Ren > > > Sent: 28 February 2022 11:52 > > > > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight > > > wrote: > > > > > > > > From: guo...@kernel.org > > > > > Sent: 27 February 2022 16:28 > > > > > > > > > > From: Christoph Hellwig > > > > > > > > > > Provide a single common definition for the compat_flock and > > > > > compat_flock64 structures using the same tricks as for the native > > > > > variants. Another extra define is added for the packing required on > > > > > x86. > > > > ... > > > > > diff --git a/arch/x86/include/asm/compat.h > > > > > b/arch/x86/include/asm/compat.h > > > > ... > > > > > /* > > > > > - * IA32 uses 4 byte alignment for 64 bit quantities, > > > > > - * so we need to pack this structure. > > > > > + * IA32 uses 4 byte alignment for 64 bit quantities, so we need to > > > > > pack the > > > > > + * compat flock64 structure. > > > > > */ > > > > ... > > > > > +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > > > > > > > struct compat_statfs { > > > > > int f_type; > > > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > > > index 1c758b0e0359..a0481fe6c5d5 100644 > > > > > --- a/include/linux/compat.h > > > > > +++ b/include/linux/compat.h > > > > > @@ -258,6 +258,37 @@ struct compat_rlimit { > > > > > compat_ulong_t rlim_max; > > > > > }; > > > > > > > > > > +#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > > +#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed)) > > > > > +#else > > > > > +#define __ARCH_COMPAT_FLOCK64_PACK > > > > > +#endif > > > > ... > > > > > +struct compat_flock64 { > > > > > + short l_type; > > > > > + short l_whence; > > > > > + compat_loff_t l_start; > > > > > + compat_loff_t l_len; > > > > > + compat_pid_tl_pid; > > > > > +#ifdef __ARCH_COMPAT_FLOCK64_PAD > > > > > + __ARCH_COMPAT_FLOCK64_PAD > > > > > +#endif > > > > > +} __ARCH_COMPAT_FLOCK64_PACK; > > > > > + > > > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > > See include/asm-generic/compat.h > > > > > > typedef s64 compat_loff_t; > > > > > > Only: > > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > > > compat_loff_t should be compat_s64 not s64. > > > > The same should be done for all 64bit 'compat' types. > Changing > typedef s64 compat_loff_t; > to > typedef compat_s64 compat_loff_t; > > should be another patch and it affects all architectures, I don't > think we should involve it in this series. Except that I think only x86 sets CONFIG_COMPAT_FOR_U64_ALIGNMENT. > look at kernel/power/user.c: > struct compat_resume_swap_area { > compat_loff_t offset; > u32 dev; > } __packed; That is a bug! The size should be 16 bytes on most 32bit architectures. So the compat code won't fault if the last 4 bytes aren't mapped whereas the native 32bit version will fault. Hopefully the compiler realises the on-stack item is actually aligned and doesn't use byte loads and shifts on (eg) sparc64. > I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for > COMPAT support patchset series. But it is wrong :-) compat_[su]64 exist so that compat syscalls that contain 64bit values get the correct alignment. Which is exactly what you have here. AFAICT most of the uses of __packed in the kernel are wrong. It should only be used (on a structure) if the structure might be on a misaligned address. This can happen in data for some network protocols. It should not be used because the structure should have no holes. (Especially in ones that don't have any holes.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 12:20, Greg KH wrote: > > On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: >> If the list does not contain the expected element, the value of >> list_for_each_entry() iterator will not point to a valid structure. >> To avoid type confusion in such case, the list iterator >> scope will be limited to list_for_each_entry() loop. >> >> In preparation to limiting scope of a list iterator to the list traversal >> loop, use a dedicated pointer to point to the found element. >> Determining if an element was found is then simply checking if >> the pointer is != NULL. >> >> Signed-off-by: Jakob Koschel >> --- >> arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- >> drivers/scsi/scsi_transport_sas.c| 17 - >> drivers/thermal/thermal_core.c | 38 ++-- >> drivers/usb/gadget/configfs.c| 22 ++-- >> drivers/usb/gadget/udc/max3420_udc.c | 11 +--- >> drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- >> drivers/usb/mtu3/mtu3_gadget.c | 11 +--- >> drivers/usb/musb/musb_gadget.c | 11 +--- >> drivers/vfio/mdev/mdev_core.c| 11 +--- >> 9 files changed, 88 insertions(+), 50 deletions(-) > > The drivers/usb/ portion of this patch should be in patch 1/X, right? I kept them separate since it's a slightly different case. Using 'ptr' instead of '>other_member'. Regardless, I'll split this commit up as you mentioned. > > Also, you will have to split these up per-subsystem so that the > different subsystem maintainers can take these in their trees. Thanks for the feedback. To clarify I understand you correctly: I should do one patch set per-subsystem with every instance/(file?) fixed as a separate commit? If I understand correctly, I'll repost accordingly. > > thanks, > > greg k-h thanks, Jakob Koschel
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
> On 28. Feb 2022, at 12:24, Dan Carpenter wrote: > > On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote: >> diff --git a/drivers/usb/gadget/udc/at91_udc.c >> b/drivers/usb/gadget/udc/at91_udc.c >> index 9040a0561466..0fd0307bc07b 100644 >> --- a/drivers/usb/gadget/udc/at91_udc.c >> +++ b/drivers/usb/gadget/udc/at91_udc.c >> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct >> at91_ep *ep) >> if (list_empty (>queue)) >> seq_printf(s, "\t(queue empty)\n"); >> >> -else list_for_each_entry (req, >queue, queue) { >> -unsignedlength = req->req.actual; >> +else >> +list_for_each_entry(req, >queue, queue) { >> +unsigned intlength = req->req.actual; >> >> -seq_printf(s, "\treq %p len %d/%d buf %p\n", >> ->req, length, >> -req->req.length, req->req.buf); >> -} >> +seq_printf(s, "\treq %p len %d/%d buf %p\n", >> +>req, length, >> +req->req.length, req->req.buf); >> +} > > Don't make unrelated white space changes. It just makes the patch > harder to review. As you're writing the patch make note of any > additional changes and do them later in a separate patch. > > Also a multi-line indent gets curly braces for readability even though > it's not required by C. And then both sides would get curly braces. > >> spin_unlock_irqrestore(>lock, flags); >> } >> >> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void >> *unused) >> >> if (udc->enabled && udc->vbus) { >> proc_ep_show(s, >ep[0]); >> -list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) { >> +list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) { > > Another unrelated change. > >> if (ep->ep.desc) >> proc_ep_show(s, ep); >> } > > > [ snip ] Thanks for pointing out, I'll remove the changes here and note them down to send them separately. > >> diff --git a/drivers/usb/gadget/udc/net2272.c >> b/drivers/usb/gadget/udc/net2272.c >> index 7c38057dcb4a..bb59200f1596 100644 >> --- a/drivers/usb/gadget/udc/net2272.c >> +++ b/drivers/usb/gadget/udc/net2272.c >> @@ -926,7 +926,8 @@ static int >> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req) >> { >> struct net2272_ep *ep; >> -struct net2272_request *req; >> +struct net2272_request *req = NULL; >> +struct net2272_request *tmp; >> unsigned long flags; >> int stopped; >> >> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request >> *_req) >> ep->stopped = 1; >> >> /* make sure it's still queued on this endpoint */ >> -list_for_each_entry(req, >queue, queue) { >> -if (>req == _req) >> +list_for_each_entry(tmp, >queue, queue) { >> +if (>req == _req) { >> +req = tmp; >> break; >> +} >> } >> -if (>req != _req) { >> +if (!req) { >> ep->stopped = stopped; >> spin_unlock_irqrestore(>dev->lock, flags); >> return -EINVAL; >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request >> *_req) >> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); >> net2272_done(ep, req, -ECONNRESET); >> } >> -req = NULL; > > Another unrelated change. These are all good changes but send them as > separate patches. You are referring to the req = NULL, right? I've changed the use of 'req' in the same function and assumed that I can just remove the unnecessary statement. But if it's better to do separately I'll do that. > >> ep->stopped = stopped; >> >> spin_unlock_irqrestore(>dev->lock, flags); > > regards, > dan carpenter thanks, Jakob Koschel
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
This is a bit more work (and a lot more noise), but I'd prefer if this were split into as many patches as there are components. I'm not going to review the parts of the patches that don't concern me, and if something turns out to be a problem later one (it shouldn't but one never knows) it'll be much easier to revert or put the blame on an individual smaller commit than on this... With that being said, ultimately I don't care that much and will leave that to people who do :) Jakob Koschel wrote on Mon, Feb 28, 2022 at 12:08:22PM +0100: > net/9p/trans_xen.c| 11 +++-- This 9p change looks good to me. Reviewed-by: Dominique Martinet # 9p -- Dominique
[PATCH 6/6] treewide: remove check of list iterator against head past the loop body
When list_for_each_entry() completes the iteration over the whole list without breaking the loop, the iterator value will be a bogus pointer computed based on the head element. While it is safe to use the pointer to determine if it was computed based on the head element, either with list_entry_is_head() or >member == head, using the iterator variable after the loop should be avoided. In preparation to limiting the scope of a list iterator to the list traversal loop, use a dedicated pointer to point to the found element. Signed-off-by: Jakob Koschel --- arch/arm/mach-mmp/sram.c | 9 ++-- arch/arm/plat-pxa/ssp.c | 28 +--- drivers/block/drbd/drbd_req.c | 45 --- drivers/counter/counter-chrdev.c | 26 ++- drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++-- drivers/dma/ppc4xx/adma.c | 11 +++-- drivers/firewire/core-transaction.c | 32 +++-- drivers/firewire/sbp2.c | 14 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +--- drivers/gpu/drm/drm_memory.c | 15 --- drivers/gpu/drm/drm_mm.c | 17 --- drivers/gpu/drm/drm_vm.c | 13 +++--- drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++-- drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++--- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 --- drivers/gpu/drm/i915/gt/intel_ring.c | 15 --- .../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++--- drivers/gpu/drm/scheduler/sched_main.c| 14 +++--- drivers/gpu/drm/ttm/ttm_bo.c | 19 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 + drivers/infiniband/hw/hfi1/tid_rdma.c | 16 --- drivers/infiniband/hw/mlx4/main.c | 12 ++--- drivers/media/dvb-frontends/mxl5xx.c | 11 +++-- drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++-- drivers/misc/mei/interrupt.c | 12 ++--- .../net/ethernet/qlogic/qede/qede_filter.c| 11 +++-- .../net/wireless/intel/ipw2x00/libipw_rx.c| 15 --- drivers/power/supply/cpcap-battery.c | 11 +++-- drivers/scsi/lpfc/lpfc_bsg.c | 16 --- drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++ drivers/staging/rtl8192e/rtllib_rx.c | 17 --- .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 --- .../rtl8192u/ieee80211/rtl819x_TSProc.c | 19 drivers/usb/gadget/composite.c| 9 ++-- fs/cifs/smb2misc.c| 10 +++-- fs/proc/kcore.c | 13 +++--- kernel/debug/kdb/kdb_main.c | 36 +-- kernel/power/snapshot.c | 10 +++-- kernel/trace/ftrace.c | 22 + kernel/trace/trace_eprobe.c | 15 --- kernel/trace/trace_events.c | 11 ++--- net/9p/trans_xen.c| 11 +++-- net/ipv4/udp_tunnel_nic.c | 10 +++-- net/tipc/name_table.c | 11 +++-- net/tipc/socket.c | 11 +++-- net/xfrm/xfrm_ipcomp.c| 11 +++-- sound/soc/intel/catpt/pcm.c | 13 +++--- sound/soc/sprd/sprd-mcdt.c| 13 +++--- 48 files changed, 455 insertions(+), 315 deletions(-) diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c index 6794e2db1ad5..fc47c107059b 100644 --- a/arch/arm/mach-mmp/sram.c +++ b/arch/arm/mach-mmp/sram.c @@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list); struct gen_pool *sram_get_gpool(char *pool_name) { struct sram_bank_info *info = NULL; + struct sram_bank_info *tmp; if (!pool_name) return NULL; mutex_lock(_lock); - list_for_each_entry(info, _bank_list, node) - if (!strcmp(pool_name, info->pool_name)) + list_for_each_entry(tmp, _bank_list, node) + if (!strcmp(pool_name, tmp->pool_name)) { + info = tmp; break; + } mutex_unlock(_lock); - if (>node == _bank_list) + if (!info) return NULL; return info->gpool; diff --git a/arch/arm/plat-pxa/ssp.c b/arch/arm/plat-pxa/ssp.c index 563440315acd..4884a8c0c89b 100644 --- a/arch/arm/plat-pxa/ssp.c +++ b/arch/arm/plat-pxa/ssp.c @@ -38,22 +38,20 @@ static LIST_HEAD(ssp_list); struct ssp_device *pxa_ssp_request(int port, const char *label) { struct ssp_device *ssp = NULL; + struct ssp_device *tmp; mutex_lock(_lock); - list_for_each_entry(ssp, _list, node) { - if (ssp->port_id == port && ssp->use_count == 0) { - ssp->use_count++; - ssp->label = label; + list_for_each_entry(tmp,
[PATCH 5/6] treewide: remove dereference of list iterator after loop body
The list iterator variable will be a bogus pointer if no break was hit. Dereferencing it could load *any* out-of-bounds/undefined value making it unsafe to use that in the comparision to determine if the specific element was found. This is fixed by using a separate list iterator variable for the loop and only setting the original variable if a suitable element was found. Then determing if the element was found is simply checking if the variable is set. Signed-off-by: Jakob Koschel --- drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 11 +++ drivers/scsi/wd719x.c | 12 fs/f2fs/segment.c | 9 ++--- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c index 57199be082fd..c56cd9e59a66 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c @@ -471,20 +471,23 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx) static int nvkm_clk_ustate_update(struct nvkm_clk *clk, int req) { - struct nvkm_pstate *pstate; + struct nvkm_pstate *pstate = NULL; + struct nvkm_pstate *tmp; int i = 0; if (!clk->allow_reclock) return -ENOSYS; if (req != -1 && req != -2) { - list_for_each_entry(pstate, >states, head) { - if (pstate->pstate == req) + list_for_each_entry(tmp, >states, head) { + if (tmp->pstate == req) { + pstate = tmp; break; + } i++; } - if (pstate->pstate != req) + if (!pstate) return -EINVAL; req = i; } diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c index 1a7947554581..be270ed8e00d 100644 --- a/drivers/scsi/wd719x.c +++ b/drivers/scsi/wd719x.c @@ -684,11 +684,15 @@ static irqreturn_t wd719x_interrupt(int irq, void *dev_id) case WD719X_INT_SPIDERFAILED: /* was the cmd completed a direct or SCB command? */ if (regs.bytes.OPC == WD719X_CMD_PROCESS_SCB) { - struct wd719x_scb *scb; - list_for_each_entry(scb, >active_scbs, list) - if (SCB_out == scb->phys) + struct wd719x_scb *scb = NULL; + struct wd719x_scb *tmp; + + list_for_each_entry(tmp, >active_scbs, list) + if (SCB_out == tmp->phys) { + scb = tmp; break; - if (SCB_out == scb->phys) + } + if (scb) wd719x_interrupt_SCB(wd, regs, scb); else dev_err(>pdev->dev, "card returned invalid SCB pointer\n"); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 1dabc8244083..a3684385e04a 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -356,16 +356,19 @@ void f2fs_drop_inmem_page(struct inode *inode, struct page *page) struct f2fs_sb_info *sbi = F2FS_I_SB(inode); struct list_head *head = >inmem_pages; struct inmem_pages *cur = NULL; + struct inmem_pages *tmp; f2fs_bug_on(sbi, !page_private_atomic(page)); mutex_lock(>inmem_lock); - list_for_each_entry(cur, head, list) { - if (cur->page == page) + list_for_each_entry(tmp, head, list) { + if (tmp->page == page) { + cur = tmp; break; + } } - f2fs_bug_on(sbi, list_empty(head) || cur->page != page); + f2fs_bug_on(sbi, !cur); list_del(>list); mutex_unlock(>inmem_lock); -- 2.25.1
[PATCH 4/6] drivers: remove unnecessary use of list iterator variable
When list_for_each_entry() completes the iteration over the whole list without breaking the loop, the iterator value will *always* be a bogus pointer computed based on the head element. To avoid type confusion use the actual list head directly instead of last iterator value. Signed-off-by: Jakob Koschel --- drivers/dma/dw-edma/dw-edma-core.c | 4 ++-- drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++- drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c index 468d1097a1ec..7883c4831857 100644 --- a/drivers/dma/dw-edma/dw-edma-core.c +++ b/drivers/dma/dw-edma/dw-edma-core.c @@ -136,7 +136,7 @@ static void dw_edma_free_burst(struct dw_edma_chunk *chunk) } /* Remove the list head */ - kfree(child); + kfree(chunk->burst); chunk->burst = NULL; } @@ -156,7 +156,7 @@ static void dw_edma_free_chunk(struct dw_edma_desc *desc) } /* Remove the list head */ - kfree(child); + kfree(desc->chunk); desc->chunk = NULL; } diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index 091f36adbbe1..c0ea9dbc4ff6 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -3963,7 +3963,8 @@ static void __i40e_reprogram_flex_pit(struct i40e_pf *pf, * correctly, the hardware will disable flexible field parsing. */ if (!list_empty(flex_pit_list)) - last_offset = list_prev_entry(entry, list)->src_offset + 1; + last_offset = list_entry(flex_pit_list->prev, +struct i40e_flex_pit, list)->src_offset + 1; for (; i < 3; i++, last_offset++) { i40e_write_rx_ctl(>hw, diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c b/drivers/net/wireless/ath/ath6kl/htc_mbox.c index e3874421c4c0..cf5b05860799 100644 --- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c +++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c @@ -104,7 +104,7 @@ static void ath6kl_credit_init(struct ath6kl_htc_credit_info *cred_info, * it use list_for_each_entry_reverse to walk around the whole ep list. * Therefore assign this lowestpri_ep_dist after walk around the ep_list */ - cred_info->lowestpri_ep_dist = cur_ep_dist->list; + cred_info->lowestpri_ep_dist = *ep_list; WARN_ON(cred_info->cur_free_credits <= 0); -- 2.25.1
[PATCH 3/6] treewide: fix incorrect use to determine if list is empty
The list iterator value will *always* be set by list_for_each_entry(). It is incorrect to assume that the iterator value will be NULL if the list is empty. Instead of checking the pointer it should be checked if the list is empty. In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL on the break, it is set to the correct value and leaving it NULL if no element was found. Signed-off-by: Jakob Koschel --- arch/powerpc/sysdev/fsl_gtm.c| 4 ++-- drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++-- drivers/perf/xgene_pmu.c | 13 +++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c index 8963eaffb1b7..39186ad6b3c3 100644 --- a/arch/powerpc/sysdev/fsl_gtm.c +++ b/arch/powerpc/sysdev/fsl_gtm.c @@ -86,7 +86,7 @@ static LIST_HEAD(gtms); */ struct gtm_timer *gtm_get_timer16(void) { - struct gtm *gtm = NULL; + struct gtm *gtm; int i; list_for_each_entry(gtm, , list_node) { @@ -103,7 +103,7 @@ struct gtm_timer *gtm_get_timer16(void) spin_unlock_irq(>lock); } - if (gtm) + if (!list_empty()) return ERR_PTR(-EBUSY); return ERR_PTR(-ENODEV); } diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c index fb24d2ed3621..d3cde05a6eba 100644 --- a/drivers/media/pci/saa7134/saa7134-alsa.c +++ b/drivers/media/pci/saa7134/saa7134-alsa.c @@ -1214,7 +1214,7 @@ static int alsa_device_exit(struct saa7134_dev *dev) static int saa7134_alsa_init(void) { - struct saa7134_dev *dev = NULL; + struct saa7134_dev *dev; saa7134_dmasound_init = alsa_device_init; saa7134_dmasound_exit = alsa_device_exit; @@ -1229,7 +1229,7 @@ static int saa7134_alsa_init(void) alsa_device_init(dev); } - if (dev == NULL) + if (list_empty(_devlist)) pr_info("saa7134 ALSA: no saa7134 cards found\n"); return 0; diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c index 2b6d476bd213..e255f9e665d1 100644 --- a/drivers/perf/xgene_pmu.c +++ b/drivers/perf/xgene_pmu.c @@ -1460,7 +1460,8 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu, struct hw_pmu_info *inf; void __iomem *dev_csr; struct resource res; - struct resource_entry *rentry; + struct resource_entry *rentry = NULL; + struct resource_entry *tmp; int enable_bit; int rc; @@ -1475,16 +1476,16 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu, return NULL; } - list_for_each_entry(rentry, _list, node) { - if (resource_type(rentry->res) == IORESOURCE_MEM) { - res = *rentry->res; - rentry = NULL; + list_for_each_entry(tmp, _list, node) { + if (resource_type(tmp->res) == IORESOURCE_MEM) { + res = *tmp->res; + rentry = tmp; break; } } acpi_dev_free_resource_list(_list); - if (rentry) { + if (!rentry) { dev_err(dev, "PMU type %d: No memory resource found\n", type); return NULL; } -- 2.25.1
[PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
If the list does not contain the expected element, the value of list_for_each_entry() iterator will not point to a valid structure. To avoid type confusion in such case, the list iterator scope will be limited to list_for_each_entry() loop. In preparation to limiting scope of a list iterator to the list traversal loop, use a dedicated pointer to point to the found element. Determining if an element was found is then simply checking if the pointer is != NULL. Signed-off-by: Jakob Koschel --- arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- drivers/scsi/scsi_transport_sas.c| 17 - drivers/thermal/thermal_core.c | 38 ++-- drivers/usb/gadget/configfs.c| 22 ++-- drivers/usb/gadget/udc/max3420_udc.c | 11 +--- drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- drivers/usb/mtu3/mtu3_gadget.c | 11 +--- drivers/usb/musb/musb_gadget.c | 11 +--- drivers/vfio/mdev/mdev_core.c| 11 +--- 9 files changed, 88 insertions(+), 50 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 48afe96ae0f0..6c916416decc 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, struct mm_struct *mm) { struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); - struct sgx_encl_mm *tmp = NULL; + struct sgx_encl_mm *found_encl_mm = NULL; + struct sgx_encl_mm *tmp; /* * The enclave itself can remove encl_mm. Note, objects can't be moved @@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, list_for_each_entry(tmp, _mm->encl->mm_list, list) { if (tmp == encl_mm) { list_del_rcu(_mm->list); + found_encl_mm = tmp; break; } } spin_unlock(_mm->encl->mm_lock); - if (tmp == encl_mm) { + if (found_encl_mm) { synchronize_srcu(_mm->encl->srcu); mmu_notifier_put(mn); } diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c index 4ee578b181da..a8cbd90db9d2 100644 --- a/drivers/scsi/scsi_transport_sas.c +++ b/drivers/scsi/scsi_transport_sas.c @@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy); * connected to a remote device is a port, so ports must be formed on * all devices with phys if they're connected to anything. */ -void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy) +void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy) { mutex_lock(>phy_list_mutex); - if (unlikely(!list_empty(>port_siblings))) { + if (unlikely(!list_empty(&_phy->port_siblings))) { /* make sure we're already on this port */ + struct sas_phy *phy = NULL; struct sas_phy *tmp; list_for_each_entry(tmp, >phy_list, port_siblings) - if (tmp == phy) + if (tmp == _phy) { + phy = tmp; break; + } /* If this trips, you added a phy that was already * part of a different port */ - if (unlikely(tmp != phy)) { + if (unlikely(!phy)) { dev_printk(KERN_ERR, >dev, "trying to add phy %s fails: it's already part of another port\n", - dev_name(>dev)); + dev_name(&_phy->dev)); BUG(); } } else { - sas_port_create_link(port, phy); - list_add_tail(>port_siblings, >phy_list); + sas_port_create_link(port, _phy); + list_add_tail(&_phy->port_siblings, >phy_list); port->num_phys++; } mutex_unlock(>phy_list_mutex); diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 82654dc8382b..97198543448b 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -625,24 +625,30 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz, { struct thermal_instance *dev; struct thermal_instance *pos; - struct thermal_zone_device *pos1; - struct thermal_cooling_device *pos2; + struct thermal_zone_device *pos1 = NULL; + struct thermal_zone_device *tmp1; + struct thermal_cooling_device *pos2 = NULL; + struct thermal_cooling_device *tmp2; unsigned long max_state; int result, ret; if (trip >= tz->trips || trip < 0) return -EINVAL; - list_for_each_entry(pos1, _tz_list, node) { - if (pos1 == tz) + list_for_each_entry(tmp1,
[PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
If the list representing the request queue does not contain the expected request, the value of list_for_each_entry() iterator will not point to a valid structure. To avoid type confusion in such case, the list iterator scope will be limited to list_for_each_entry() loop. In preparation to limiting scope of a list iterator to the list traversal loop, use a dedicated pointer to point to the found request object. Signed-off-by: Jakob Koschel --- drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 ++ drivers/usb/gadget/udc/at91_udc.c| 26 ++-- drivers/usb/gadget/udc/atmel_usba_udc.c | 11 ++ drivers/usb/gadget/udc/bdc/bdc_ep.c | 11 +++--- drivers/usb/gadget/udc/fsl_qe_udc.c | 11 ++ drivers/usb/gadget/udc/fsl_udc_core.c| 11 ++ drivers/usb/gadget/udc/goku_udc.c| 11 ++ drivers/usb/gadget/udc/gr_udc.c | 11 ++ drivers/usb/gadget/udc/lpc32xx_udc.c | 11 ++ drivers/usb/gadget/udc/mv_u3d_core.c | 11 ++ drivers/usb/gadget/udc/mv_udc_core.c | 11 ++ drivers/usb/gadget/udc/net2272.c | 12 ++- drivers/usb/gadget/udc/net2280.c | 11 ++ drivers/usb/gadget/udc/omap_udc.c| 11 ++ drivers/usb/gadget/udc/pxa25x_udc.c | 11 ++ drivers/usb/gadget/udc/s3c-hsudc.c | 11 ++ drivers/usb/gadget/udc/udc-xilinx.c | 11 ++ 17 files changed, 128 insertions(+), 75 deletions(-) diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c index 917892ca8753..cad874ee4472 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c @@ -466,19 +466,22 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep, struct usb_request *u_req) { struct ast_vhub_ep *ep = to_ast_ep(u_ep); struct ast_vhub *vhub = ep->vhub; - struct ast_vhub_req *req; + struct ast_vhub_req *req = NULL; + struct ast_vhub_req *tmp; unsigned long flags; int rc = -EINVAL; spin_lock_irqsave(>lock, flags); /* Make sure it's actually queued on this endpoint */ - list_for_each_entry (req, >queue, queue) { - if (>req == u_req) + list_for_each_entry(tmp, >queue, queue) { + if (>req == u_req) { + req = tmp; break; + } } - if (>req == u_req) { + if (req) { EPVDBG(ep, "dequeue req @%p active=%d\n", req, req->active); if (req->active) diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c index 9040a0561466..0fd0307bc07b 100644 --- a/drivers/usb/gadget/udc/at91_udc.c +++ b/drivers/usb/gadget/udc/at91_udc.c @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct at91_ep *ep) if (list_empty (>queue)) seq_printf(s, "\t(queue empty)\n"); - else list_for_each_entry (req, >queue, queue) { - unsignedlength = req->req.actual; + else + list_for_each_entry(req, >queue, queue) { + unsigned intlength = req->req.actual; - seq_printf(s, "\treq %p len %d/%d buf %p\n", - >req, length, - req->req.length, req->req.buf); - } + seq_printf(s, "\treq %p len %d/%d buf %p\n", + >req, length, + req->req.length, req->req.buf); + } spin_unlock_irqrestore(>lock, flags); } @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused) if (udc->enabled && udc->vbus) { proc_ep_show(s, >ep[0]); - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) { + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) { if (ep->ep.desc) proc_ep_show(s, ep); } @@ -704,7 +705,8 @@ static int at91_ep_queue(struct usb_ep *_ep, static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) { struct at91_ep *ep; - struct at91_request *req; + struct at91_request *req = NULL; + struct at91_request *tmp; unsigned long flags; struct at91_udc *udc; @@ -717,11 +719,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) spin_lock_irqsave(>lock, flags); /* make sure it's actually queued on this endpoint */ - list_for_each_entry (req, >queue, queue) { - if (>req == _req) + list_for_each_entry(tmp, >queue, queue) { + if (>req == _req) { + req = tmp; break; + } } - if
[PATCH 0/6] Remove usage of list iterator past the loop body
This is the first patch removing several categories of use cases of the list iterator variable past the loop. This is follow up to the discussion in: https://lore.kernel.org/all/20220217184829.1991035-1-jakobkosc...@gmail.com/ As concluded in: https://lore.kernel.org/all/yhdfeiwi4edth...@kroah.com/ the correct use should be using a separate variable after the loop and using a 'tmp' variable as the list iterator. The list iterator will not point to a valid structure after the loop if no break/goto was hit. Invalid uses of the list iterator variable can be avoided altogether by simply using a separate pointer to iterate the list. Linus and Greg agreed on the following pattern: - struct gr_request *req; + struct gr_request *req = NULL; + struct gr_request *tmp; struct gr_ep *ep; int ret = 0; - list_for_each_entry(req, >queue, queue) { - if (>req == _req) + list_for_each_entry(tmp, >queue, queue) { + if (>req == _req) { + req = tmp; break; + } } - if (>req != _req) { + if (!req) { ret = -EINVAL; goto out; } With gnu89 the list iterator variable cannot yet be declared within the for loop of the list iterator. Moving to a more modern version of C would allow defining the list iterator variable within the macro, limiting the scope to the loop. This avoids any incorrect usage past the loop altogether. This are around 30% of the cases where the iterator variable is used past the loop (identified with a slightly modified version of use_after_iter.cocci). I've decided to split it into at a few patches separated by similar use cases. Because the output of get_maintainer.pl was too big, I included all the found lists and everyone from the previous discussion. Jakob Koschel (6): drivers: usb: remove usage of list iterator past the loop body treewide: remove using list iterator after loop body as a ptr treewide: fix incorrect use to determine if list is empty drivers: remove unnecessary use of list iterator variable treewide: remove dereference of list iterator after loop body treewide: remove check of list iterator against head past the loop body arch/arm/mach-mmp/sram.c | 9 ++-- arch/arm/plat-pxa/ssp.c | 28 +--- arch/powerpc/sysdev/fsl_gtm.c | 4 +- arch/x86/kernel/cpu/sgx/encl.c| 6 ++- drivers/block/drbd/drbd_req.c | 45 --- drivers/counter/counter-chrdev.c | 26 ++- drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++-- drivers/dma/dw-edma/dw-edma-core.c| 4 +- drivers/dma/ppc4xx/adma.c | 11 +++-- drivers/firewire/core-transaction.c | 32 +++-- drivers/firewire/sbp2.c | 14 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +--- drivers/gpu/drm/drm_memory.c | 15 --- drivers/gpu/drm/drm_mm.c | 17 --- drivers/gpu/drm/drm_vm.c | 13 +++--- drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++-- drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++--- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 --- drivers/gpu/drm/i915/gt/intel_ring.c | 15 --- .../gpu/drm/nouveau/nvkm/subdev/clk/base.c| 11 +++-- .../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++--- drivers/gpu/drm/scheduler/sched_main.c| 14 +++--- drivers/gpu/drm/ttm/ttm_bo.c | 19 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 + drivers/infiniband/hw/hfi1/tid_rdma.c | 16 --- drivers/infiniband/hw/mlx4/main.c | 12 ++--- drivers/media/dvb-frontends/mxl5xx.c | 11 +++-- drivers/media/pci/saa7134/saa7134-alsa.c | 4 +- drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++-- drivers/misc/mei/interrupt.c | 12 ++--- .../net/ethernet/intel/i40e/i40e_ethtool.c| 3 +- .../net/ethernet/qlogic/qede/qede_filter.c| 11 +++-- drivers/net/wireless/ath/ath6kl/htc_mbox.c| 2 +- .../net/wireless/intel/ipw2x00/libipw_rx.c| 15 --- drivers/perf/xgene_pmu.c | 13 +++--- drivers/power/supply/cpcap-battery.c | 11 +++-- drivers/scsi/lpfc/lpfc_bsg.c | 16 --- drivers/scsi/scsi_transport_sas.c | 17 --- drivers/scsi/wd719x.c | 12 +++-- drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++ drivers/staging/rtl8192e/rtllib_rx.c | 17 --- .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 --- .../rtl8192u/ieee80211/rtl819x_TSProc.c | 19 drivers/thermal/thermal_core.c| 38 ++-- drivers/usb/gadget/composite.c| 9 ++--
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 12:08 schrieb Jakob Koschel: If the list does not contain the expected element, the value of list_for_each_entry() iterator will not point to a valid structure. To avoid type confusion in such case, the list iterator scope will be limited to list_for_each_entry() loop. We explicitly have the list_entry_is_head() macro to test after a loop if the element pointer points to the head of the list instead of a valid list entry. So at least from my side I absolutely don't think that this is a good idea. In preparation to limiting scope of a list iterator to the list traversal loop, use a dedicated pointer to point to the found element. Determining if an element was found is then simply checking if the pointer is != NULL. Since when do we actually want to do this? Take this code here as an example: diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 48afe96ae0f0..6c916416decc 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -450,7 +450,8 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, struct mm_struct *mm) { struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier); - struct sgx_encl_mm *tmp = NULL; + struct sgx_encl_mm *found_encl_mm = NULL; + struct sgx_encl_mm *tmp; /* * The enclave itself can remove encl_mm. Note, objects can't be moved @@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn, list_for_each_entry(tmp, _mm->encl->mm_list, list) { if (tmp == encl_mm) { list_del_rcu(_mm->list); + found_encl_mm = tmp; break; } } spin_unlock(_mm->encl->mm_lock); - if (tmp == encl_mm) { + if (found_encl_mm) { synchronize_srcu(_mm->encl->srcu); mmu_notifier_put(mn); } I don't think that using the extra variable makes the code in any way more reliable or easier to read. Regards, Christian.
Re: [PATCH V7 03/20] compat: consolidate the compat_flock{, 64} definition
On Mon, Feb 28, 2022 at 8:02 PM David Laight wrote: > > From: Guo Ren > > Sent: 28 February 2022 11:52 > > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight > > wrote: > > > > > > From: guo...@kernel.org > > > > Sent: 27 February 2022 16:28 > > > > > > > > From: Christoph Hellwig > > > > > > > > Provide a single common definition for the compat_flock and > > > > compat_flock64 structures using the same tricks as for the native > > > > variants. Another extra define is added for the packing required on > > > > x86. > > > ... > > > > diff --git a/arch/x86/include/asm/compat.h > > > > b/arch/x86/include/asm/compat.h > > > ... > > > > /* > > > > - * IA32 uses 4 byte alignment for 64 bit quantities, > > > > - * so we need to pack this structure. > > > > + * IA32 uses 4 byte alignment for 64 bit quantities, so we need to > > > > pack the > > > > + * compat flock64 structure. > > > > */ > > > ... > > > > +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > > > > > struct compat_statfs { > > > > int f_type; > > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > > index 1c758b0e0359..a0481fe6c5d5 100644 > > > > --- a/include/linux/compat.h > > > > +++ b/include/linux/compat.h > > > > @@ -258,6 +258,37 @@ struct compat_rlimit { > > > > compat_ulong_t rlim_max; > > > > }; > > > > > > > > +#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > +#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed)) > > > > +#else > > > > +#define __ARCH_COMPAT_FLOCK64_PACK > > > > +#endif > > > ... > > > > +struct compat_flock64 { > > > > + short l_type; > > > > + short l_whence; > > > > + compat_loff_t l_start; > > > > + compat_loff_t l_len; > > > > + compat_pid_tl_pid; > > > > +#ifdef __ARCH_COMPAT_FLOCK64_PAD > > > > + __ARCH_COMPAT_FLOCK64_PAD > > > > +#endif > > > > +} __ARCH_COMPAT_FLOCK64_PACK; > > > > + > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > See include/asm-generic/compat.h > > > > typedef s64 compat_loff_t; > > > > Only: > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > compat_loff_t should be compat_s64 not s64. > > The same should be done for all 64bit 'compat' types. Changing typedef s64 compat_loff_t; to typedef compat_s64 compat_loff_t; should be another patch and it affects all architectures, I don't think we should involve it in this series. look at kernel/power/user.c: struct compat_resume_swap_area { compat_loff_t offset; u32 dev; } __packed; I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for COMPAT support patchset series. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
RE: [PATCH V7 03/20] compat: consolidate the compat_flock{,64} definition
From: Guo Ren > Sent: 28 February 2022 11:52 > > On Mon, Feb 28, 2022 at 2:40 PM David Laight wrote: > > > > From: guo...@kernel.org > > > Sent: 27 February 2022 16:28 > > > > > > From: Christoph Hellwig > > > > > > Provide a single common definition for the compat_flock and > > > compat_flock64 structures using the same tricks as for the native > > > variants. Another extra define is added for the packing required on > > > x86. > > ... > > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > > ... > > > /* > > > - * IA32 uses 4 byte alignment for 64 bit quantities, > > > - * so we need to pack this structure. > > > + * IA32 uses 4 byte alignment for 64 bit quantities, so we need to pack > > > the > > > + * compat flock64 structure. > > > */ > > ... > > > +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > > > struct compat_statfs { > > > int f_type; > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > index 1c758b0e0359..a0481fe6c5d5 100644 > > > --- a/include/linux/compat.h > > > +++ b/include/linux/compat.h > > > @@ -258,6 +258,37 @@ struct compat_rlimit { > > > compat_ulong_t rlim_max; > > > }; > > > > > > +#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > +#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed)) > > > +#else > > > +#define __ARCH_COMPAT_FLOCK64_PACK > > > +#endif > > ... > > > +struct compat_flock64 { > > > + short l_type; > > > + short l_whence; > > > + compat_loff_t l_start; > > > + compat_loff_t l_len; > > > + compat_pid_tl_pid; > > > +#ifdef __ARCH_COMPAT_FLOCK64_PAD > > > + __ARCH_COMPAT_FLOCK64_PAD > > > +#endif > > > +} __ARCH_COMPAT_FLOCK64_PACK; > > > + > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > See include/asm-generic/compat.h > > typedef s64 compat_loff_t; > > Only: > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > typedef s64 __attribute__((aligned(4))) compat_s64; > > So how do you think compat_loff_t could be defined with __aligned__(4)? compat_loff_t should be compat_s64 not s64. The same should be done for all 64bit 'compat' types. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH V7 03/20] compat: consolidate the compat_flock{, 64} definition
On Mon, Feb 28, 2022 at 2:40 PM David Laight wrote: > > From: guo...@kernel.org > > Sent: 27 February 2022 16:28 > > > > From: Christoph Hellwig > > > > Provide a single common definition for the compat_flock and > > compat_flock64 structures using the same tricks as for the native > > variants. Another extra define is added for the packing required on > > x86. > ... > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > ... > > /* > > - * IA32 uses 4 byte alignment for 64 bit quantities, > > - * so we need to pack this structure. > > + * IA32 uses 4 byte alignment for 64 bit quantities, so we need to pack the > > + * compat flock64 structure. > > */ > ... > > +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED > > > > struct compat_statfs { > > int f_type; > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > index 1c758b0e0359..a0481fe6c5d5 100644 > > --- a/include/linux/compat.h > > +++ b/include/linux/compat.h > > @@ -258,6 +258,37 @@ struct compat_rlimit { > > compat_ulong_t rlim_max; > > }; > > > > +#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED > > +#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed)) > > +#else > > +#define __ARCH_COMPAT_FLOCK64_PACK > > +#endif > ... > > +struct compat_flock64 { > > + short l_type; > > + short l_whence; > > + compat_loff_t l_start; > > + compat_loff_t l_len; > > + compat_pid_tl_pid; > > +#ifdef __ARCH_COMPAT_FLOCK64_PAD > > + __ARCH_COMPAT_FLOCK64_PAD > > +#endif > > +} __ARCH_COMPAT_FLOCK64_PACK; > > + > > Provided compat_loff_t are correctly defined with __aligned__(4) See include/asm-generic/compat.h typedef s64 compat_loff_t; Only: #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT typedef s64 __attribute__((aligned(4))) compat_s64; So how do you think compat_loff_t could be defined with __aligned__(4)? > marking the structure packed isn't needed. > I believe compat_u64 and compat_s64 both have aligned(4). > It is also wrong, consider: > > struct foo { > char x; > struct compat_flock64 fl64; > }; > > There should be 3 bytes of padding after 'x'. > But you've removed it. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 3/6] treewide: fix incorrect use to determine if list is empty
On Mon, Feb 28, 2022 at 12:08:19PM +0100, Jakob Koschel wrote: > The list iterator value will *always* be set by list_for_each_entry(). > It is incorrect to assume that the iterator value will be NULL if the > list is empty. > > Instead of checking the pointer it should be checked if > the list is empty. > In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL > on the break, it is set to the correct value and leaving it > NULL if no element was found. > > Signed-off-by: Jakob Koschel > --- > arch/powerpc/sysdev/fsl_gtm.c| 4 ++-- > drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++-- > drivers/perf/xgene_pmu.c | 13 +++-- > 3 files changed, 11 insertions(+), 10 deletions(-) These are all bug fixes. 1) Send them as 3 separate patches. 2) Add Fixes tags. regards, dan carpenter
Re: [PATCH 08/11] swiotlb: make the swiotlb_init interface more useful
On Mon, Feb 28, 2022 at 02:53:39AM +, Michael Kelley (LINUX) wrote: > From: Christoph Hellwig Sent: Sunday, February 27, 2022 6:31 AM > > > > Pass a bool to pass if swiotlb needs to be enabled based on the > > addressing needs and replace the verbose argument with a set of > > flags, including one to force enable bounce buffering. > > > > Note that this patch removes the possibility to force xen-swiotlb > > use using swiotlb=force on the command line on x86 (arm and arm64 > > never supported that), but this interface will be restored shortly. > > > > Signed-off-by: Christoph Hellwig > > --- > > arch/arm/mm/init.c | 6 + > > arch/arm64/mm/init.c | 6 + > > arch/ia64/mm/init.c| 4 +-- > > arch/mips/cavium-octeon/dma-octeon.c | 2 +- > > arch/mips/loongson64/dma.c | 2 +- > > arch/mips/sibyte/common/dma.c | 2 +- > > arch/powerpc/include/asm/swiotlb.h | 1 + > > arch/powerpc/mm/mem.c | 3 ++- > > arch/powerpc/platforms/pseries/setup.c | 3 --- > > arch/riscv/mm/init.c | 8 +- > > arch/s390/mm/init.c| 3 +-- > > arch/x86/kernel/cpu/mshyperv.c | 8 -- > > arch/x86/kernel/pci-dma.c | 15 ++- > > arch/x86/mm/mem_encrypt_amd.c | 3 --- > > drivers/xen/swiotlb-xen.c | 4 +-- > > include/linux/swiotlb.h| 15 ++- > > include/trace/events/swiotlb.h | 29 - > > kernel/dma/swiotlb.c | 35 ++ > > 18 files changed, 56 insertions(+), 93 deletions(-) > > [snip] > > > > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c > > index 5a99f993e6392..568274917f1cd 100644 > > --- a/arch/x86/kernel/cpu/mshyperv.c > > +++ b/arch/x86/kernel/cpu/mshyperv.c > > @@ -336,14 +336,6 @@ static void __init ms_hyperv_init_platform(void) > > swiotlb_unencrypted_base = > > ms_hyperv.shared_gpa_boundary; > > #endif > > } > > - > > -#ifdef CONFIG_SWIOTLB > > - /* > > -* Enable swiotlb force mode in Isolation VM to > > -* use swiotlb bounce buffer for dma transaction. > > -*/ > > - swiotlb_force = SWIOTLB_FORCE; > > -#endif > > With this code removed, it's not clear to me what forces the use of the > swiotlb in a Hyper-V isolated VM. The code in pci_swiotlb_detect_4g() doesn't > catch this case because cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) > returns "false" in a Hyper-V guest. In the Hyper-V guest, it's only > cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) that returns "true". I'm > looking more closely at the meaning of the CC_ATTR_* values, and it may > be that Hyper-V should also return "true" for CC_ATTR_MEM_ENCRYPT, > but I don't think CC_ATTR_HOST_MEM_ENCRYPT should return "true". Ok, I assumed that CC_ATTR_HOST_MEM_ENCRYPT returned true in this case. I guess we just need to check for CC_ATTR_GUEST_MEM_ENCRYPT as well there?
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote: > diff --git a/drivers/usb/gadget/udc/at91_udc.c > b/drivers/usb/gadget/udc/at91_udc.c > index 9040a0561466..0fd0307bc07b 100644 > --- a/drivers/usb/gadget/udc/at91_udc.c > +++ b/drivers/usb/gadget/udc/at91_udc.c > @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct > at91_ep *ep) > if (list_empty (>queue)) > seq_printf(s, "\t(queue empty)\n"); > > - else list_for_each_entry (req, >queue, queue) { > - unsignedlength = req->req.actual; > + else > + list_for_each_entry(req, >queue, queue) { > + unsigned intlength = req->req.actual; > > - seq_printf(s, "\treq %p len %d/%d buf %p\n", > - >req, length, > - req->req.length, req->req.buf); > - } > + seq_printf(s, "\treq %p len %d/%d buf %p\n", > + >req, length, > + req->req.length, req->req.buf); > + } Don't make unrelated white space changes. It just makes the patch harder to review. As you're writing the patch make note of any additional changes and do them later in a separate patch. Also a multi-line indent gets curly braces for readability even though it's not required by C. And then both sides would get curly braces. > spin_unlock_irqrestore(>lock, flags); > } > > @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused) > > if (udc->enabled && udc->vbus) { > proc_ep_show(s, >ep[0]); > - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) { > + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) { Another unrelated change. > if (ep->ep.desc) > proc_ep_show(s, ep); > } [ snip ] > diff --git a/drivers/usb/gadget/udc/net2272.c > b/drivers/usb/gadget/udc/net2272.c > index 7c38057dcb4a..bb59200f1596 100644 > --- a/drivers/usb/gadget/udc/net2272.c > +++ b/drivers/usb/gadget/udc/net2272.c > @@ -926,7 +926,8 @@ static int > net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req) > { > struct net2272_ep *ep; > - struct net2272_request *req; > + struct net2272_request *req = NULL; > + struct net2272_request *tmp; > unsigned long flags; > int stopped; > > @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > *_req) > ep->stopped = 1; > > /* make sure it's still queued on this endpoint */ > - list_for_each_entry(req, >queue, queue) { > - if (>req == _req) > + list_for_each_entry(tmp, >queue, queue) { > + if (>req == _req) { > + req = tmp; > break; > + } > } > - if (>req != _req) { > + if (!req) { > ep->stopped = stopped; > spin_unlock_irqrestore(>dev->lock, flags); > return -EINVAL; > @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > *_req) > dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); > net2272_done(ep, req, -ECONNRESET); > } > - req = NULL; Another unrelated change. These are all good changes but send them as separate patches. > ep->stopped = stopped; > > spin_unlock_irqrestore(>dev->lock, flags); regards, dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > If the list does not contain the expected element, the value of > list_for_each_entry() iterator will not point to a valid structure. > To avoid type confusion in such case, the list iterator > scope will be limited to list_for_each_entry() loop. > > In preparation to limiting scope of a list iterator to the list traversal > loop, use a dedicated pointer to point to the found element. > Determining if an element was found is then simply checking if > the pointer is != NULL. > > Signed-off-by: Jakob Koschel > --- > arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > drivers/scsi/scsi_transport_sas.c| 17 - > drivers/thermal/thermal_core.c | 38 ++-- > drivers/usb/gadget/configfs.c| 22 ++-- > drivers/usb/gadget/udc/max3420_udc.c | 11 +--- > drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- > drivers/usb/mtu3/mtu3_gadget.c | 11 +--- > drivers/usb/musb/musb_gadget.c | 11 +--- > drivers/vfio/mdev/mdev_core.c| 11 +--- > 9 files changed, 88 insertions(+), 50 deletions(-) The drivers/usb/ portion of this patch should be in patch 1/X, right? Also, you will have to split these up per-subsystem so that the different subsystem maintainers can take these in their trees. thanks, greg k-h
Re: [PATCH V3 09/30] arm/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
On Mon, Feb 28, 2022 at 04:17:32PM +0530, Anshuman Khandual wrote: > This defines and exports a platform specific custom vm_get_page_prot() via > subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX > macros can be dropped which are no longer needed. What I would really like to know is why having to run _code_ to work out what the page protections need to be is better than looking it up in a table. Not only is this more expensive in terms of CPU cycles, it also brings additional code size with it. I'm struggling to see what the benefit is. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
[PATCH V3 30/30] mm/mmap: Drop ARCH_HAS_VM_GET_PAGE_PROT
All platforms now define their own vm_get_page_prot() and also there is no generic version left to fallback on. Hence drop ARCH_HAS_GET_PAGE_PROT. Cc: Andrew Morton Cc: linux...@kvack.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/alpha/Kconfig | 1 - arch/arc/Kconfig| 1 - arch/arm/Kconfig| 1 - arch/arm64/Kconfig | 1 - arch/csky/Kconfig | 1 - arch/hexagon/Kconfig| 1 - arch/ia64/Kconfig | 1 - arch/m68k/Kconfig | 1 - arch/microblaze/Kconfig | 1 - arch/mips/Kconfig | 1 - arch/nds32/Kconfig | 1 - arch/nios2/Kconfig | 1 - arch/openrisc/Kconfig | 1 - arch/parisc/Kconfig | 1 - arch/powerpc/Kconfig| 1 - arch/riscv/Kconfig | 1 - arch/s390/Kconfig | 1 - arch/sh/Kconfig | 1 - arch/sparc/Kconfig | 2 -- arch/um/Kconfig | 1 - arch/x86/Kconfig| 1 - arch/xtensa/Kconfig | 1 - mm/Kconfig | 3 --- mm/mmap.c | 23 --- 24 files changed, 49 deletions(-) diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index 73e82fe5c770..4e87783c90ad 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -2,7 +2,6 @@ config ALPHA bool default y - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_32BIT_USTAT_F_TINODE select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig index 78ff0644b343..3c2a4753d09b 100644 --- a/arch/arc/Kconfig +++ b/arch/arc/Kconfig @@ -13,7 +13,6 @@ config ARC select ARCH_HAS_SETUP_DMA_OPS select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_SUPPORTS_ATOMIC_RMW if ARC_HAS_LLSC select ARCH_32BIT_OFF_T select BUILDTIME_TABLE_SORT diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 87b2e89ef3d6..4c97cb40eebb 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -23,7 +23,6 @@ config ARM select ARCH_HAS_SYNC_DMA_FOR_CPU if SWIOTLB || !MMU select ARCH_HAS_TEARDOWN_DMA_OPS if MMU select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAVE_CUSTOM_GPIO_H select ARCH_HAVE_NMI_SAFE_CMPXCHG if CPU_V7 || CPU_V7M || CPU_V6K select ARCH_HAS_GCOV_PROFILE_ALL diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 7153d5fff603..bfb92b98d5aa 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -43,7 +43,6 @@ config ARM64 select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_TEARDOWN_DMA_OPS if IOMMU_SUPPORT select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAS_ZONE_DMA_SET if EXPERT select ARCH_HAVE_ELF_PROT select ARCH_HAVE_NMI_SAFE_CMPXCHG diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig index 209dac5686dd..132f43f12dd8 100644 --- a/arch/csky/Kconfig +++ b/arch/csky/Kconfig @@ -6,7 +6,6 @@ config CSKY select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_USE_BUILTIN_BSWAP select ARCH_USE_QUEUED_RWLOCKS select ARCH_WANT_FRAME_POINTERS if !CPU_CK610 && $(cc-option,-mbacktrace) diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig index cdc5df32a1e3..15dd8f38b698 100644 --- a/arch/hexagon/Kconfig +++ b/arch/hexagon/Kconfig @@ -6,7 +6,6 @@ config HEXAGON def_bool y select ARCH_32BIT_OFF_T select ARCH_HAS_SYNC_DMA_FOR_DEVICE - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_NO_PREEMPT select DMA_GLOBAL_POOL # Other pending projects/to-do items. diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index 0ab15e8d5783..a7e01573abd8 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -11,7 +11,6 @@ config IA64 select ARCH_HAS_DMA_MARK_CLEAN select ARCH_HAS_STRNCPY_FROM_USER select ARCH_HAS_STRNLEN_USER - select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO select ACPI diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig index 114e65164692..936e1803c7c7 100644 --- a/arch/m68k/Kconfig +++ b/arch/m68k/Kconfig @@ -11,7 +11,6 @@ config M68K select ARCH_NO_PREEMPT if !COLDFIRE select ARCH_USE_MEMTEST if MMU_MOTOROLA select ARCH_WANT_IPC_PARSE_VERSION - select ARCH_HAS_VM_GET_PAGE_PROT select BINFMT_FLAT_ARGVP_ENVP_ON_STACK select DMA_DIRECT_REMAP if HAS_DMA && MMU && !COLDFIRE select GENERIC_ATOMIC64 diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig index f2c25ba8621e..59798e43cdb0 100644 --- a/arch/microblaze/Kconfig +++ b/arch/microblaze/Kconfig @@ -7,7 +7,6 @@ config MICROBLAZE
[PATCH V3 29/30] mm/mmap: Drop generic vm_get_page_prot()
All available platforms export their own vm_get_page_prot() implementation via ARCH_HAS_VM_GET_PAGE_PROT. Hence a generic implementation is no longer needed. Cc: Andrew Morton Cc: linux...@kvack.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- mm/mmap.c | 40 1 file changed, 40 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index c13dd9c37866..ff343f76a825 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -102,46 +102,6 @@ static void unmap_region(struct mm_struct *mm, * w: (no) no * x: (yes) yes */ -pgprot_t vm_get_page_prot(unsigned long vm_flags) -{ - switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { - case VM_NONE: - return __P000; - case VM_READ: - return __P001; - case VM_WRITE: - return __P010; - case VM_READ | VM_WRITE: - return __P011; - case VM_EXEC: - return __P100; - case VM_EXEC | VM_READ: - return __P101; - case VM_EXEC | VM_WRITE: - return __P110; - case VM_EXEC | VM_READ | VM_WRITE: - return __P111; - case VM_SHARED: - return __S000; - case VM_SHARED | VM_READ: - return __S001; - case VM_SHARED | VM_WRITE: - return __S010; - case VM_SHARED | VM_READ | VM_WRITE: - return __S011; - case VM_SHARED | VM_EXEC: - return __S100; - case VM_SHARED | VM_EXEC | VM_READ: - return __S101; - case VM_SHARED | VM_EXEC | VM_WRITE: - return __S110; - case VM_SHARED | VM_EXEC | VM_READ | VM_WRITE: - return __S111; - default: - BUILD_BUG(); - } -} -EXPORT_SYMBOL(vm_get_page_prot); #endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags) -- 2.25.1
[PATCH V3 28/30] ia64/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: linux-i...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/ia64/Kconfig | 1 + arch/ia64/include/asm/pgtable.h | 17 -- arch/ia64/mm/init.c | 41 - 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index a7e01573abd8..0ab15e8d5783 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -11,6 +11,7 @@ config IA64 select ARCH_HAS_DMA_MARK_CLEAN select ARCH_HAS_STRNCPY_FROM_USER select ARCH_HAS_STRNLEN_USER + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO select ACPI diff --git a/arch/ia64/include/asm/pgtable.h b/arch/ia64/include/asm/pgtable.h index 9584b2c5f394..8154c78bba56 100644 --- a/arch/ia64/include/asm/pgtable.h +++ b/arch/ia64/include/asm/pgtable.h @@ -161,23 +161,6 @@ * attempts to write to the page. */ /* xwr */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_READONLY /* write to priv pg -> copy & make writable */ -#define __P011 PAGE_READONLY /* ditto */ -#define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX) -#define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX) -#define __P110 PAGE_COPY_EXEC -#define __P111 PAGE_COPY_EXEC - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED /* we don't have (and don't need) write-only */ -#define __S011 PAGE_SHARED -#define __S100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX) -#define __S101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX) -#define __S110 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX) -#define __S111 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX) #define pgd_ERROR(e) printk("%s:%d: bad pgd %016lx.\n", __FILE__, __LINE__, pgd_val(e)) #if CONFIG_PGTABLE_LEVELS == 4 diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c index 5d165607bf35..2a922883e30f 100644 --- a/arch/ia64/mm/init.c +++ b/arch/ia64/mm/init.c @@ -273,7 +273,7 @@ static int __init gate_vma_init(void) gate_vma.vm_start = FIXADDR_USER_START; gate_vma.vm_end = FIXADDR_USER_END; gate_vma.vm_flags = VM_READ | VM_MAYREAD | VM_EXEC | VM_MAYEXEC; - gate_vma.vm_page_prot = __P101; + gate_vma.vm_page_prot = __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX); return 0; } @@ -492,3 +492,42 @@ void arch_remove_memory(u64 start, u64 size, struct vmem_altmap *altmap) __remove_pages(start_pfn, nr_pages, altmap); } #endif + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + /* write to priv pg -> copy & make writable */ + case VM_WRITE: + /* write to priv pg -> copy & make writable */ + case VM_WRITE | VM_READ: + return PAGE_READONLY; + case VM_EXEC: + return __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX); + case VM_EXEC | VM_READ: + return __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX); + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY_EXEC; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + /* we don't have (and don't need) write-only */ + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + return __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX); + case VM_SHARED | VM_EXEC | VM_READ: + return __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX); + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX); + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 27/30] nds32/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Nick Hu Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/nds32/Kconfig | 1 + arch/nds32/include/asm/pgtable.h | 17 --- arch/nds32/mm/mmap.c | 37 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/arch/nds32/Kconfig b/arch/nds32/Kconfig index 4d1421b18734..576e05479925 100644 --- a/arch/nds32/Kconfig +++ b/arch/nds32/Kconfig @@ -10,6 +10,7 @@ config NDS32 select ARCH_HAS_DMA_PREP_COHERENT select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_WANT_FRAME_POINTERS if FTRACE select CLKSRC_MMIO select CLONE_BACKWARDS diff --git a/arch/nds32/include/asm/pgtable.h b/arch/nds32/include/asm/pgtable.h index 419f984eef70..79f64ed734cb 100644 --- a/arch/nds32/include/asm/pgtable.h +++ b/arch/nds32/include/asm/pgtable.h @@ -152,23 +152,6 @@ extern void __pgd_error(const char *file, int line, unsigned long val); #endif /* __ASSEMBLY__ */ /* xwr */ -#define __P000 (PAGE_NONE | _PAGE_CACHE_SHRD) -#define __P001 (PAGE_READ | _PAGE_CACHE_SHRD) -#define __P010 (PAGE_COPY | _PAGE_CACHE_SHRD) -#define __P011 (PAGE_COPY | _PAGE_CACHE_SHRD) -#define __P100 (PAGE_EXEC | _PAGE_CACHE_SHRD) -#define __P101 (PAGE_READ | _PAGE_E | _PAGE_CACHE_SHRD) -#define __P110 (PAGE_COPY | _PAGE_E | _PAGE_CACHE_SHRD) -#define __P111 (PAGE_COPY | _PAGE_E | _PAGE_CACHE_SHRD) - -#define __S000 (PAGE_NONE | _PAGE_CACHE_SHRD) -#define __S001 (PAGE_READ | _PAGE_CACHE_SHRD) -#define __S010 (PAGE_RDWR | _PAGE_CACHE_SHRD) -#define __S011 (PAGE_RDWR | _PAGE_CACHE_SHRD) -#define __S100 (PAGE_EXEC | _PAGE_CACHE_SHRD) -#define __S101 (PAGE_READ | _PAGE_E | _PAGE_CACHE_SHRD) -#define __S110 (PAGE_RDWR | _PAGE_E | _PAGE_CACHE_SHRD) -#define __S111 (PAGE_RDWR | _PAGE_E | _PAGE_CACHE_SHRD) #ifndef __ASSEMBLY__ /* diff --git a/arch/nds32/mm/mmap.c b/arch/nds32/mm/mmap.c index 1bdf5e7d1b43..0399b928948d 100644 --- a/arch/nds32/mm/mmap.c +++ b/arch/nds32/mm/mmap.c @@ -71,3 +71,40 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, info.align_offset = pgoff << PAGE_SHIFT; return vm_unmapped_area(); } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return (PAGE_NONE | _PAGE_CACHE_SHRD); + case VM_READ: + return (PAGE_READ | _PAGE_CACHE_SHRD); + case VM_WRITE: + case VM_WRITE | VM_READ: + return (PAGE_COPY | _PAGE_CACHE_SHRD); + case VM_EXEC: + return (PAGE_EXEC | _PAGE_CACHE_SHRD); + case VM_EXEC | VM_READ: + return (PAGE_READ | _PAGE_E | _PAGE_CACHE_SHRD); + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return (PAGE_COPY | _PAGE_E | _PAGE_CACHE_SHRD); + case VM_SHARED: + return (PAGE_NONE | _PAGE_CACHE_SHRD); + case VM_SHARED | VM_READ: + return (PAGE_READ | _PAGE_CACHE_SHRD); + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return (PAGE_RDWR | _PAGE_CACHE_SHRD); + case VM_SHARED | VM_EXEC: + return (PAGE_EXEC | _PAGE_CACHE_SHRD); + case VM_SHARED | VM_EXEC | VM_READ: + return (PAGE_READ | _PAGE_E | _PAGE_CACHE_SHRD); + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return (PAGE_RDWR | _PAGE_E | _PAGE_CACHE_SHRD); + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 26/30] hexagon/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Brian Cain Cc: linux-hexa...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/hexagon/Kconfig | 1 + arch/hexagon/include/asm/pgtable.h | 24 --- arch/hexagon/mm/init.c | 67 ++ 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig index 15dd8f38b698..cdc5df32a1e3 100644 --- a/arch/hexagon/Kconfig +++ b/arch/hexagon/Kconfig @@ -6,6 +6,7 @@ config HEXAGON def_bool y select ARCH_32BIT_OFF_T select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_NO_PREEMPT select DMA_GLOBAL_POOL # Other pending projects/to-do items. diff --git a/arch/hexagon/include/asm/pgtable.h b/arch/hexagon/include/asm/pgtable.h index 18cd6ea9ab23..5eceddfe013d 100644 --- a/arch/hexagon/include/asm/pgtable.h +++ b/arch/hexagon/include/asm/pgtable.h @@ -127,31 +127,7 @@ extern unsigned long _dflt_cache_att; #define CACHEDEF (CACHE_DEFAULT << 6) /* Private (copy-on-write) page protections. */ -#define __P000 __pgprot(_PAGE_PRESENT | _PAGE_USER | CACHEDEF) -#define __P001 __pgprot(_PAGE_PRESENT | _PAGE_USER | _PAGE_READ | CACHEDEF) -#define __P010 __P000 /* Write-only copy-on-write */ -#define __P011 __P001 /* Read/Write copy-on-write */ -#define __P100 __pgprot(_PAGE_PRESENT | _PAGE_USER | \ - _PAGE_EXECUTE | CACHEDEF) -#define __P101 __pgprot(_PAGE_PRESENT | _PAGE_USER | _PAGE_EXECUTE | \ - _PAGE_READ | CACHEDEF) -#define __P110 __P100 /* Write/execute copy-on-write */ -#define __P111 __P101 /* Read/Write/Execute, copy-on-write */ - /* Shared page protections. */ -#define __S000 __P000 -#define __S001 __P001 -#define __S010 __pgprot(_PAGE_PRESENT | _PAGE_USER | \ - _PAGE_WRITE | CACHEDEF) -#define __S011 __pgprot(_PAGE_PRESENT | _PAGE_USER | _PAGE_READ | \ - _PAGE_WRITE | CACHEDEF) -#define __S100 __pgprot(_PAGE_PRESENT | _PAGE_USER | \ - _PAGE_EXECUTE | CACHEDEF) -#define __S101 __P101 -#define __S110 __pgprot(_PAGE_PRESENT | _PAGE_USER | \ - _PAGE_EXECUTE | _PAGE_WRITE | CACHEDEF) -#define __S111 __pgprot(_PAGE_PRESENT | _PAGE_USER | _PAGE_READ | \ - _PAGE_EXECUTE | _PAGE_WRITE | CACHEDEF) extern pgd_t swapper_pg_dir[PTRS_PER_PGD]; /* located in head.S */ diff --git a/arch/hexagon/mm/init.c b/arch/hexagon/mm/init.c index f01e91e10d95..b53595fc4103 100644 --- a/arch/hexagon/mm/init.c +++ b/arch/hexagon/mm/init.c @@ -236,3 +236,70 @@ void __init setup_arch_memory(void) * which is called by start_kernel() later on in the process */ } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + CACHEDEF); + case VM_READ: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_READ | CACHEDEF); + /* Write-only copy-on-write */ + case VM_WRITE: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + CACHEDEF); + /* Read/Write copy-on-write */ + case VM_WRITE | VM_READ: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_READ | CACHEDEF); + case VM_EXEC: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_EXECUTE | CACHEDEF); + case VM_EXEC | VM_READ: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_EXECUTE | _PAGE_READ | + CACHEDEF); + /* Write/execute copy-on-write */ + case VM_EXEC | VM_WRITE: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_EXECUTE | CACHEDEF); + /* Read/Write/Execute, copy-on-write */ + case VM_EXEC | VM_WRITE | VM_READ: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_EXECUTE | _PAGE_READ | + CACHEDEF); + case VM_SHARED: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + CACHEDEF); + case VM_SHARED | VM_READ: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_READ | CACHEDEF); + case VM_SHARED | VM_WRITE: + return __pgprot(_PAGE_PRESENT | _PAGE_USER | + _PAGE_WRITE | CACHEDEF); + case VM_SHARED | VM_WRITE |
[PATCH V3 25/30] nios2/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Dinh Nguyen Cc: linux-ker...@vger.kernel.org Acked-by: Dinh Nguyen Signed-off-by: Anshuman Khandual --- arch/nios2/Kconfig | 1 + arch/nios2/include/asm/pgtable.h | 24 arch/nios2/mm/init.c | 47 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/arch/nios2/Kconfig b/arch/nios2/Kconfig index 33fd06f5fa41..85a58a357a3b 100644 --- a/arch/nios2/Kconfig +++ b/arch/nios2/Kconfig @@ -6,6 +6,7 @@ config NIOS2 select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE select ARCH_HAS_DMA_SET_UNCACHED + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_NO_SWAP select COMMON_CLK select TIMER_OF diff --git a/arch/nios2/include/asm/pgtable.h b/arch/nios2/include/asm/pgtable.h index 4a995fa628ee..ba3f9822c0b3 100644 --- a/arch/nios2/include/asm/pgtable.h +++ b/arch/nios2/include/asm/pgtable.h @@ -34,30 +34,6 @@ struct mm_struct; ((x) ? _PAGE_EXEC : 0) |\ ((r) ? _PAGE_READ : 0) |\ ((w) ? _PAGE_WRITE : 0)) -/* - * These are the macros that generic kernel code needs - * (to populate protection_map[]) - */ - -/* Remove W bit on private pages for COW support */ -#define __P000 MKP(0, 0, 0) -#define __P001 MKP(0, 0, 1) -#define __P010 MKP(0, 0, 0)/* COW */ -#define __P011 MKP(0, 0, 1)/* COW */ -#define __P100 MKP(1, 0, 0) -#define __P101 MKP(1, 0, 1) -#define __P110 MKP(1, 0, 0)/* COW */ -#define __P111 MKP(1, 0, 1)/* COW */ - -/* Shared pages can have exact HW mapping */ -#define __S000 MKP(0, 0, 0) -#define __S001 MKP(0, 0, 1) -#define __S010 MKP(0, 1, 0) -#define __S011 MKP(0, 1, 1) -#define __S100 MKP(1, 0, 0) -#define __S101 MKP(1, 0, 1) -#define __S110 MKP(1, 1, 0) -#define __S111 MKP(1, 1, 1) /* Used all over the kernel */ #define PAGE_KERNEL __pgprot(_PAGE_PRESENT | _PAGE_CACHED | _PAGE_READ | \ diff --git a/arch/nios2/mm/init.c b/arch/nios2/mm/init.c index 613fcaa5988a..e867f5d85580 100644 --- a/arch/nios2/mm/init.c +++ b/arch/nios2/mm/init.c @@ -124,3 +124,50 @@ const char *arch_vma_name(struct vm_area_struct *vma) { return (vma->vm_start == KUSER_BASE) ? "[kuser]" : NULL; } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + /* Remove W bit on private pages for COW support */ + case VM_NONE: + return MKP(0, 0, 0); + case VM_READ: + return MKP(0, 0, 1); + /* COW */ + case VM_WRITE: + return MKP(0, 0, 0); + /* COW */ + case VM_WRITE | VM_READ: + return MKP(0, 0, 1); + case VM_EXEC: + return MKP(1, 0, 0); + case VM_EXEC | VM_READ: + return MKP(1, 0, 1); + /* COW */ + case VM_EXEC | VM_WRITE: + return MKP(1, 0, 0); + /* COW */ + case VM_EXEC | VM_WRITE | VM_READ: + return MKP(1, 0, 1); + /* Shared pages can have exact HW mapping */ + case VM_SHARED: + return MKP(0, 0, 0); + case VM_SHARED | VM_READ: + return MKP(0, 0, 1); + case VM_SHARED | VM_WRITE: + return MKP(0, 1, 0); + case VM_SHARED | VM_WRITE | VM_READ: + return MKP(0, 1, 1); + case VM_SHARED | VM_EXEC: + return MKP(1, 0, 0); + case VM_SHARED | VM_EXEC | VM_READ: + return MKP(1, 0, 1); + case VM_SHARED | VM_EXEC | VM_WRITE: + return MKP(1, 1, 0); + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return MKP(1, 1, 1); + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 24/30] microblaze/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Michal Simek Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/microblaze/Kconfig | 1 + arch/microblaze/include/asm/pgtable.h | 17 --- arch/microblaze/mm/init.c | 41 +++ 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig index 59798e43cdb0..f2c25ba8621e 100644 --- a/arch/microblaze/Kconfig +++ b/arch/microblaze/Kconfig @@ -7,6 +7,7 @@ config MICROBLAZE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_WANT_IPC_PARSE_VERSION select BUILDTIME_TABLE_SORT diff --git a/arch/microblaze/include/asm/pgtable.h b/arch/microblaze/include/asm/pgtable.h index c136a01e467e..6df373077ff2 100644 --- a/arch/microblaze/include/asm/pgtable.h +++ b/arch/microblaze/include/asm/pgtable.h @@ -204,23 +204,6 @@ extern pte_t *va_to_pte(unsigned long address); * We consider execute permission the same as read. * Also, write permissions imply read permissions. */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY_X -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY_X -#define __P100 PAGE_READONLY -#define __P101 PAGE_READONLY_X -#define __P110 PAGE_COPY -#define __P111 PAGE_COPY_X - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY_X -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED_X -#define __S100 PAGE_READONLY -#define __S101 PAGE_READONLY_X -#define __S110 PAGE_SHARED -#define __S111 PAGE_SHARED_X #ifndef __ASSEMBLY__ /* diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c index 952f35b335b2..68faf7d04faf 100644 --- a/arch/microblaze/mm/init.c +++ b/arch/microblaze/mm/init.c @@ -280,3 +280,44 @@ void * __ref zalloc_maybe_bootmem(size_t size, gfp_t mask) return p; } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY_X; + case VM_WRITE: + return PAGE_COPY; + case VM_WRITE | VM_READ: + return PAGE_COPY_X; + case VM_EXEC: + return PAGE_READONLY; + case VM_EXEC | VM_READ: + return PAGE_READONLY_X; + case VM_EXEC | VM_WRITE: + return PAGE_COPY; + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY_X; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY_X; + case VM_SHARED | VM_WRITE: + return PAGE_SHARED; + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED_X; + case VM_SHARED | VM_EXEC: + return PAGE_READONLY; + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READONLY_X; + case VM_SHARED | VM_EXEC | VM_WRITE: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED_X; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 23/30] um/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Jeff Dike Cc: linux...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/um/Kconfig | 1 + arch/um/include/asm/pgtable.h | 17 - arch/um/kernel/mem.c | 35 +++ arch/x86/um/mem_32.c | 2 +- 4 files changed, 37 insertions(+), 18 deletions(-) diff --git a/arch/um/Kconfig b/arch/um/Kconfig index 4d398b80aea8..5836296868a8 100644 --- a/arch/um/Kconfig +++ b/arch/um/Kconfig @@ -9,6 +9,7 @@ config UML select ARCH_HAS_KCOV select ARCH_HAS_STRNCPY_FROM_USER select ARCH_HAS_STRNLEN_USER + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_NO_PREEMPT select HAVE_ARCH_AUDITSYSCALL select HAVE_ARCH_SECCOMP_FILTER diff --git a/arch/um/include/asm/pgtable.h b/arch/um/include/asm/pgtable.h index b9e20bbe2f75..d982622c0708 100644 --- a/arch/um/include/asm/pgtable.h +++ b/arch/um/include/asm/pgtable.h @@ -68,23 +68,6 @@ extern unsigned long end_iomem; * Also, write permissions imply read permissions. This is the closest we can * get.. */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY -#define __P100 PAGE_READONLY -#define __P101 PAGE_READONLY -#define __P110 PAGE_COPY -#define __P111 PAGE_COPY - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_READONLY -#define __S101 PAGE_READONLY -#define __S110 PAGE_SHARED -#define __S111 PAGE_SHARED /* * ZERO_PAGE is a global shared page that is always zero: used diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c index 15295c3237a0..37c6c7b9dadc 100644 --- a/arch/um/kernel/mem.c +++ b/arch/um/kernel/mem.c @@ -197,3 +197,38 @@ void *uml_kmalloc(int size, int flags) { return kmalloc(size, flags); } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY; + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_EXEC: + case VM_EXEC | VM_READ: + return PAGE_READONLY; + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); diff --git a/arch/x86/um/mem_32.c b/arch/x86/um/mem_32.c index 19c5dbd46770..cafd01f730da 100644 --- a/arch/x86/um/mem_32.c +++ b/arch/x86/um/mem_32.c @@ -17,7 +17,7 @@ static int __init gate_vma_init(void) gate_vma.vm_start = FIXADDR_USER_START; gate_vma.vm_end = FIXADDR_USER_END; gate_vma.vm_flags = VM_READ | VM_MAYREAD | VM_EXEC | VM_MAYEXEC; - gate_vma.vm_page_prot = __P101; + gate_vma.vm_page_prot = PAGE_READONLY; return 0; } -- 2.25.1
[PATCH V3 22/30] openrisc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Jonas Bonn Cc: openr...@lists.librecores.org Cc: linux-ker...@vger.kernel.org Acked-by: Stafford Horne Signed-off-by: Anshuman Khandual --- arch/openrisc/Kconfig | 1 + arch/openrisc/include/asm/pgtable.h | 18 - arch/openrisc/mm/init.c | 41 + 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig index f724b3f1aeed..842a61426816 100644 --- a/arch/openrisc/Kconfig +++ b/arch/openrisc/Kconfig @@ -10,6 +10,7 @@ config OPENRISC select ARCH_HAS_DMA_SET_UNCACHED select ARCH_HAS_DMA_CLEAR_UNCACHED select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select COMMON_CLK select OF select OF_EARLY_FLATTREE diff --git a/arch/openrisc/include/asm/pgtable.h b/arch/openrisc/include/asm/pgtable.h index cdd657f80bfa..fe686c4b7065 100644 --- a/arch/openrisc/include/asm/pgtable.h +++ b/arch/openrisc/include/asm/pgtable.h @@ -176,24 +176,6 @@ extern void paging_init(void); __pgprot(_PAGE_ALL | _PAGE_SRE | _PAGE_SWE \ | _PAGE_SHARED | _PAGE_DIRTY | _PAGE_EXEC | _PAGE_CI) -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY_X -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY_X -#define __P100 PAGE_READONLY -#define __P101 PAGE_READONLY_X -#define __P110 PAGE_COPY -#define __P111 PAGE_COPY_X - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY_X -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED_X -#define __S100 PAGE_READONLY -#define __S101 PAGE_READONLY_X -#define __S110 PAGE_SHARED -#define __S111 PAGE_SHARED_X - /* zero page used for uninitialized stuff */ extern unsigned long empty_zero_page[2048]; #define ZERO_PAGE(vaddr) (virt_to_page(empty_zero_page)) diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c index 97305bde1b16..0d06e3ebef5d 100644 --- a/arch/openrisc/mm/init.c +++ b/arch/openrisc/mm/init.c @@ -210,3 +210,44 @@ void __init mem_init(void) mem_init_done = 1; return; } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY_X; + case VM_WRITE: + return PAGE_COPY; + case VM_WRITE | VM_READ: + return PAGE_COPY_X; + case VM_EXEC: + return PAGE_READONLY; + case VM_EXEC | VM_READ: + return PAGE_READONLY_X; + case VM_EXEC | VM_WRITE: + return PAGE_COPY; + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY_X; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY_X; + case VM_SHARED | VM_WRITE: + return PAGE_SHARED; + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED_X; + case VM_SHARED | VM_EXEC: + return PAGE_READONLY; + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READONLY_X; + case VM_SHARED | VM_EXEC | VM_WRITE: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED_X; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 21/30] parisc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: "James E.J. Bottomley" Cc: linux-par...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/parisc/Kconfig | 1 + arch/parisc/include/asm/pgtable.h | 20 arch/parisc/mm/init.c | 40 +++ 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig index 43c1c880def6..de512f120b50 100644 --- a/arch/parisc/Kconfig +++ b/arch/parisc/Kconfig @@ -10,6 +10,7 @@ config PARISC select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_STRICT_KERNEL_RWX select ARCH_HAS_UBSAN_SANITIZE_ALL + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_NO_SG_CHAIN select ARCH_SUPPORTS_HUGETLBFS if PA20 select ARCH_SUPPORTS_MEMORY_FAILURE diff --git a/arch/parisc/include/asm/pgtable.h b/arch/parisc/include/asm/pgtable.h index 3e7cf882639f..80d99b2b5913 100644 --- a/arch/parisc/include/asm/pgtable.h +++ b/arch/parisc/include/asm/pgtable.h @@ -269,26 +269,6 @@ extern void __update_cache(pte_t pte); * pages. */ -/*xwr*/ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 __P000 /* copy on write */ -#define __P011 __P001 /* copy on write */ -#define __P100 PAGE_EXECREAD -#define __P101 PAGE_EXECREAD -#define __P110 __P100 /* copy on write */ -#define __P111 __P101 /* copy on write */ - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_WRITEONLY -#define __S011 PAGE_SHARED -#define __S100 PAGE_EXECREAD -#define __S101 PAGE_EXECREAD -#define __S110 PAGE_RWX -#define __S111 PAGE_RWX - - extern pgd_t swapper_pg_dir[]; /* declared in init_task.c */ /* initial page tables for 0-8MB for kernel */ diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c index 1dc2e88e7b04..f9e841f874a8 100644 --- a/arch/parisc/mm/init.c +++ b/arch/parisc/mm/init.c @@ -865,3 +865,43 @@ void flush_tlb_all(void) spin_unlock(_lock); } #endif + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY; + /* copy on write */ + case VM_WRITE: + return PAGE_NONE; + /* copy on write */ + case VM_WRITE | VM_READ: + return PAGE_READONLY; + case VM_EXEC: + case VM_EXEC | VM_READ: + /* copy on write */ + case VM_EXEC | VM_WRITE: + /* copy on write */ + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_EXECREAD; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_WRITE: + return PAGE_WRITEONLY; + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_EXECREAD; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_RWX; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 20/30] xtensa/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Chris Zankel Cc: Guo Ren Cc: linux-xte...@linux-xtensa.org Cc: linux-c...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/xtensa/Kconfig | 1 + arch/xtensa/include/asm/pgtable.h | 19 - arch/xtensa/mm/init.c | 35 +++ 3 files changed, 36 insertions(+), 19 deletions(-) diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig index 8ac599aa6d99..1608f7517546 100644 --- a/arch/xtensa/Kconfig +++ b/arch/xtensa/Kconfig @@ -9,6 +9,7 @@ config XTENSA select ARCH_HAS_DMA_SET_UNCACHED if MMU select ARCH_HAS_STRNCPY_FROM_USER if !KASAN select ARCH_HAS_STRNLEN_USER + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_USE_MEMTEST select ARCH_USE_QUEUED_RWLOCKS select ARCH_USE_QUEUED_SPINLOCKS diff --git a/arch/xtensa/include/asm/pgtable.h b/arch/xtensa/include/asm/pgtable.h index bd5aeb795567..509f765281d8 100644 --- a/arch/xtensa/include/asm/pgtable.h +++ b/arch/xtensa/include/asm/pgtable.h @@ -198,26 +198,7 @@ * the MMU can't do page protection for execute, and considers that the same as * read. Also, write permissions may imply read permissions. * What follows is the closest we can get by reasonable means.. - * See linux/mm/mmap.c for protection_map[] array that uses these definitions. */ -#define __P000 PAGE_NONE /* private --- */ -#define __P001 PAGE_READONLY /* private --r */ -#define __P010 PAGE_COPY /* private -w- */ -#define __P011 PAGE_COPY /* private -wr */ -#define __P100 PAGE_READONLY_EXEC /* private x-- */ -#define __P101 PAGE_READONLY_EXEC /* private x-r */ -#define __P110 PAGE_COPY_EXEC /* private xw- */ -#define __P111 PAGE_COPY_EXEC /* private xwr */ - -#define __S000 PAGE_NONE /* shared --- */ -#define __S001 PAGE_READONLY /* shared --r */ -#define __S010 PAGE_SHARED /* shared -w- */ -#define __S011 PAGE_SHARED /* shared -wr */ -#define __S100 PAGE_READONLY_EXEC /* shared x-- */ -#define __S101 PAGE_READONLY_EXEC /* shared x-r */ -#define __S110 PAGE_SHARED_EXEC/* shared xw- */ -#define __S111 PAGE_SHARED_EXEC/* shared xwr */ - #ifndef __ASSEMBLY__ #define pte_ERROR(e) \ diff --git a/arch/xtensa/mm/init.c b/arch/xtensa/mm/init.c index 6a32b2cf2718..5f090749e9e0 100644 --- a/arch/xtensa/mm/init.c +++ b/arch/xtensa/mm/init.c @@ -216,3 +216,38 @@ static int __init parse_memmap_opt(char *str) return 0; } early_param("memmap", parse_memmap_opt); + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY; + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_EXEC: + case VM_EXEC | VM_READ: + return PAGE_READONLY_EXEC; + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY_EXEC; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READONLY_EXEC; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED_EXEC; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 19/30] csky/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Geert Uytterhoeven Cc: linux-c...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/csky/Kconfig | 1 + arch/csky/include/asm/pgtable.h | 18 -- arch/csky/mm/init.c | 32 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig index 132f43f12dd8..209dac5686dd 100644 --- a/arch/csky/Kconfig +++ b/arch/csky/Kconfig @@ -6,6 +6,7 @@ config CSKY select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_USE_BUILTIN_BSWAP select ARCH_USE_QUEUED_RWLOCKS select ARCH_WANT_FRAME_POINTERS if !CPU_CK610 && $(cc-option,-mbacktrace) diff --git a/arch/csky/include/asm/pgtable.h b/arch/csky/include/asm/pgtable.h index 151607ed5158..2c6b1cfb1cce 100644 --- a/arch/csky/include/asm/pgtable.h +++ b/arch/csky/include/asm/pgtable.h @@ -76,24 +76,6 @@ #define MAX_SWAPFILES_CHECK() \ BUILD_BUG_ON(MAX_SWAPFILES_SHIFT != 5) -#define __P000 PAGE_NONE -#define __P001 PAGE_READ -#define __P010 PAGE_READ -#define __P011 PAGE_READ -#define __P100 PAGE_READ -#define __P101 PAGE_READ -#define __P110 PAGE_READ -#define __P111 PAGE_READ - -#define __S000 PAGE_NONE -#define __S001 PAGE_READ -#define __S010 PAGE_WRITE -#define __S011 PAGE_WRITE -#define __S100 PAGE_READ -#define __S101 PAGE_READ -#define __S110 PAGE_WRITE -#define __S111 PAGE_WRITE - extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)]; #define ZERO_PAGE(vaddr) (virt_to_page(empty_zero_page)) diff --git a/arch/csky/mm/init.c b/arch/csky/mm/init.c index bf2004aa811a..f9babbed17d4 100644 --- a/arch/csky/mm/init.c +++ b/arch/csky/mm/init.c @@ -197,3 +197,35 @@ void __init fixaddr_init(void) vaddr = __fix_to_virt(__end_of_fixed_addresses - 1) & PMD_MASK; fixrange_init(vaddr, vaddr + PMD_SIZE, swapper_pg_dir); } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + case VM_WRITE: + case VM_WRITE | VM_READ: + case VM_EXEC: + case VM_EXEC | VM_READ: + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_READ; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READ; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_WRITE; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READ; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_WRITE; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 18/30] arc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Vineet Gupta Cc: linux-snps-...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/arc/Kconfig | 1 + arch/arc/include/asm/pgtable-bits-arcv2.h | 17 -- arch/arc/mm/mmap.c| 41 +++ 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig index 3c2a4753d09b..78ff0644b343 100644 --- a/arch/arc/Kconfig +++ b/arch/arc/Kconfig @@ -13,6 +13,7 @@ config ARC select ARCH_HAS_SETUP_DMA_OPS select ARCH_HAS_SYNC_DMA_FOR_CPU select ARCH_HAS_SYNC_DMA_FOR_DEVICE + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_SUPPORTS_ATOMIC_RMW if ARC_HAS_LLSC select ARCH_32BIT_OFF_T select BUILDTIME_TABLE_SORT diff --git a/arch/arc/include/asm/pgtable-bits-arcv2.h b/arch/arc/include/asm/pgtable-bits-arcv2.h index 183d23bc1e00..798308f4dbad 100644 --- a/arch/arc/include/asm/pgtable-bits-arcv2.h +++ b/arch/arc/include/asm/pgtable-bits-arcv2.h @@ -72,23 +72,6 @@ * This is to enable COW mechanism */ /* xwr */ -#define __P000 PAGE_U_NONE -#define __P001 PAGE_U_R -#define __P010 PAGE_U_R /* Pvt-W => !W */ -#define __P011 PAGE_U_R /* Pvt-W => !W */ -#define __P100 PAGE_U_X_R /* X => R */ -#define __P101 PAGE_U_X_R -#define __P110 PAGE_U_X_R /* Pvt-W => !W and X => R */ -#define __P111 PAGE_U_X_R /* Pvt-W => !W */ - -#define __S000 PAGE_U_NONE -#define __S001 PAGE_U_R -#define __S010 PAGE_U_W_R /* W => R */ -#define __S011 PAGE_U_W_R -#define __S100 PAGE_U_X_R /* X => R */ -#define __S101 PAGE_U_X_R -#define __S110 PAGE_U_X_W_R /* X => R */ -#define __S111 PAGE_U_X_W_R #ifndef __ASSEMBLY__ diff --git a/arch/arc/mm/mmap.c b/arch/arc/mm/mmap.c index 722d26b94307..d286894d7359 100644 --- a/arch/arc/mm/mmap.c +++ b/arch/arc/mm/mmap.c @@ -74,3 +74,44 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, info.align_offset = pgoff << PAGE_SHIFT; return vm_unmapped_area(); } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_U_NONE; + case VM_READ: + /* Pvt-W => !W */ + case VM_WRITE: + /* Pvt-W => !W */ + case VM_WRITE | VM_READ: + return PAGE_U_R; + /* X => R */ + case VM_EXEC: + case VM_EXEC | VM_READ: +/* Pvt-W => !W and X => R */ + case VM_EXEC | VM_WRITE: +/* Pvt-W => !W */ + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_U_X_R; + case VM_SHARED: + return PAGE_U_NONE; + case VM_SHARED | VM_READ: + return PAGE_U_R; + /* W => R */ + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_U_W_R; +/* X => R */ + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_U_X_R; + /* X => R */ + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_U_X_W_R; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 17/30] sh/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Yoshinori Sato Cc: Rich Felker Cc: linux...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/sh/Kconfig | 1 + arch/sh/include/asm/pgtable.h | 17 arch/sh/mm/mmap.c | 38 +++ 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig index 2474a04ceac4..f3fcd1c5e002 100644 --- a/arch/sh/Kconfig +++ b/arch/sh/Kconfig @@ -11,6 +11,7 @@ config SUPERH select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_PTE_SPECIAL select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HIBERNATION_POSSIBLE if MMU select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_WANT_IPC_PARSE_VERSION diff --git a/arch/sh/include/asm/pgtable.h b/arch/sh/include/asm/pgtable.h index d7ddb1ec86a0..6fb9ec54cf9b 100644 --- a/arch/sh/include/asm/pgtable.h +++ b/arch/sh/include/asm/pgtable.h @@ -89,23 +89,6 @@ static inline unsigned long phys_addr_mask(void) * completely separate permission bits for user and kernel space. */ /*xwr*/ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY -#define __P100 PAGE_EXECREAD -#define __P101 PAGE_EXECREAD -#define __P110 PAGE_COPY -#define __P111 PAGE_COPY - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_WRITEONLY -#define __S011 PAGE_SHARED -#define __S100 PAGE_EXECREAD -#define __S101 PAGE_EXECREAD -#define __S110 PAGE_RWX -#define __S111 PAGE_RWX typedef pte_t *pte_addr_t; diff --git a/arch/sh/mm/mmap.c b/arch/sh/mm/mmap.c index 6a1a1297baae..cad14af6c8e6 100644 --- a/arch/sh/mm/mmap.c +++ b/arch/sh/mm/mmap.c @@ -162,3 +162,41 @@ int valid_mmap_phys_addr_range(unsigned long pfn, size_t size) { return 1; } + +#ifdef CONFIG_MMU +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY; + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_EXEC: + case VM_EXEC | VM_READ: + return PAGE_EXECREAD; + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_WRITE: + return PAGE_WRITEONLY; + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_EXECREAD; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_RWX; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); +#endif -- 2.25.1
[PATCH V3 16/30] alpha/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Richard Henderson Cc: linux-al...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/alpha/Kconfig | 1 + arch/alpha/include/asm/pgtable.h | 17 --- arch/alpha/mm/init.c | 37 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index 4e87783c90ad..73e82fe5c770 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -2,6 +2,7 @@ config ALPHA bool default y + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_32BIT_USTAT_F_TINODE select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO diff --git a/arch/alpha/include/asm/pgtable.h b/arch/alpha/include/asm/pgtable.h index 02f0429f1068..9fb5e9d10bb6 100644 --- a/arch/alpha/include/asm/pgtable.h +++ b/arch/alpha/include/asm/pgtable.h @@ -116,23 +116,6 @@ struct vm_area_struct; * arch/alpha/mm/fault.c) */ /* xwr */ -#define __P000 _PAGE_P(_PAGE_FOE | _PAGE_FOW | _PAGE_FOR) -#define __P001 _PAGE_P(_PAGE_FOE | _PAGE_FOW) -#define __P010 _PAGE_P(_PAGE_FOE) -#define __P011 _PAGE_P(_PAGE_FOE) -#define __P100 _PAGE_P(_PAGE_FOW | _PAGE_FOR) -#define __P101 _PAGE_P(_PAGE_FOW) -#define __P110 _PAGE_P(0) -#define __P111 _PAGE_P(0) - -#define __S000 _PAGE_S(_PAGE_FOE | _PAGE_FOW | _PAGE_FOR) -#define __S001 _PAGE_S(_PAGE_FOE | _PAGE_FOW) -#define __S010 _PAGE_S(_PAGE_FOE) -#define __S011 _PAGE_S(_PAGE_FOE) -#define __S100 _PAGE_S(_PAGE_FOW | _PAGE_FOR) -#define __S101 _PAGE_S(_PAGE_FOW) -#define __S110 _PAGE_S(0) -#define __S111 _PAGE_S(0) /* * pgprot_noncached() is only for infiniband pci support, and a real diff --git a/arch/alpha/mm/init.c b/arch/alpha/mm/init.c index f6114d03357c..2e78008b2553 100644 --- a/arch/alpha/mm/init.c +++ b/arch/alpha/mm/init.c @@ -280,3 +280,40 @@ mem_init(void) high_memory = (void *) __va(max_low_pfn * PAGE_SIZE); memblock_free_all(); } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return _PAGE_P(_PAGE_FOE | _PAGE_FOW | _PAGE_FOR); + case VM_READ: + return _PAGE_P(_PAGE_FOE | _PAGE_FOW); + case VM_WRITE: + case VM_WRITE | VM_READ: + return _PAGE_P(_PAGE_FOE); + case VM_EXEC: + return _PAGE_P(_PAGE_FOW | _PAGE_FOR); + case VM_EXEC | VM_READ: + return _PAGE_P(_PAGE_FOW); + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return _PAGE_P(0); + case VM_SHARED: + return _PAGE_S(_PAGE_FOE | _PAGE_FOW | _PAGE_FOR); + case VM_SHARED | VM_READ: + return _PAGE_S(_PAGE_FOE | _PAGE_FOW); + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return _PAGE_S(_PAGE_FOE); + case VM_SHARED | VM_EXEC: + return _PAGE_S(_PAGE_FOW | _PAGE_FOR); + case VM_SHARED | VM_EXEC | VM_READ: + return _PAGE_S(_PAGE_FOW); + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return _PAGE_S(0); + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 15/30] riscv/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Paul Walmsley Cc: Palmer Dabbelt Cc: linux-ri...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/riscv/Kconfig | 1 + arch/riscv/include/asm/pgtable.h | 16 arch/riscv/mm/init.c | 42 3 files changed, 43 insertions(+), 16 deletions(-) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 5adcbd9b5e88..9391742f9286 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -31,6 +31,7 @@ config RISCV select ARCH_HAS_STRICT_MODULE_RWX if MMU && !XIP_KERNEL select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_HAS_UBSAN_SANITIZE_ALL + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT select ARCH_STACKWALK diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h index 7e949f25c933..d2bb14cac28b 100644 --- a/arch/riscv/include/asm/pgtable.h +++ b/arch/riscv/include/asm/pgtable.h @@ -183,24 +183,8 @@ extern struct pt_alloc_ops pt_ops __initdata; extern pgd_t swapper_pg_dir[]; /* MAP_PRIVATE permissions: xwr (copy-on-write) */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READ -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY -#define __P100 PAGE_EXEC -#define __P101 PAGE_READ_EXEC -#define __P110 PAGE_COPY_EXEC -#define __P111 PAGE_COPY_READ_EXEC /* MAP_SHARED permissions: xwr */ -#define __S000 PAGE_NONE -#define __S001 PAGE_READ -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_EXEC -#define __S101 PAGE_READ_EXEC -#define __S110 PAGE_SHARED_EXEC -#define __S111 PAGE_SHARED_EXEC #ifdef CONFIG_TRANSPARENT_HUGEPAGE static inline int pmd_present(pmd_t pmd) diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index c27294128e18..8cb5d1eeb287 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -1050,3 +1050,45 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, return vmemmap_populate_basepages(start, end, node, NULL); } #endif + +#ifdef CONFIG_MMU +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + /* MAP_PRIVATE permissions: xwr (copy-on-write) */ + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READ; + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_EXEC: + return PAGE_EXEC; + case VM_EXEC | VM_READ: + return PAGE_READ_EXEC; + case VM_EXEC | VM_WRITE: + return PAGE_COPY_EXEC; + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_COPY_READ_EXEC; + /* MAP_SHARED permissions: xwr */ + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READ; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + return PAGE_EXEC; + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READ_EXEC; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED_EXEC; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); +#endif /* CONFIG_MMU */ -- 2.25.1
[PATCH V3 14/30] s390/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Heiko Carstens Cc: Vasily Gorbik Cc: linux-s...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Acked-by: Sven Schnelle Acked-by: Alexander Gordeev Signed-off-by: Anshuman Khandual --- arch/s390/Kconfig | 1 + arch/s390/include/asm/pgtable.h | 17 - arch/s390/mm/mmap.c | 33 + 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index be9f39fd06df..cb1b487e8201 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -78,6 +78,7 @@ config S390 select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_UBSAN_SANITIZE_ALL select ARCH_HAS_VDSO_DATA + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAVE_NMI_SAFE_CMPXCHG select ARCH_INLINE_READ_LOCK select ARCH_INLINE_READ_LOCK_BH diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 008a6c856fa4..3893ef64b439 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -422,23 +422,6 @@ static inline int is_module_addr(void *addr) * implies read permission. */ /*xwr*/ -#define __P000 PAGE_NONE -#define __P001 PAGE_RO -#define __P010 PAGE_RO -#define __P011 PAGE_RO -#define __P100 PAGE_RX -#define __P101 PAGE_RX -#define __P110 PAGE_RX -#define __P111 PAGE_RX - -#define __S000 PAGE_NONE -#define __S001 PAGE_RO -#define __S010 PAGE_RW -#define __S011 PAGE_RW -#define __S100 PAGE_RX -#define __S101 PAGE_RX -#define __S110 PAGE_RWX -#define __S111 PAGE_RWX /* * Segment entry (large page) protection definitions. diff --git a/arch/s390/mm/mmap.c b/arch/s390/mm/mmap.c index e54f928503c5..e99c198aa5de 100644 --- a/arch/s390/mm/mmap.c +++ b/arch/s390/mm/mmap.c @@ -188,3 +188,36 @@ void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack) mm->get_unmapped_area = arch_get_unmapped_area_topdown; } } + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_RO; + case VM_EXEC: + case VM_EXEC | VM_READ: + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_RX; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_RO; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_RW; + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_RX; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_RWX; + default: + BUILD_BUG(); + } +} +EXPORT_SYMBOL(vm_get_page_prot); -- 2.25.1
[PATCH V3 13/30] mm/mmap: Drop arch_vm_get_page_pgprot()
There are no platforms left which use arch_vm_get_page_prot(). Just drop arch_vm_get_page_prot() construct and simplify remaining code. Cc: Andrew Morton Cc: linux...@kvack.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- include/linux/mman.h | 4 mm/mmap.c| 10 +- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/include/linux/mman.h b/include/linux/mman.h index b66e91b8176c..58b3abd457a3 100644 --- a/include/linux/mman.h +++ b/include/linux/mman.h @@ -93,10 +93,6 @@ static inline void vm_unacct_memory(long pages) #define arch_calc_vm_flag_bits(flags) 0 #endif -#ifndef arch_vm_get_page_prot -#define arch_vm_get_page_prot(vm_flags) __pgprot(0) -#endif - #ifndef arch_validate_prot /* * This is called from mprotect(). PROT_GROWSDOWN and PROT_GROWSUP have diff --git a/mm/mmap.c b/mm/mmap.c index c8fd8f06bf7c..c13dd9c37866 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -102,7 +102,7 @@ static void unmap_region(struct mm_struct *mm, * w: (no) no * x: (yes) yes */ -static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) +pgprot_t vm_get_page_prot(unsigned long vm_flags) { switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { case VM_NONE: @@ -141,14 +141,6 @@ static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) BUILD_BUG(); } } - -pgprot_t vm_get_page_prot(unsigned long vm_flags) -{ - pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) | - pgprot_val(arch_vm_get_page_prot(vm_flags))); - - return ret; -} EXPORT_SYMBOL(vm_get_page_prot); #endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */ -- 2.25.1
[PATCH V3 12/30] mm/mmap: Drop arch_filter_pgprot()
There are no platforms left which subscribe ARCH_HAS_FILTER_PGPROT. Hence just drop arch_filter_pgprot() and also the config ARCH_HAS_FILTER_PGPROT. Cc: Andrew Morton Cc: linux...@kvack.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- mm/Kconfig | 3 --- mm/mmap.c | 10 +- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/mm/Kconfig b/mm/Kconfig index fa436478a94c..212fb6e1ddaa 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -744,9 +744,6 @@ config IDLE_PAGE_TRACKING config ARCH_HAS_CACHE_LINE_SIZE bool -config ARCH_HAS_FILTER_PGPROT - bool - config ARCH_HAS_VM_GET_PAGE_PROT bool diff --git a/mm/mmap.c b/mm/mmap.c index 78eeac277a80..c8fd8f06bf7c 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -102,14 +102,6 @@ static void unmap_region(struct mm_struct *mm, * w: (no) no * x: (yes) yes */ - -#ifndef CONFIG_ARCH_HAS_FILTER_PGPROT -static inline pgprot_t arch_filter_pgprot(pgprot_t prot) -{ - return prot; -} -#endif - static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) { switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { @@ -155,7 +147,7 @@ pgprot_t vm_get_page_prot(unsigned long vm_flags) pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) | pgprot_val(arch_vm_get_page_prot(vm_flags))); - return arch_filter_pgprot(ret); + return ret; } EXPORT_SYMBOL(vm_get_page_prot); #endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */ -- 2.25.1
[PATCH V3 11/30] mm/mmap: Drop protection_map[]
There are no other users for protection_map[]. Hence just drop this array construct and instead define __vm_get_page_prot() which will provide page protection map based on vm_flags combination switch. Cc: Andrew Morton Cc: linux...@kvack.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- drivers/gpu/drm/drm_vm.c | 4 +-- include/linux/mm.h | 6 mm/mmap.c| 63 ++-- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index e957d4851dc0..14862df7532f 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -482,7 +482,7 @@ static int drm_mmap_dma(struct file *filp, struct vm_area_struct *vma) #else /* Ye gads this is ugly. With more thought we could move this up higher and use - `protection_map' instead. */ + `vm_get_page_prot()' instead. */ vma->vm_page_prot = __pgprot(pte_val (pte_wrprotect @@ -566,7 +566,7 @@ static int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma) #else /* Ye gads this is ugly. With more thought we could move this up higher and use - `protection_map' instead. */ + `vm_get_page_prot()' instead. */ vma->vm_page_prot = __pgprot(pte_val (pte_wrprotect diff --git a/include/linux/mm.h b/include/linux/mm.h index 213cc569b192..ff74bd2d7850 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -418,12 +418,6 @@ extern unsigned int kobjsize(const void *objp); #endif #define VM_FLAGS_CLEAR (ARCH_VM_PKEY_FLAGS | VM_ARCH_CLEAR) -/* - * mapping from the currently active vm_flags protection bits (the - * low four bits) to a page protection mask.. - */ -extern pgprot_t protection_map[16]; - /* * The default fault flags that should be used by most of the * arch-specific page fault handlers. diff --git a/mm/mmap.c b/mm/mmap.c index f2310f6e7466..78eeac277a80 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -102,24 +102,6 @@ static void unmap_region(struct mm_struct *mm, * w: (no) no * x: (yes) yes */ -pgprot_t protection_map[16] __ro_after_init = { - [VM_NONE] = __P000, - [VM_READ] = __P001, - [VM_WRITE] = __P010, - [VM_WRITE | VM_READ]= __P011, - [VM_EXEC] = __P100, - [VM_EXEC | VM_READ] = __P101, - [VM_EXEC | VM_WRITE]= __P110, - [VM_EXEC | VM_WRITE | VM_READ] = __P111, - [VM_SHARED] = __S000, - [VM_SHARED | VM_READ] = __S001, - [VM_SHARED | VM_WRITE] = __S010, - [VM_SHARED | VM_WRITE | VM_READ]= __S011, - [VM_SHARED | VM_EXEC] = __S100, - [VM_SHARED | VM_EXEC | VM_READ] = __S101, - [VM_SHARED | VM_EXEC | VM_WRITE]= __S110, - [VM_SHARED | VM_EXEC | VM_WRITE | VM_READ] = __S111 -}; #ifndef CONFIG_ARCH_HAS_FILTER_PGPROT static inline pgprot_t arch_filter_pgprot(pgprot_t prot) @@ -128,10 +110,49 @@ static inline pgprot_t arch_filter_pgprot(pgprot_t prot) } #endif +static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return __P000; + case VM_READ: + return __P001; + case VM_WRITE: + return __P010; + case VM_READ | VM_WRITE: + return __P011; + case VM_EXEC: + return __P100; + case VM_EXEC | VM_READ: + return __P101; + case VM_EXEC | VM_WRITE: + return __P110; + case VM_EXEC | VM_READ | VM_WRITE: + return __P111; + case VM_SHARED: + return __S000; + case VM_SHARED | VM_READ: + return __S001; + case VM_SHARED | VM_WRITE: + return __S010; + case VM_SHARED | VM_READ | VM_WRITE: + return __S011; + case VM_SHARED | VM_EXEC: + return __S100; + case VM_SHARED | VM_EXEC | VM_READ: + return __S101; + case VM_SHARED | VM_EXEC | VM_WRITE: + return __S110; + case VM_SHARED | VM_EXEC | VM_READ | VM_WRITE: + return __S111; + default: + BUILD_BUG(); + } +} + pgprot_t
[PATCH V3 10/30] x86/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
From: Christoph Hellwig This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. This also unsubscribes from ARCH_HAS_FILTER_PGPROT, after dropping off arch_filter_pgprot() and arch_vm_get_page_prot(). Cc: Thomas Gleixner Cc: Ingo Molnar Cc: linux-ker...@vger.kernel.org Signed-off-by: Christoph Hellwig Signed-off-by: Anshuman Khandual --- arch/x86/Kconfig | 2 +- arch/x86/include/asm/pgtable.h | 5 -- arch/x86/include/asm/pgtable_types.h | 19 arch/x86/include/uapi/asm/mman.h | 14 -- arch/x86/mm/Makefile | 2 +- arch/x86/mm/mem_encrypt_amd.c| 6 --- arch/x86/mm/pgprot.c | 71 7 files changed, 73 insertions(+), 46 deletions(-) create mode 100644 arch/x86/mm/pgprot.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index b1ce75d0ab0c..b2ea06c87708 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -75,7 +75,6 @@ config X86 select ARCH_HAS_EARLY_DEBUG if KGDB select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_FAST_MULTIPLIER - select ARCH_HAS_FILTER_PGPROT select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_KCOVif X86_64 @@ -94,6 +93,7 @@ config X86 select ARCH_HAS_SYNC_CORE_BEFORE_USERMODE select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_UBSAN_SANITIZE_ALL + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAS_DEBUG_WX select ARCH_HAS_ZONE_DMA_SET if EXPERT select ARCH_HAVE_NMI_SAFE_CMPXCHG diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 8a9432fb3802..985e1b823691 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -648,11 +648,6 @@ static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot) #define canon_pgprot(p) __pgprot(massage_pgprot(p)) -static inline pgprot_t arch_filter_pgprot(pgprot_t prot) -{ - return canon_pgprot(prot); -} - static inline int is_new_memtype_allowed(u64 paddr, unsigned long size, enum page_cache_mode pcm, enum page_cache_mode new_pcm) diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h index 40497a9020c6..1a9dd933088e 100644 --- a/arch/x86/include/asm/pgtable_types.h +++ b/arch/x86/include/asm/pgtable_types.h @@ -228,25 +228,6 @@ enum page_cache_mode { #endif /* __ASSEMBLY__ */ -/* xwr */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY -#define __P100 PAGE_READONLY_EXEC -#define __P101 PAGE_READONLY_EXEC -#define __P110 PAGE_COPY_EXEC -#define __P111 PAGE_COPY_EXEC - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_READONLY_EXEC -#define __S101 PAGE_READONLY_EXEC -#define __S110 PAGE_SHARED_EXEC -#define __S111 PAGE_SHARED_EXEC - /* * early identity mapping pte attrib macros. */ diff --git a/arch/x86/include/uapi/asm/mman.h b/arch/x86/include/uapi/asm/mman.h index d4a8d0424bfb..775dbd3aff73 100644 --- a/arch/x86/include/uapi/asm/mman.h +++ b/arch/x86/include/uapi/asm/mman.h @@ -5,20 +5,6 @@ #define MAP_32BIT 0x40/* only give out 32bit addresses */ #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS -/* - * Take the 4 protection key bits out of the vma->vm_flags - * value and turn them in to the bits that we can put in - * to a pte. - * - * Only override these if Protection Keys are available - * (which is only on 64-bit). - */ -#define arch_vm_get_page_prot(vm_flags)__pgprot( \ - ((vm_flags) & VM_PKEY_BIT0 ? _PAGE_PKEY_BIT0 : 0) | \ - ((vm_flags) & VM_PKEY_BIT1 ? _PAGE_PKEY_BIT1 : 0) | \ - ((vm_flags) & VM_PKEY_BIT2 ? _PAGE_PKEY_BIT2 : 0) | \ - ((vm_flags) & VM_PKEY_BIT3 ? _PAGE_PKEY_BIT3 : 0)) - #define arch_calc_vm_prot_bits(prot, key) (\ ((key) & 0x1 ? VM_PKEY_BIT0 : 0) | \ ((key) & 0x2 ? VM_PKEY_BIT1 : 0) | \ diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile index fe3d3061fc11..fb6b41a48ae5 100644 --- a/arch/x86/mm/Makefile +++ b/arch/x86/mm/Makefile @@ -20,7 +20,7 @@ CFLAGS_REMOVE_mem_encrypt_identity.o = -pg endif obj-y := init.o init_$(BITS).o fault.o ioremap.o extable.o mmap.o \ - pgtable.o physaddr.o setup_nx.o tlb.o cpu_entry_area.o maccess.o + pgtable.o physaddr.o setup_nx.o tlb.o cpu_entry_area.o maccess.o pgprot.o obj-y += pat/ diff --git a/arch/x86/mm/mem_encrypt_amd.c
[PATCH V3 09/30] arm/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Russell King Cc: Arnd Bergmann Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/arm/Kconfig | 1 + arch/arm/include/asm/pgtable.h | 20 +- arch/arm/lib/uaccess_with_memcpy.c | 2 +- arch/arm/mm/mmu.c | 44 ++ 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 4c97cb40eebb..87b2e89ef3d6 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -23,6 +23,7 @@ config ARM select ARCH_HAS_SYNC_DMA_FOR_CPU if SWIOTLB || !MMU select ARCH_HAS_TEARDOWN_DMA_OPS if MMU select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAVE_CUSTOM_GPIO_H select ARCH_HAVE_NMI_SAFE_CMPXCHG if CPU_V7 || CPU_V7M || CPU_V6K select ARCH_HAS_GCOV_PROFILE_ALL diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h index cd1f84bb40ae..64711716cd84 100644 --- a/arch/arm/include/asm/pgtable.h +++ b/arch/arm/include/asm/pgtable.h @@ -70,7 +70,7 @@ extern void __pgd_error(const char *file, int line, pgd_t); #endif /* - * The pgprot_* and protection_map entries will be fixed up in runtime + * The pgprot_* entries will be fixed up in runtime in vm_get_page_prot() * to include the cachable and bufferable bits based on memory policy, * as well as any architecture dependent bits like global/ASID and SMP * shared mapping bits. @@ -137,24 +137,6 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, * 2) If we could do execute protection, then read is implied * 3) write implies read permissions */ -#define __P000 __PAGE_NONE -#define __P001 __PAGE_READONLY -#define __P010 __PAGE_COPY -#define __P011 __PAGE_COPY -#define __P100 __PAGE_READONLY_EXEC -#define __P101 __PAGE_READONLY_EXEC -#define __P110 __PAGE_COPY_EXEC -#define __P111 __PAGE_COPY_EXEC - -#define __S000 __PAGE_NONE -#define __S001 __PAGE_READONLY -#define __S010 __PAGE_SHARED -#define __S011 __PAGE_SHARED -#define __S100 __PAGE_READONLY_EXEC -#define __S101 __PAGE_READONLY_EXEC -#define __S110 __PAGE_SHARED_EXEC -#define __S111 __PAGE_SHARED_EXEC - #ifndef __ASSEMBLY__ /* * ZERO_PAGE is a global shared page that is always zero: used diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c index 106f83a5ea6d..12d8d9794a28 100644 --- a/arch/arm/lib/uaccess_with_memcpy.c +++ b/arch/arm/lib/uaccess_with_memcpy.c @@ -247,7 +247,7 @@ static int __init test_size_treshold(void) if (!dst_page) goto no_dst; kernel_ptr = page_address(src_page); - user_ptr = vmap(_page, 1, VM_IOREMAP, __pgprot(__P010)); + user_ptr = vmap(_page, 1, VM_IOREMAP, __pgprot(__PAGE_COPY)); if (!user_ptr) goto no_vmap; diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 274e4f73fd33..9cdf45da57de 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -403,6 +403,8 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) local_flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); } +static pteval_t user_pgprot; + /* * Adjust the PMD section entries according to the CPU in use. */ @@ -410,7 +412,7 @@ static void __init build_mem_type_table(void) { struct cachepolicy *cp; unsigned int cr = get_cr(); - pteval_t user_pgprot, kern_pgprot, vecs_pgprot; + pteval_t kern_pgprot, vecs_pgprot; int cpu_arch = cpu_architecture(); int i; @@ -627,11 +629,6 @@ static void __init build_mem_type_table(void) user_pgprot |= PTE_EXT_PXN; #endif - for (i = 0; i < 16; i++) { - pteval_t v = pgprot_val(protection_map[i]); - protection_map[i] = __pgprot(v | user_pgprot); - } - mem_types[MT_LOW_VECTORS].prot_pte |= vecs_pgprot; mem_types[MT_HIGH_VECTORS].prot_pte |= vecs_pgprot; @@ -670,6 +667,41 @@ static void __init build_mem_type_table(void) } } +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return __pgprot(pgprot_val(__PAGE_NONE) | user_pgprot); + case VM_READ: + return __pgprot(pgprot_val(__PAGE_READONLY) | user_pgprot); + case VM_WRITE: + case VM_WRITE | VM_READ: + return __pgprot(pgprot_val(__PAGE_COPY) | user_pgprot); + case VM_EXEC: + case VM_EXEC | VM_READ: + return __pgprot(pgprot_val(__PAGE_READONLY_EXEC) | user_pgprot); + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ:
[PATCH V3 08/30] m68k/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Thomas Bogendoerfer Cc: linux-m...@lists.linux-m68k.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/m68k/Kconfig| 1 + arch/m68k/include/asm/mcf_pgtable.h | 59 arch/m68k/include/asm/motorola_pgtable.h | 29 arch/m68k/include/asm/sun3_pgtable.h | 22 - arch/m68k/mm/mcfmmu.c| 59 arch/m68k/mm/motorola.c | 43 +++-- arch/m68k/mm/sun3mmu.c | 39 7 files changed, 139 insertions(+), 113 deletions(-) diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig index 936e1803c7c7..114e65164692 100644 --- a/arch/m68k/Kconfig +++ b/arch/m68k/Kconfig @@ -11,6 +11,7 @@ config M68K select ARCH_NO_PREEMPT if !COLDFIRE select ARCH_USE_MEMTEST if MMU_MOTOROLA select ARCH_WANT_IPC_PARSE_VERSION + select ARCH_HAS_VM_GET_PAGE_PROT select BINFMT_FLAT_ARGVP_ENVP_ON_STACK select DMA_DIRECT_REMAP if HAS_DMA && MMU && !COLDFIRE select GENERIC_ATOMIC64 diff --git a/arch/m68k/include/asm/mcf_pgtable.h b/arch/m68k/include/asm/mcf_pgtable.h index 6f2b87d7a50d..dc5c8ab6aa57 100644 --- a/arch/m68k/include/asm/mcf_pgtable.h +++ b/arch/m68k/include/asm/mcf_pgtable.h @@ -86,65 +86,6 @@ | CF_PAGE_READABLE \ | CF_PAGE_DIRTY) -/* - * Page protections for initialising protection_map. See mm/mmap.c - * for use. In general, the bit positions are xwr, and P-items are - * private, the S-items are shared. - */ -#define __P000 PAGE_NONE -#define __P001 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE) -#define __P010 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_WRITABLE) -#define __P011 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE \ -| CF_PAGE_WRITABLE) -#define __P100 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_EXEC) -#define __P101 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE \ -| CF_PAGE_EXEC) -#define __P110 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_WRITABLE \ -| CF_PAGE_EXEC) -#define __P111 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE \ -| CF_PAGE_WRITABLE \ -| CF_PAGE_EXEC) - -#define __S000 PAGE_NONE -#define __S001 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE) -#define __S010 PAGE_SHARED -#define __S011 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_SHARED \ -| CF_PAGE_READABLE) -#define __S100 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_EXEC) -#define __S101 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_READABLE \ -| CF_PAGE_EXEC) -#define __S110 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_SHARED \ -| CF_PAGE_EXEC) -#define __S111 __pgprot(CF_PAGE_VALID \ -| CF_PAGE_ACCESSED \ -| CF_PAGE_SHARED \ -| CF_PAGE_READABLE \ -| CF_PAGE_EXEC) - #define PTE_MASK PAGE_MASK #define CF_PAGE_CHG_MASK (PTE_MASK | CF_PAGE_ACCESSED | CF_PAGE_DIRTY) diff --git a/arch/m68k/include/asm/motorola_pgtable.h b/arch/m68k/include/asm/motorola_pgtable.h index 022c3abc280d..dcbb856f567e 100644 --- a/arch/m68k/include/asm/motorola_pgtable.h +++ b/arch/m68k/include/asm/motorola_pgtable.h @@ -76,35 +76,6 @@ extern unsigned long mm_cachebits; #define PAGE_READONLY __pgprot(_PAGE_PRESENT | _PAGE_RONLY | _PAGE_ACCESSED | mm_cachebits) #define PAGE_KERNEL
[PATCH V3 07/30] mips/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. Cc: Thomas Bogendoerfer Cc: linux-m...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Acked-by: Thomas Bogendoerfer Signed-off-by: Anshuman Khandual --- arch/mips/Kconfig | 1 + arch/mips/include/asm/pgtable.h | 22 arch/mips/mm/cache.c| 60 +++-- 3 files changed, 36 insertions(+), 47 deletions(-) diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 058446f01487..fcbfc52a1567 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -13,6 +13,7 @@ config MIPS select ARCH_HAS_STRNLEN_USER select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_HAS_UBSAN_SANITIZE_ALL + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_KEEP_MEMBLOCK select ARCH_SUPPORTS_UPROBES diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h index 7b8037f25d9e..bf193ad4f195 100644 --- a/arch/mips/include/asm/pgtable.h +++ b/arch/mips/include/asm/pgtable.h @@ -41,28 +41,6 @@ struct vm_area_struct; * by reasonable means.. */ -/* - * Dummy values to fill the table in mmap.c - * The real values will be generated at runtime - */ -#define __P000 __pgprot(0) -#define __P001 __pgprot(0) -#define __P010 __pgprot(0) -#define __P011 __pgprot(0) -#define __P100 __pgprot(0) -#define __P101 __pgprot(0) -#define __P110 __pgprot(0) -#define __P111 __pgprot(0) - -#define __S000 __pgprot(0) -#define __S001 __pgprot(0) -#define __S010 __pgprot(0) -#define __S011 __pgprot(0) -#define __S100 __pgprot(0) -#define __S101 __pgprot(0) -#define __S110 __pgprot(0) -#define __S111 __pgprot(0) - extern unsigned long _page_cachable_default; extern void __update_cache(unsigned long address, pte_t pte); diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c index 830ab91e574f..9f33ce4fb105 100644 --- a/arch/mips/mm/cache.c +++ b/arch/mips/mm/cache.c @@ -159,30 +159,6 @@ EXPORT_SYMBOL(_page_cachable_default); #define PM(p) __pgprot(_page_cachable_default | (p)) -static inline void setup_protection_map(void) -{ - protection_map[0] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); - protection_map[1] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC); - protection_map[2] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); - protection_map[3] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC); - protection_map[4] = PM(_PAGE_PRESENT); - protection_map[5] = PM(_PAGE_PRESENT); - protection_map[6] = PM(_PAGE_PRESENT); - protection_map[7] = PM(_PAGE_PRESENT); - - protection_map[8] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); - protection_map[9] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC); - protection_map[10] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | - _PAGE_NO_READ); - protection_map[11] = PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE); - protection_map[12] = PM(_PAGE_PRESENT); - protection_map[13] = PM(_PAGE_PRESENT); - protection_map[14] = PM(_PAGE_PRESENT | _PAGE_WRITE); - protection_map[15] = PM(_PAGE_PRESENT | _PAGE_WRITE); -} - -#undef PM - void cpu_cache_init(void) { if (cpu_has_3k_cache) { @@ -206,6 +182,40 @@ void cpu_cache_init(void) octeon_cache_init(); } +} - setup_protection_map(); +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); + case VM_READ: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC); + case VM_WRITE: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); + case VM_WRITE | VM_READ: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC); + case VM_EXEC: + case VM_EXEC | VM_READ: + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PM(_PAGE_PRESENT); + case VM_SHARED: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ); + case VM_SHARED | VM_READ: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC); + case VM_SHARED | VM_WRITE: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ); + case VM_SHARED | VM_WRITE | VM_READ: + return PM(_PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE); + case VM_SHARED | VM_EXEC: + case VM_SHARED | VM_EXEC | VM_READ: + return PM(_PAGE_PRESENT); + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PM(_PAGE_PRESENT | _PAGE_WRITE); + default: + BUILD_BUG(); + } }
[PATCH V3 06/30] sparc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. This also localizes the helper 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 Acked-by: David S. Miller Signed-off-by: Anshuman Khandual --- arch/sparc/Kconfig | 2 + arch/sparc/include/asm/mman.h | 6 --- arch/sparc/include/asm/pgtable_32.h | 19 arch/sparc/include/asm/pgtable_64.h | 19 arch/sparc/mm/init_32.c | 35 +++ arch/sparc/mm/init_64.c | 70 + 6 files changed, 88 insertions(+), 63 deletions(-) diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig index 1cab1b284f1a..ff29156f2380 100644 --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -59,6 +59,7 @@ config SPARC32 select HAVE_UID16 select OLD_SIGACTION select ZONE_DMA + select ARCH_HAS_VM_GET_PAGE_PROT config SPARC64 def_bool 64BIT @@ -84,6 +85,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/include/asm/pgtable_32.h b/arch/sparc/include/asm/pgtable_32.h index ffccfe3b22ed..060a435f96d6 100644 --- a/arch/sparc/include/asm/pgtable_32.h +++ b/arch/sparc/include/asm/pgtable_32.h @@ -64,25 +64,6 @@ void paging_init(void); extern unsigned long ptr_in_current_pgd; -/* xwr */ -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_COPY -#define __P011 PAGE_COPY -#define __P100 PAGE_READONLY -#define __P101 PAGE_READONLY -#define __P110 PAGE_COPY -#define __P111 PAGE_COPY - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_READONLY -#define __S101 PAGE_READONLY -#define __S110 PAGE_SHARED -#define __S111 PAGE_SHARED - /* First physical page can be anywhere, the following is needed so that * va-->pa and vice versa conversions work properly without performance * hit for all __pa()/__va() operations. diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h index 4679e45c8348..a779418ceba9 100644 --- a/arch/sparc/include/asm/pgtable_64.h +++ b/arch/sparc/include/asm/pgtable_64.h @@ -187,25 +187,6 @@ bool kern_addr_valid(unsigned long addr); #define _PAGE_SZHUGE_4U_PAGE_SZ4MB_4U #define _PAGE_SZHUGE_4V_PAGE_SZ4MB_4V -/* These are actually filled in at boot time by sun4{u,v}_pgprot_init() */ -#define __P000 __pgprot(0) -#define __P001 __pgprot(0) -#define __P010 __pgprot(0) -#define __P011 __pgprot(0) -#define __P100 __pgprot(0) -#define __P101 __pgprot(0) -#define __P110 __pgprot(0) -#define __P111 __pgprot(0) - -#define __S000 __pgprot(0) -#define __S001 __pgprot(0) -#define __S010 __pgprot(0) -#define __S011 __pgprot(0) -#define __S100 __pgprot(0) -#define __S101 __pgprot(0) -#define __S110 __pgprot(0) -#define __S111 __pgprot(0) - #ifndef __ASSEMBLY__ pte_t mk_pte_io(unsigned long, pgprot_t, int, unsigned long); diff --git a/arch/sparc/mm/init_32.c b/arch/sparc/mm/init_32.c index 1e9f577f084d..348cbfe08b60 100644 --- a/arch/sparc/mm/init_32.c +++ b/arch/sparc/mm/init_32.c @@ -302,3 +302,38 @@ void sparc_flush_page_to_ram(struct page *page) __flush_page_to_ram(vaddr); } EXPORT_SYMBOL(sparc_flush_page_to_ram); + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + return PAGE_READONLY; + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_COPY; + case VM_EXEC: + case VM_EXEC | VM_READ: + return PAGE_READONLY; + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: +
[PATCH V3 05/30] arm64/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. This also localizes both arch_filter_pgprot and arch_vm_get_page_prot() helpers, unsubscribing from ARCH_HAS_FILTER_PGPROT as well. Moved both these localized functions near vm_get_page_prot(). Cc: Catalin Marinas Cc: Will Deacon Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Anshuman Khandual --- arch/arm64/Kconfig| 2 +- arch/arm64/include/asm/memory.h | 3 +- arch/arm64/include/asm/mman.h | 24 - arch/arm64/include/asm/pgtable-prot.h | 18 --- arch/arm64/include/asm/pgtable.h | 11 arch/arm64/mm/mmap.c | 78 +++ 6 files changed, 80 insertions(+), 56 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 2e5d2eac6fc6..7153d5fff603 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -23,7 +23,6 @@ config ARM64 select ARCH_HAS_DMA_PREP_COHERENT select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI select ARCH_HAS_FAST_MULTIPLIER - select ARCH_HAS_FILTER_PGPROT select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_GIGANTIC_PAGE @@ -44,6 +43,7 @@ config ARM64 select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_TEARDOWN_DMA_OPS if IOMMU_SUPPORT select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAS_ZONE_DMA_SET if EXPERT select ARCH_HAVE_ELF_PROT select ARCH_HAVE_NMI_SAFE_CMPXCHG diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index 0af70d9abede..64a613a0349b 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -126,8 +126,7 @@ * Memory types available. * * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in - * the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note - * that protection_map[] only contains MT_NORMAL attributes. + * the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. */ #define MT_NORMAL 0 #define MT_NORMAL_TAGGED 1 diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h index e3e28f7daf62..5966ee4a6154 100644 --- a/arch/arm64/include/asm/mman.h +++ b/arch/arm64/include/asm/mman.h @@ -35,30 +35,6 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags) } #define arch_calc_vm_flag_bits(flags) arch_calc_vm_flag_bits(flags) -static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags) -{ - pteval_t prot = 0; - - if (vm_flags & VM_ARM64_BTI) - prot |= PTE_GP; - - /* -* There are two conditions required for returning a Normal Tagged -* memory type: (1) the user requested it via PROT_MTE passed to -* mmap() or mprotect() and (2) the corresponding vma supports MTE. We -* register (1) as VM_MTE in the vma->vm_flags and (2) as -* VM_MTE_ALLOWED. Note that the latter can only be set during the -* mmap() call since mprotect() does not accept MAP_* flags. -* Checking for VM_MTE only is sufficient since arch_validate_flags() -* does not permit (VM_MTE & !VM_MTE_ALLOWED). -*/ - if (vm_flags & VM_MTE) - prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); - - return __pgprot(prot); -} -#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) { diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h index 7032f04c8ac6..d8ee0aa7886d 100644 --- a/arch/arm64/include/asm/pgtable-prot.h +++ b/arch/arm64/include/asm/pgtable-prot.h @@ -88,24 +88,6 @@ extern bool arm64_use_ng_mappings; #define PAGE_READONLY_EXEC __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN) #define PAGE_EXECONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN) -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_READONLY -#define __P011 PAGE_READONLY -#define __P100 PAGE_EXECONLY -#define __P101 PAGE_READONLY_EXEC -#define __P110 PAGE_READONLY_EXEC -#define __P111 PAGE_READONLY_EXEC - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_EXECONLY -#define __S101 PAGE_READONLY_EXEC -#define __S110 PAGE_SHARED_EXEC -#define __S111 PAGE_SHARED_EXEC - #endif /* __ASSEMBLY__ */ #endif /* __ASM_PGTABLE_PROT_H */ diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index c4ba047a82d2..94e147e5456c 100644 --- a/arch/arm64/include/asm/pgtable.h +++