Re: [arc] get_user() not clearing destination on failure
On Thu, Aug 18, 2016 at 11:11:09PM -0700, Vineet Gupta wrote: > On 08/17/2016 08:00 PM, Al Viro wrote: > Just curious - why is that. The typical usage paradigm is check for return > value > and if 0 only then proceed to use the actual value. > > Also for discussion sake, will eliminating the intermediate __x be helpful - > so it > would use the actual variable defined in caller of get_user() so if it is init > properly, the value would be "retained" for -EFAULT. Anyways all of this is > just > for understanding better. The effect of __get_user(v, p) should mirror that of v = *p had p been a kernel pointer. Including the sign expansion/truncation/etc. That's what those casts and assignments are about, and unless you want to play with doing usual arithmetical conversion in your assembler bits, it has to stay in C. Sure, we could have __get_user(v, p) use v instead of (in your case) __x and do v = (__typeof(*p))v; to trigger the conversions, but... that's where the second problem bites: __get_user() has to be a macro, and __get_user(a[i++], p) would obviously break with that approach. So there's no realistic way to get rid of that intermediate - not without a massive PITA with folding conversions into asm part and playing with __builtin_choose_expr/__builtin_types_compatible_p, which is going to be both painful and very easy to get wrong. As for checking for error and discarding the results if it has happened... not always possible or reasonable. If you are using __get_user() in the first place, you are likely to be on a fairly hot path (otherwise the normal get_user() would've worked just as well). And having explicit test and branch after each of those can be unpleasant enough. Which means that you do something like a series of err |= __get_user(...); and check err once in the end (or have something like __get_user_err(..., err) or __get_user_ex() as x86 does, etc.). Which means that by the time you get to "OK, we sod off now" path, you don't know which of __get_user() has failed. If any of the destinations are not immediately destroyed, you get a fun questions along the lines of "which ones do I clean?", and even simpler "how do I make sure that none of them is missed by an accident?" The same goes for get_user(), except that instead of "we use it on hot paths" we have "it's used by arseloads of ioctls in arseloads of drivers, with general code quality being what one would expect"... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RFC 2/4] lib/strncpy_from_user: Remove redundant user space pointer range check
On Tue, Jan 14, 2020 at 01:22:07PM -0800, Linus Torvalds wrote:
> The fact is, copying a string from user space is *very* different from
> copying a fixed number of bytes, and that whole dance with
>
> max_addr = user_addr_max();
>
> is absolutely required and necessary.
>
> You completely broke string copying.
BTW, a quick grep through the callers has found something odd -
static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
size_t size, loff_t *ppos)
{
char buf[64];
int buf_size;
int ret;
buf_size = min(size, (sizeof(buf) - 1));
if (strncpy_from_user(buf, user_buf, buf_size) < 0)
return -EFAULT;
buf[buf_size] = 0;
What the hell? If somebody is calling write(fd, buf, n) they'd
better be ready to see any byte from buf[0] up to buf[n - 1]
fetched, and if something is unmapped - deal with -EFAULT.
Is something really doing that and if so, why does kmemleak
try to accomodate that idiocy?
The same goes for several more ->write() instances - mtrr_write(),
armada_debugfs_crtc_reg_write() and cio_ignore_write(); IMO that's
seriously misguided (and cio one ought use vmemdup_user() instead
of what it's doing)...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RFC 1/4] asm-generic/uaccess: don't define inline functions if noinline lib/* in use
On Wed, Jan 15, 2020 at 10:08:31AM +0100, Arnd Bergmann wrote: > > I would suggest that anybody who uses asm-generic/uaccess.h needs to > > simply use the generic library version. > > Or possibly just everybody altogether: the remaining architectures that > have a custom implementation don't seem to be doing any better either. No go for s390. There you really want to access userland memory in larger chunks - it's oriented for block transfers. IIRC, the insn they are using has a costly setup phase, independent of the amount to copy, followed by reasonably fast transfer more or less linear by the size. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 05/10] arch/kmap_atomic: Consolidate duplicate code
On Thu, Apr 30, 2020 at 01:38:40PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > Every arch has the same code to ensure atomic operations and a check for > !HIGHMEM page. > > Remove the duplicate code by defining a core kmap_atomic() which only > calls the arch specific kmap_atomic_high() when the page is high memory. Err AFAICS, you've just silently changed the semantics for kmap_atomic_prot() here. And while most of the callers are converted, drivers/gpu/drm/ttm/ttm_bo_util.c one is not, so at the very least it's a bisect hazard... And I would argue that having kmap_atomic() differ from kmap_atomic_prot() wrt disabling preempt is asking for trouble. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 08/10] arch/kmap: Don't hard code kmap_prot values
On Thu, Apr 30, 2020 at 01:38:43PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > To support kmap_atomic_prot() on all architectures each arch must > support protections passed in to them. > > Change csky, mips, nds32 and xtensa to use their global kmap_prot value > rather than a hard coded value which was equal. Minor nitpick: it's probably worth pointing out that kmap_prot on those is a constant... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 09/10] arch/kmap: Define kmap_atomic_prot() for all arch's
On Thu, Apr 30, 2020 at 01:38:44PM -0700, ira.we...@intel.com wrote:
> -static inline void *kmap_atomic(struct page *page)
> +static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
> {
> preempt_disable();
> pagefault_disable();
> if (!PageHighMem(page))
> return page_address(page);
> - return kmap_atomic_high(page);
> + return kmap_atomic_high_prot(page, prot);
> }
> +#define kmap_atomic(page)kmap_atomic_prot(page, kmap_prot)
OK, so it *was* just a bisect hazard - you return to original semantics
wrt preempt_disable()...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V1 09/10] arch/kmap: Define kmap_atomic_prot() for all arch's
On Fri, May 01, 2020 at 03:37:34AM +0100, Al Viro wrote:
> On Thu, Apr 30, 2020 at 01:38:44PM -0700, ira.we...@intel.com wrote:
>
> > -static inline void *kmap_atomic(struct page *page)
> > +static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
> > {
> > preempt_disable();
> > pagefault_disable();
> > if (!PageHighMem(page))
> > return page_address(page);
> > - return kmap_atomic_high(page);
> > + return kmap_atomic_high_prot(page, prot);
> > }
> > +#define kmap_atomic(page) kmap_atomic_prot(page, kmap_prot)
>
> OK, so it *was* just a bisect hazard - you return to original semantics
> wrt preempt_disable()...
FWIW, how about doing the following: just before #5/10 have a patch
that would touch only microblaze, ppc and x86 splitting their
kmap_atomic_prot() into an inline helper + kmap_atomic_high_prot().
Then your #5 would leave their kmap_atomic_prot() as-is (it would
use kmap_atomic_prot_high() instead). The rest of the series plays
out pretty much the same way it does now, and wrappers on those
3 architectures would go away when an identical generic one is
introduced in this commit (#9/10).
AFAICS, that would avoid the bisect hazard and might even end
up with less noise in the patches...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V2 00/11] Subject: Remove duplicated kmap code
On Sun, May 03, 2020 at 06:09:01PM -0700, ira.we...@intel.com wrote: > From: Ira Weiny > > The kmap infrastructure has been copied almost verbatim to every architecture. > This series consolidates obvious duplicated code by defining core functions > which call into the architectures only when needed. > > Some of the k[un]map_atomic() implementations have some similarities but the > similarities were not sufficient to warrant further changes. > > In addition we remove a duplicate implementation of kmap() in DRM. > > Testing was done by 0day to cover all the architectures I can't readily > build/test. OK... Looking through my old notes on kmap unification (this winter, never went anywhere), * arch/mips/mm/cache.c ought to use linux/highmem.h, not asm/highmem.h I suspect that your series doesn't build on some configs there. Hadn't verified that, though. * kmap_atomic_to_page() is dead, but not quite gone - csky and nds32 brought the damn thing back (nds32 - only an extern). It needs killin'... * parisc is (arguably) abusing kunmap()/kunmap_atomic() for cache flushing. Replace the bulk of its highmem.h with #define ARCH_HAS_FLUSH_ON_KUNMAP #define arch_before_kunmap flush_kernel_dcache_page_addr and have default kunmap()/kunmap_atomic() do #ifdef ARCH_HAS_FLUSH_ON_KUNMAP arch_before_kunmap(page_address(page)); #endif and #ifdef ARCH_HAS_FLUSH_ON_KUNMAP arch_before_kunmap(addr); #endif resp. Kills ARCH_HAS_KMAP along with ifdefs on it, makes parisc use somewhat less hacky. I'd suggest checking various configs on mips - that's likely to cause headache. Said that, my analysis of include chains back then is pretty much worthless by now - I really hate the amount of indirect include chains leading to that sucker on some, but not all configs ;-/ IIRC, the proof that everything using kmap*/kunmap* would pull linux/highmem.h regardless of config took several hours of digging, ran for several pages and had been hopelessly brittle. arch/mips/mm/cache.c was the only exception caught by it, but these days there might be more. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH V2 00/11] Subject: Remove duplicated kmap code
On Sun, May 03, 2020 at 10:04:47PM -0700, Ira Weiny wrote:
> Grepping for 'asm/highmem.h' and investigations don't reveal any issues...
> But
> you do have me worried. That said 0-day has been crunching on multiple
> versions of this series without issues such as this (save the mips issue
> above).
>
> I have to say it would be nice if the relation between linux/highmem.h and
> asm/highmem.h was more straightforward.
IIRC, the headache was due to asm/pgtable.h on several architectures and
asm/cacheflush.h on parisc.
|| IOW, there's one in linux/highmem.h (conditional on !CONFIG_HIGHMEM,
|| !ARCH_HAS_KMAP) and several per-architecture variants, usually declared in
|| their asm/highmem.h. In three of those (microblaze, parisc and powerpc)
these
|| are inlines (parisc one identical to linux/highmem.h, lives in
asm/cacheflush.h,
|| powerpc and microblaze ones calling kmap_atomic_prot() which is defined in
|| arch/$ARCH/mm/highmem.c).
||
|| parisc case is weird - essentially, they want to call
|| flush_kernel_dcache_page_addr() at the places where kunmap/kunmap_atomic
|| is done. And they do so despite not selecting HIGHMEM, with definitions
|| in usual place. They do have ARCH_HAS_KMAP defined, which prevents
|| getting buggered in linux/highmem.h. ARCH_HAS_KMAP is parisc-unique,
|| BTW, and checked only in linux/highmem.h.
||
|| All genuine arch-specific variants are defined in (or call functions
|| defined in) translation units that are only included CONFIG_HIGHMEM builds.
||
|| It would be tempting to consolidate those, e.g. by adding
__kmap_atomic()
|| and __kmap_atomic_prot() without that boilerplate, with universal
kmap_atomic()
|| and kmap_atomic_prot() in linux/highmem.h. Potential problem with that would
|| be something that pulls ash/highmem.h (or asm/cacheflush.h in case of parisc)
|| directly and uses kmap_atomic/kmap_atomic_prot. There's not a lot places
|| pulling asm/highmem.h, and many of them are not even in includes:
||
|| arch/arm/include/asm/efi.h:13:#include
|| arch/arm/mm/dma-mapping.c:31:#include
|| arch/arm/mm/flush.c:14:#include
|| arch/arm/mm/mmu.c:27:#include
|| arch/mips/include/asm/pgtable-32.h:22:#include
|| arch/mips/mm/cache.c:19:#include
|| arch/mips/mm/fault.c:28:#include /* For
VMALLOC_END */
|| arch/nds32/include/asm/pgtable.h:60:#include
|| arch/x86/kernel/setup_percpu.c:20:#include
|| include/linux/highmem.h:35:#include
||
|| Users of asm/cacheflush.h are rather more numerous; however, anything
|| outside of parisc-specific code has to pull linux/highmem.h, or it won't see
|| the definitions of kmap_atomic/kmap_atomic_prot at all. arch/parisc itself
|| has no callers of those.
||
|| Outside of arch/* there is a plenty of callers. However, the following is
|| true: all instances of kmap_atomic or kmap_atomic_prot outside of arch/*
|| are either inside the linux/highmem.h or are preceded by include of
|| linux/highmem.h on any build that sees them (there is a common include
|| chain that is conditional upon CONFIG_BLOCK, but it's only needed in
|| drivers that are BLOCK-dependent). It was not fun to verify, to put
|| it mildly...
||
|| So for parisc we have no problem - leaving
__kmap_atomic()/__kmap_atomic_prot()
|| in asm/cachefile.h and adding universal wrappers in linux/highmem.h will be
|| fine. For other architectures the things might be trickier.
||
|| * arc: all users in arch/arc/ are within arch/arc/mm/{cache,highmem}.c;
|| both pull linux/highmem.h. We are fine.
||
|| * arm: much, much worse. We have several files that pull linux/highmem.h:
|| arch/arm/mm/cache-feroceon-l2.c, arch/arm/mm/cache-xsc3l2.c,
|| arch/arm/mm/copypage-*.c, arch/arm/mm/dma-mapping.c, arch/arm/mm/flush.c,
|| arch/arm/mm/highmem.c, arch/arm/probes/uprobes/core.c,
|| arch/arm/include/asm/kvm_mmu.h (kmap_atomic_pfn()).
|| Those are fine, but we also have this:
|| arch/arm/include/asm/pgtable.h:200:#define __pte_map(pmd)
(pte_t *)kmap_atomic(pmd_page(*(pmd)))
|| arch/arm/include/asm/pgtable.h:208:#define pte_offset_map(pmd,addr)
(__pte_map(pmd) + pte_index(addr))
|| and sure as hell, asm/pgtable.h does *NOT* pull linux/highmem.h.
||
|| Fortunately, the users of pte_offset_map() (__pte_map() has no other users)
|| are few, both in arch/arm and outside of arch. All arm ones are pulling
|| linux/highmem (arch/arm/mm/{pgd,fault*}.c). Outside of arch we have several
|| that pull highmem.h (by way of rmap.h or pagemap.h, usually):
|| fs/userfaultfd.c, mm/gup.c, mm/hmm.c, mm/huge_memory.c,
|| mm/khugepaged.c, mm/memory-failure.c, mm/memory.c, mm/migrate.c,
|| mm/mremap.c, mm/page_vma_mapped.c, mm/swap_state.c, mm/swapfile.c,
|| mm/vmalloc.c
|| and then there are these in linux/mm.h:
||
|| #define pte_offset_map_lock(mm, pmd, address, ptlp) \
|| ({ \
|| spinlock_t *__ptl = pte_lockptr(mm, pmd
Re: [PATCH V2 00/11] Subject: Remove duplicated kmap code
On Mon, May 04, 2020 at 01:17:41PM -0700, Ira Weiny wrote: > > || * arm: much, much worse. We have several files that pull > > linux/highmem.h: > > || arch/arm/mm/cache-feroceon-l2.c, arch/arm/mm/cache-xsc3l2.c, > > || arch/arm/mm/copypage-*.c, arch/arm/mm/dma-mapping.c, arch/arm/mm/flush.c, > > || arch/arm/mm/highmem.c, arch/arm/probes/uprobes/core.c, > > || arch/arm/include/asm/kvm_mmu.h (kmap_atomic_pfn()). > > || Those are fine, but we also have this: > > || arch/arm/include/asm/pgtable.h:200:#define __pte_map(pmd) > > (pte_t *)kmap_atomic(pmd_page(*(pmd))) > > || arch/arm/include/asm/pgtable.h:208:#define pte_offset_map(pmd,addr) > > (__pte_map(pmd) + pte_index(addr)) > > || and sure as hell, asm/pgtable.h does *NOT* pull linux/highmem.h. > > It does not pull asm/highmem.h either... No, but the users of those macros need to be considered. > > || #define pte_offset_map(dir, addr) \ > > || ((pte_t *) kmap_atomic(pmd_page(*(dir))) + pte_index(addr)) > > || One pte_offset_map user in arch/microblaze: > > || arch/microblaze/kernel/signal.c:207:ptep = pte_offset_map(pmdp, > > address); > > || Messy, but doesn't require any changes (we have asm/pgalloc.h included > > || there, and that pull linux/highmem.h). > > AFAICS asm/pgtable.h does not include asm/highmem.h here... > > So looks like arch/microblaze/kernel/signal.c will need linux/highmem.h See above - line 39 in there is #include and line 14 in arch/microblaze/include/asm/pgalloc.h is #include It's conditional upon CONFIG_MMU in there, but so's the use of pte_offset_map() in arch/microblaze/kernel/signal.c So it shouldn't be a problem. > > || * xtensa: users in arch/xtensa/kernel/pci-dma.c, > > arch/xtensa/mm/highmem.c, > > || arch/xtensa/mm/cache.c and arch/xtensa/platforms/iss/simdisk.c (all pull > > || linux/highmem.h). > > Actually > > arch/xtensa/mm/cache.c gets linux/highmem.h from linux/pagemap.h > > arch/xtensa/platforms/iss/simdisk.c may have an issue? > linux/blkdev.h -> CONFIG_BLOCK -> linux/pagemap.h -> linux/highmem.h > But simdisk.c requires BLK_DEV_SIMDISK -> CONFIG_BLOCK... > Yep - see above re major chain of indirect includes conditional upon CONFIG_BLOCK and its uses in places that only build with such configs. There's a plenty of similar considerations outside of arch/*, unfortunately... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] arch/{mips,sparc,microblaze,powerpc}: Don't enable pagefault/preempt twice
On Tue, May 19, 2020 at 09:54:22AM -0700, Guenter Roeck wrote:
> On Mon, May 18, 2020 at 11:48:43AM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > The kunmap_atomic clean up failed to remove one set of pagefault/preempt
> > enables when vaddr is not in the fixmap.
> >
> > Fixes: bee2128a09e6 ("arch/kunmap_atomic: consolidate duplicate code")
> > Signed-off-by: Ira Weiny
>
> microblazeel works with this patch, as do the nosmp sparc32 boot tests,
> but sparc32 boot tests with SMP enabled still fail with lots of messages
> such as:
BTW, what's your setup for sparc32 boot tests? IOW, how do you manage to
shrink the damn thing enough to have the loader cope with it? I hadn't
been able to do that for the current mainline ;-/
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] arch/{mips,sparc,microblaze,powerpc}: Don't enable pagefault/preempt twice
On Thu, May 21, 2020 at 03:20:46PM -0700, Guenter Roeck wrote:
> On 5/21/20 10:27 AM, Al Viro wrote:
> > On Tue, May 19, 2020 at 09:54:22AM -0700, Guenter Roeck wrote:
> >> On Mon, May 18, 2020 at 11:48:43AM -0700, ira.we...@intel.com wrote:
> >>> From: Ira Weiny
> >>>
> >>> The kunmap_atomic clean up failed to remove one set of pagefault/preempt
> >>> enables when vaddr is not in the fixmap.
> >>>
> >>> Fixes: bee2128a09e6 ("arch/kunmap_atomic: consolidate duplicate code")
> >>> Signed-off-by: Ira Weiny
> >>
> >> microblazeel works with this patch, as do the nosmp sparc32 boot tests,
> >> but sparc32 boot tests with SMP enabled still fail with lots of messages
> >> such as:
> >
> > BTW, what's your setup for sparc32 boot tests? IOW, how do you manage to
> > shrink the damn thing enough to have the loader cope with it? I hadn't
> > been able to do that for the current mainline ;-/
> >
>
> defconfig seems to work just fine, even after enabling various debug
> and file system options.
The hell? How do you manage to get the kernel in? sparc32_defconfig
ends up with 5316876 bytes unpacked...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] arch/{mips,sparc,microblaze,powerpc}: Don't enable pagefault/preempt twice
On Thu, May 21, 2020 at 11:46:12PM +0100, Al Viro wrote:
> On Thu, May 21, 2020 at 03:20:46PM -0700, Guenter Roeck wrote:
> > On 5/21/20 10:27 AM, Al Viro wrote:
> > > On Tue, May 19, 2020 at 09:54:22AM -0700, Guenter Roeck wrote:
> > >> On Mon, May 18, 2020 at 11:48:43AM -0700, ira.we...@intel.com wrote:
> > >>> From: Ira Weiny
> > >>>
> > >>> The kunmap_atomic clean up failed to remove one set of pagefault/preempt
> > >>> enables when vaddr is not in the fixmap.
> > >>>
> > >>> Fixes: bee2128a09e6 ("arch/kunmap_atomic: consolidate duplicate code")
> > >>> Signed-off-by: Ira Weiny
> > >>
> > >> microblazeel works with this patch, as do the nosmp sparc32 boot tests,
> > >> but sparc32 boot tests with SMP enabled still fail with lots of messages
> > >> such as:
> > >
> > > BTW, what's your setup for sparc32 boot tests? IOW, how do you manage to
> > > shrink the damn thing enough to have the loader cope with it? I hadn't
> > > been able to do that for the current mainline ;-/
> > >
> >
> > defconfig seems to work just fine, even after enabling various debug
> > and file system options.
>
> The hell? How do you manage to get the kernel in? sparc32_defconfig
> ends up with 5316876 bytes unpacked...
Incidentally, trying to load it via -kernel/-initrd leads to
Configuration device id QEMU version 1 machine id 64
Probing SBus slot 0 offset 0
Probing SBus slot 1 offset 0
Probing SBus slot 2 offset 0
Probing SBus slot 3 offset 0
Probing SBus slot 15 offset 0
Invalid FCode start byte
CPUs: 1 x TI,TMS390Z55
UUID: ----
Welcome to OpenBIOS v1.1 built on Dec 27 2018 19:17
Type 'help' for detailed information
[sparc] Kernel already loaded
switching to new context:
PROMLIB: obio_ranges 1
PROMLIB: Sun Boot Prom Version 3 Revision 2
Linux version 5.7.0-rc1-2-gcf51e129b968 (al@duke) (gcc version 6.3.0
20170516 (Debian 6.3.0-18), GNU ld (GNU Binutils for Debian) 2.28) #32 Thu May
21 18:36:07 EDT 2020
printk: bootconsole [earlyprom0] enabled
ARCH: SUN4M
TYPE: Sun4m SparcStation10/20
Ethernet address: 52:54:00:12:34:56
303MB HIGHMEM available.
OF stdout device is: /obio/zs@0,10:a
PROM: Built device tree with 30051 bytes of memory.
Booting Linux...
Power off control detected.
Kernel panic - not syncing: Failed to allocate memory for percpu areas.
CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc1-2-gcf51e129b968 #32
[f04f92a8 :
setup_per_cpu_areas+0x58/0x90 ]
[f04edbf4 :
start_kernel+0xc0/0x4a0 ]
[f04ed43c :
continue_boot+0x324/0x334 ]
[ :
0x0 ]
Press Stop-A (L1-A) from sun keyboard or send break
twice on console to return to the boot prom
---[ end Kernel panic - not syncing: Failed to allocate memory for percpu
areas. ]---
Giving guest more RAM doesn't change the outcome (well, the number HIGHMEM line
is
obviously higher, but that's it).
So which sparc32 kernel have you booted with defconfig and how have you done
that?
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] arch/{mips,sparc,microblaze,powerpc}: Don't enable pagefault/preempt twice
On Thu, May 21, 2020 at 06:11:08PM -0700, Guenter Roeck wrote: > Mainline, with: > > qemu-system-sparc -M SS-4 -kernel arch/sparc/boot/zImage -no-reboot \ > -snapshot -drive file=rootfs.ext2,format=raw,if=scsi \ > -append "panic=-1 slub_debug=FZPUA root=/dev/sda console=ttyS0" > -nographic -monitor none > > The machine doesn't really matter, though. It does, unfortunately - try that with SS-10 and watch what happens ;-/ ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] arch/{mips,sparc,microblaze,powerpc}: Don't enable pagefault/preempt twice
On Fri, May 22, 2020 at 02:29:50AM +0100, Al Viro wrote: > On Thu, May 21, 2020 at 06:11:08PM -0700, Guenter Roeck wrote: > > > Mainline, with: > > > > qemu-system-sparc -M SS-4 -kernel arch/sparc/boot/zImage -no-reboot \ > > -snapshot -drive file=rootfs.ext2,format=raw,if=scsi \ > > -append "panic=-1 slub_debug=FZPUA root=/dev/sda console=ttyS0" > > -nographic -monitor none > > > > The machine doesn't really matter, though. > > It does, unfortunately - try that with SS-10 and watch what happens ;-/ Ugh... It's actually something in -m handling: -m 256 passes, -m 512 leads to that panic. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2] fs/dax: include to fix build error on ARC
On Thu, Dec 31, 2020 at 08:29:14PM -0800, Randy Dunlap wrote: > fs/dax.c uses copy_user_page() but ARC does not provide that interface, > resulting in a build error. > > Provide copy_user_page() in (beside copy_page()) and > add to fs/dax.c to fix the build error. > > ../fs/dax.c: In function 'copy_cow_page_dax': > ../fs/dax.c:702:2: error: implicit declaration of function 'copy_user_page'; > did you mean 'copy_to_user_page'? [-Werror=implicit-function-declaration] Could somebody explain what the force-cast is doing in there? I mean, the call is copy_user_page(vto, (void __force *)kaddr, vaddr, to); kaddr is a local variable there, declared as void *; AFAICS, that had been pure cargo-cult since commit 7a9eb20666317794d0279843fbd091af93907780 Author: Dan Williams Date: Fri Jun 3 18:06:47 2016 -0700 pmem: kill __pmem address space I mean, it's been more than 4 years, time to bury that body... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 07/14] uaccess: generalize access_ok()
On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
> diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> index c7b763d2f526..8867ddf3e6c7 100644
> --- a/arch/csky/kernel/signal.c
> +++ b/arch/csky/kernel/signal.c
> @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal
> *ksig,
> static int
> setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> {
> - struct rt_sigframe *frame;
> + struct rt_sigframe __user *frame;
> int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
Minor nit: might make sense to separate annotations (here, on nios2, etc.) from
the rest...
This, OTOH,
> diff --git a/arch/sparc/include/asm/uaccess_64.h
> b/arch/sparc/include/asm/uaccess_64.h
> index 5c12fb46bc61..000bac67cf31 100644
> --- a/arch/sparc/include/asm/uaccess_64.h
> +++ b/arch/sparc/include/asm/uaccess_64.h
...
> -static inline bool __chk_range_not_ok(unsigned long addr, unsigned long
> size, unsigned long limit)
> -{
> - if (__builtin_constant_p(size))
> - return addr > limit - size;
> -
> - addr += size;
> - if (addr < size)
> - return true;
> -
> - return addr > limit;
> -}
> -
> -#define __range_not_ok(addr, size, limit) \
> -({ \
> - __chk_user_ptr(addr); \
> - __chk_range_not_ok((unsigned long __force)(addr), size, limit); \
> -})
> -
> -static inline int __access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> -
> -static inline int access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
is really wrong. For sparc64, access_ok() should always be true.
This __range_not_ok() thing is used *only* for valid_user_frame() in
arch/sparc/kernel/perf_event.c - it's not a part of normal access_ok()
there.
sparc64 has separate address spaces for kernel and for userland; access_ok()
had never been useful there.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 04/14] x86: use more conventional access_ok() definition
On Mon, Feb 14, 2022 at 12:01:05PM -0800, Linus Torvalds wrote: > On Mon, Feb 14, 2022 at 11:46 AM Arnd Bergmann wrote: > > > > As Al pointed out, they turned out to be necessary on sparc64, but the only > > definitions are on sparc64 and x86, so it's possible that they serve a > > similar > > purpose here, in which case changing the limit from TASK_SIZE to > > TASK_SIZE_MAX is probably wrong as well. > > x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the > get_user() assembler implementation does the same. > > I think any __range_not_ok() users that use TASK_SIZE are entirely > historical, and should be just fixed. IIRC, that was mostly userland stack trace collection in perf. I'll try to dig in archives and see what shows up - it's been a while ago... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 05/14] uaccess: add generic __{get,put}_kernel_nofault
On Mon, Feb 14, 2022 at 05:34:43PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> All architectures that don't provide __{get,put}_kernel_nofault() yet
> can implement this on top of __{get,put}_user.
>
> Add a generic version that lets everything use the normal
> copy_{from,to}_kernel_nofault() code based on these, removing the last
> use of get_fs()/set_fs() from architecture-independent code.
I'd put the list of those architectures (AFAICS, that's alpha, ia64,
microblaze, nds32, nios2, openrisc, sh, sparc32, xtensa) into commit
message - it's not that hard to find out, but...
And AFAICS, you've missed nios2 - see
#define __put_user(x, ptr) put_user(x, ptr)
in there. nds32 oddities are dealt with earlier in the series, this
one is not...
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 09/14] m68k: drop custom __access_ok()
On Mon, Feb 14, 2022 at 05:34:47PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> While most m68k platforms use separate address spaces for user
> and kernel space, at least coldfire does not, and the other
> ones have a TASK_SIZE that is less than the entire 4GB address
> range.
>
> Using the generic implementation of __access_ok() stops coldfire
> user space from trivially accessing kernel memory, and is probably
> the right thing elsewhere for consistency as well.
Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
(and trim the comment down to "coldfire and 68000 will pick generic
variant")?
> Signed-off-by: Arnd Bergmann
> ---
> arch/m68k/include/asm/uaccess.h | 13 -
> 1 file changed, 13 deletions(-)
>
> diff --git a/arch/m68k/include/asm/uaccess.h b/arch/m68k/include/asm/uaccess.h
> index d6bb5720365a..64914872a5c9 100644
> --- a/arch/m68k/include/asm/uaccess.h
> +++ b/arch/m68k/include/asm/uaccess.h
> @@ -10,19 +10,6 @@
> #include
> #include
> #include
> -
> -/* We let the MMU do all checking */
> -static inline int __access_ok(const void __user *addr,
> - unsigned long size)
> -{
> - /*
> - * XXX: for !CONFIG_CPU_HAS_ADDRESS_SPACES this really needs to check
> - * for TASK_SIZE!
> - * Removing this helper is probably sufficient.
> - */
> - return 1;
> -}
> -#define __access_ok __access_ok
> #include
>
> /*
> --
> 2.29.2
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 11/14] sparc64: remove CONFIG_SET_FS support
On Mon, Feb 14, 2022 at 05:34:49PM +0100, Arnd Bergmann wrote: > -/* > - * Sparc64 is segmented, though more like the M68K than the I386. > - * We use the secondary ASI to address user memory, which references a > - * completely different VM map, thus there is zero chance of the user > - * doing something queer and tricking us into poking kernel memory. Actually, this part of comment probably ought to stay - it is relevant for understanding what's going on (e.g. why is access_ok() always true, etc.) ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 04/14] x86: use more conventional access_ok() definition
On Mon, Feb 14, 2022 at 08:17:07PM +, Al Viro wrote: > On Mon, Feb 14, 2022 at 12:01:05PM -0800, Linus Torvalds wrote: > > On Mon, Feb 14, 2022 at 11:46 AM Arnd Bergmann wrote: > > > > > > As Al pointed out, they turned out to be necessary on sparc64, but the > > > only > > > definitions are on sparc64 and x86, so it's possible that they serve a > > > similar > > > purpose here, in which case changing the limit from TASK_SIZE to > > > TASK_SIZE_MAX is probably wrong as well. > > > > x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the > > get_user() assembler implementation does the same. > > > > I think any __range_not_ok() users that use TASK_SIZE are entirely > > historical, and should be just fixed. > > IIRC, that was mostly userland stack trace collection in perf. > I'll try to dig in archives and see what shows up - it's been > a while ago... After some digging: access_ok() needs only to make sure that MMU won't go anywhere near the kernel page tables; address limit for 32bit threads is none of its concern, so TASK_SIZE_MAX is right for it. valid_user_frame() in arch/x86/events/core.c: used while walking the userland call chain. The reason it's not access_ok() is only that perf_callchain_user() might've been called from interrupt that came while we'd been under KERNEL_DS. That had been back in 2015 and it had been obsoleted since 2017, commit 88b0193d9418 (perf/callchain: Force USER_DS when invoking perf_callchain_user()). We had been guaranteed USER_DS ever since. IOW, it could've reverted to use of access_ok() at any point after that. TASK_SIZE vs TASK_SIZE_MAX is pretty much an accident there - might've been TASK_SIZE_MAX from the very beginning. copy_stack_frame() in arch/x86/kernel/stacktrace.c: similar story, except the commit that made sure callers will have USER_DS - cac9b9a4b083 (stacktrace: Force USER_DS for stack_trace_save_user()) in this case. Also could've been using access_ok() just fine. Amusingly, access_ok() used to be there, until it had been replaced with explicit check on Jul 22 2019 - 4 days after that had been made useless by fix in the caller... copy_from_user_nmi(). That one is a bit more interesting. We have a call chain from perf_output_sample_ustack() (covered by force_uaccess_begin() these days, not that it mattered for x86 now), there's something odd in dumpstack.c:copy_code() (with explicit check for TASK_SIZE_MAX in the caller) and there's a couple of callers in Intel PMU code. AFAICS, there's no reason whatsoever to use TASK_SIZE in that one - the point is to prevent copyin from the kernel memory, and in that respect TASK_SIZE_MAX isn't any worse. The check in copy_code() probably should go. So all of those guys should be simply switched to access_ok(). Might be worth making that a preliminary patch - it's independent from everything else and there's no point folding it into any of the patches in the series. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 14/14] uaccess: drop set_fs leftovers
On Mon, Feb 14, 2022 at 05:34:52PM +0100, Arnd Bergmann wrote: > diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h > index b5835325d44b..2f4a1b1ef387 100644 > --- a/arch/parisc/include/asm/futex.h > +++ b/arch/parisc/include/asm/futex.h > @@ -99,7 +99,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > /* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is >* our gateway page, and causes no end of trouble... >*/ > - if (uaccess_kernel() && !uaddr) > + if (!uaddr) > return -EFAULT; Huh? uaccess_kernel() is removed since it becomes always false now, so this looks odd. AFAICS, the comment above that check refers to futex_detect_cmpxchg() -> cmpxchg_futex_value_locked() -> futex_atomic_cmpxchg_inatomic() call chain. Which had been gone since commit 3297481d688a (futex: Remove futex_cmpxchg detection). The comment *and* the check should've been killed off back then. Let's make sure to get both now... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 09/14] m68k: drop custom __access_ok()
On Tue, Feb 15, 2022 at 07:29:42AM +0100, Christoph Hellwig wrote: > On Tue, Feb 15, 2022 at 12:37:41AM +0000, Al Viro wrote: > > Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES > > (and trim the comment down to "coldfire and 68000 will pick generic > > variant")? > > I wonder if we should invert CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, > select the separate address space config for s390, sparc64, non-coldfire > m68k and mips with EVA and then just have one single access_ok for > overlapping address space (as added by Arnd) and non-overlapping ones > (always return true). parisc is also such... How about select ALTERNATE_SPACE_USERLAND for that bunch? While we are at it, how many unusual access_ok() instances are left after this series? arm64, itanic, um, anything else? FWIW, sparc32 has a slightly unusual instance (see uaccess_32.h there); it's obviously cheaper than generic and I wonder if the trick is legitimate (and applicable elsewhere, perhaps)... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/18] clean up asm/uaccess.h, kill set_fs for good
On Thu, Feb 17, 2022 at 07:20:11AM +, Christophe Leroy wrote: > And we have also > user_access_begin()/user_read_access_begin()/user_write_access_begin() > which call access_ok() then do the real work. Could be made generic with > call to some arch specific __user_access_begin() and friends after the > access_ok() and eventually the might_fault(). Not a good idea, considering the fact that we do not want to invite uses of "faster" variants... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/18] clean up asm/uaccess.h, kill set_fs for good
On Thu, Feb 17, 2022 at 08:49:59AM +0100, Arnd Bergmann wrote:
> Same here: architectures can already provide a __put_user_fn()
> and __get_user_fn(), to get the generic versions of the interface,
> but few architectures use that. You can actually get all the interfaces
> by just providing raw_copy_from_user() and raw_copy_to_user(),
> but the get_user/put_user versions you get from that are fairly
> inefficient.
FWIW, __{get,put}_user_{8,16,32,64} would probably make it easier to
unify. That's where the really variable part tends to be, anyway.
IMO __get_user_fn() had been a mistake.
One thing I somewhat dislike about the series is the boilerplate in
asm/uaccess.h instances - #include in
a lot of them might make sense as a transitory state, but getting
stuck with those indefinitely...
BTW, do we need user_addr_max() anymore? The definition in
asm-generic/access-ok.h is the only one, so ifndef around it is pointless.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] bit_spinlock: Include
On Tue, Jan 10, 2023 at 07:08:33PM +0100, Christophe JAILLET wrote: > Le 10/01/2023 à 08:19, Vineet Gupta a écrit : > > > > On 1/8/23 11:04, Christophe JAILLET wrote: > > > In an attempt to simplify some includes in , it > > > appeared, when compiling fs/ecryptfs/dentry.c, that > > > > > > was relying on other includes to get the definition of cpu_relax(). > > > (see [1]) > > > > > > It broke on arc. > > > > It its just ARC that broke, maybe we can do something there ? > > Hi, > > It is all what build-bots have spotted so far :) > > I don't think that "fixing" it in ARC is the right approach, unless I missed > something. > > does use cpu_relax(), so it should include what is > need for that, and not rely on other black magic. Umm... That's not obvious - it only uses cpu_relax() in macros, so missing include would not cause problems if all actual users of those macros happen to pull the needed header by other means. Said that, we have 1) defined directly in asm/processor.h, using nothing but the stuff provided by compiler.h: alpha, arc, csky, loongarch, m68k, microblaze, nios2, openrisc, parisc, s390, sh, xtensa 2) same, using something in headers pulled by asm/processor.h itself: ia64 (needs asm/intrinsic.h) hexagon (needs asm/hexagon_vm.h) um (needs arch/um/include/linux/time-internal.h) 3) same, but defined in something pulled by asm/processor.h rather than in asm/processor.h itself; asm/vdso/processor.h is the common location - those are the cases when we share the same definition for kernel and vdso builds sparc (asm/processor_32.h or asm/processor_64.h) arm (asm/vdso/processor.h) arm64 (asm/vdso/processor.h) powerpc (asm/vdso/processor.h) x86 (asm/vdso/processor.h) riscv (asm/vdso/processor.h; needs several headers included there - jump_label.h, etc.) mips (asm/vdso/processor.h, needs asm/barrier.h, pulled from asm/processor.h by way of linux/atomic.h -> asm/atomic.h -> asm/barrier.h) So asm/processor.h is sufficient for working cpu_relax() and if some arch-independent code wants cpu_relax() it should pull either asm/processor.h or linux/processor.h ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
