Re: [PATCH v3 38/41] powerpc: convert put_page() to put_user_page*()
Hi John,
john.hubb...@gmail.com writes:
> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c
> b/arch/powerpc/mm/book3s64/iommu_api.c
> index b056cae3388b..e126193ba295 100644
> --- a/arch/powerpc/mm/book3s64/iommu_api.c
> +++ b/arch/powerpc/mm/book3s64/iommu_api.c
> @@ -203,6 +202,7 @@ static void mm_iommu_unpin(struct
> mm_iommu_table_group_mem_t *mem)
> {
> long i;
> struct page *page = NULL;
> + bool dirty = false;
I don't think you need that initialisation do you?
> if (!mem->hpas)
> return;
> @@ -215,10 +215,9 @@ static void mm_iommu_unpin(struct
> mm_iommu_table_group_mem_t *mem)
> if (!page)
> continue;
>
> - if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY)
> - SetPageDirty(page);
> + dirty = mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY;
> - put_page(page);
> + put_user_pages_dirty_lock(&page, 1, dirty);
> mem->hpas[i] = 0;
> }
> }
cheers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 38/41] powerpc: convert put_page() to put_user_page*()
John Hubbard writes:
> On 8/7/19 10:42 PM, Michael Ellerman wrote:
>> Hi John,
>>
>> john.hubb...@gmail.com writes:
>>> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c
>>> b/arch/powerpc/mm/book3s64/iommu_api.c
>>> index b056cae3388b..e126193ba295 100644
>>> --- a/arch/powerpc/mm/book3s64/iommu_api.c
>>> +++ b/arch/powerpc/mm/book3s64/iommu_api.c
>>> @@ -203,6 +202,7 @@ static void mm_iommu_unpin(struct
>>> mm_iommu_table_group_mem_t *mem)
>>> {
>>> long i;
>>> struct page *page = NULL;
>>> + bool dirty = false;
>>
>> I don't think you need that initialisation do you?
>>
>
> Nope, it can go. Fixed locally, thanks.
Thanks.
> Did you get a chance to look at enough of the other bits to feel comfortable
> with the patch, overall?
Mostly :) It's not really my area, but all the conversions looked
correct to me as best as I could tell.
So I'm fine for it to go in as part of the series:
Acked-by: Michael Ellerman (powerpc)
cheers
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t
Elena Reshetova writes: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: Elena Reshetova > Signed-off-by: Hans Liljestrand > Signed-off-by: Kees Cook > Signed-off-by: David Windsor > --- > drivers/md/md.c | 6 +++--- > drivers/md/md.h | 3 ++- > 2 files changed, 5 insertions(+), 4 deletions(-) When booting linux-next (specifically 5be4921c9958ec) I'm seeing the backtrace below. I suspect this patch is just exposing an existing issue? cheers [0.230738] md: Waiting for all devices to be available before autodetect [0.230742] md: If you don't use raid, use raid=noautodetect [0.230962] refcount_t: increment on 0; use-after-free. [0.230988] [ cut here ] [0.230996] WARNING: CPU: 0 PID: 1 at lib/refcount.c:114 .refcount_inc+0x5c/0x70 [0.231001] Modules linked in: [0.231006] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc1-gccN-next-20170310-g5be4921 #1 [0.231012] task: c0004940 task.stack: c0004944 [0.231016] NIP: c05ac6bc LR: c05ac6b8 CTR: c0743390 [0.231021] REGS: c00049443160 TRAP: 0700 Not tainted (4.11.0-rc1-gccN-next-20170310-g5be4921) [0.231026] MSR: 80029032 [0.231033] CR: 24024422 XER: 000c [0.231038] CFAR: c0a5356c SOFTE: 1 [0.231038] GPR00: c05ac6b8 c000494433e0 c1079d00 002b [0.231038] GPR04: 00ef c10418a0 [0.231038] GPR08: 4af8 c0ecc9a8 c0ecc9a8 [0.231038] GPR12: 28024824 c6bb c00049443a00 [0.231038] GPR16: c00049443a10 [0.231038] GPR20: c0f7dd20 [0.231038] GPR24: 014080c0 c12060b8 c1206080 0009 [0.231038] GPR28: c0f7dde0 0090 c000461ae800 [0.231100] NIP [c05ac6bc] .refcount_inc+0x5c/0x70 [0.231104] LR [c05ac6b8] .refcount_inc+0x58/0x70 [0.231108] Call Trace: [0.231112] [c000494433e0] [c05ac6b8] .refcount_inc+0x58/0x70 (unreliable) [0.231120] [c00049443450] [c086c008] .mddev_find+0x1e8/0x430 [0.231125] [c00049443530] [c0872b6c] .md_open+0x2c/0x140 [0.231132] [c000494435c0] [c03962a4] .__blkdev_get+0xd4/0x520 [0.231138] [c00049443690] [c0396cc0] .blkdev_get+0x1c0/0x4f0 [0.231145] [c00049443790] [c0336d64] .do_dentry_open.isra.1+0x2a4/0x410 [0.231152] [c00049443830] [c03523f4] .path_openat+0x624/0x1580 [0.231157] [c00049443990] [c0354ce4] .do_filp_open+0x84/0x120 [0.231163] [c00049443b10] [c0338d74] .do_sys_open+0x214/0x300 [0.231170] [c00049443be0] [c0da69ac] .md_run_setup+0xa0/0xec [0.231176] [c00049443c60] [c0da4fbc] .prepare_namespace+0x60/0x240 [0.231182] [c00049443ce0] [c0da47a8] .kernel_init_freeable+0x330/0x36c [0.231190] [c00049443db0] [c000dc44] .kernel_init+0x24/0x160 [0.231197] [c00049443e30] [c000badc] .ret_from_kernel_thread+0x58/0x7c [0.231202] Instruction dump: [0.231206] 6000 3d22ffee 89296bfb 2f89 409effdc 3c62ffc6 3921 3d42ffee [0.231216] 38630928 992a6bfb 484a6e79 6000 <0fe0> 4bb8 6000 6000 [0.231226] ---[ end trace 8c51f269ad91ffc2 ]--- [0.231233] md: Autodetecting RAID arrays. [0.231236] md: autorun ... [0.231239] md: ... autorun DONE. [0.234188] EXT4-fs (sda4): mounting ext3 file system using the ext4 subsystem [0.250506] refcount_t: underflow; use-after-free. [0.250531] [ cut here ] [0.250537] WARNING: CPU: 0 PID: 3 at lib/refcount.c:207 .refcount_dec_not_one+0x104/0x120 [0.250542] Modules linked in: [0.250546] CPU: 0 PID: 3 Comm: kworker/0:0 Tainted: GW 4.11.0-rc1-gccN-next-20170310-g5be4921 #1 [0.250553] Workqueue: events .delayed_fput [0.250557] task: c00049404900 task.stack: c00049448000 [0.250562] NIP: c05ac964 LR: c05ac960 CTR: c0743390 [0.250567] REGS: c0004944b530 TRAP: 0700 Tainted: GW (4.11.0-rc1-gccN-next-20170310-g5be4921) [0.250572] MSR: 80029032 [0.250578] CR: 24002422 XER: 0007 [0.250584] CFAR: c0a5356c SOFTE: 1 [0.250584] GPR00: c05ac960 c0004944b7b0 c1079d00 0026 [0.250584] GPR04: 0113 c10418a0 [0.250584] GPR08: 00
Re: [PATCH 1/7] selftest: sync: basic tests for sw_sync framework
Emilio López writes: > These tests are based on the libsync test suite from Android. > This commit lays the ground for future tests, as well as includes > tests for a variety of basic allocation commands. Hi Emilio, Just a few comments on the Makefile. > diff --git a/tools/testing/selftests/sync/Makefile > b/tools/testing/selftests/sync/Makefile > new file mode 100644 > index 000..f67827f > --- /dev/null > +++ b/tools/testing/selftests/sync/Makefile > @@ -0,0 +1,24 @@ > +CC = $(CROSS_COMPILE)gcc lib.mk does that for you. > +CFLAGS := -O2 -g -std=gnu89 -pthread -Wall -Wextra It's more polite to just add to CFLAGS rather than defining it from scratch. That way a user can add CFLAGS via setting them in the environment. ie. this would be: CFLAGS += -O2 -g -std=gnu89 -pthread -Wall -Wextra or: CFLAGS := -O2 -g -std=gnu89 -pthread -Wall -Wextra $(CFLAGS) > +CFLAGS += -I../../../../include/uapi/ Please don't include the unprocessed uapi headers, they are not meant to be directly included in userspace programs. They even say so: include/uapi/linux/types.h:#warning "Attempt to use kernel headers from user space, see http://kernelnewbies.org/KernelHeaders"; > +CFLAGS += -I../../../../include/ Please don't include the *kernel* headers, they're really not meant to be used in userspace programs :) > +CFLAGS += -I../../../../usr/include/ That is the correct place to get them from. They'll have been put there by 'make headers_install'. > +CFLAGS += -I../../../../drivers/dma-buf/ That's also a bit fishy. > +LDFLAGS += -pthread > + > +TEST_PROGS = sync_test > + > +all: $(TEST_PROGS) > + > +include ../lib.mk > + > +SRC = sync_test.o sync.o SRC would usually point to .c files, .o's would be in OBJS or something similar. Though it's not that important obviously. Just listing the .c files in SRC should work, make knows how to turn them into .o's for you. > +TESTS += sync_alloc.o And similarly there. So I think you could just use: SRC := sync_test.c sync.c sync_alloc.c Or if you actually just want to list all the .c files in the directory then this would also work: SRC := $(wildcard *.c) > +sync_test: $(SRC) $(TESTS) > + > +.PHONY: clean lib.mk did that for you. > + > +clean: > + $(RM) sync_test $(SRC) $(TESTS) If you redefined SRC above to be the actual sources then you obviously don't want to clean them, so here you would just use *.o. cheers ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] selftest: sync: basic tests for sw_sync framework
Emilio López writes: > El 22/09/16 a las 06:43, Michael Ellerman escribió: >> Emilio López writes: >> >> Please don't include the *kernel* headers, they're really not meant to >> be used in userspace programs :) >> >>> +CFLAGS += -I../../../../usr/include/ >> >> That is the correct place to get them from. They'll have been put there >> by 'make headers_install'. > > My inspiration here has been tools/testing/selftests/memfd/Makefile, > which does it this way. If I only include the ones on usr then it > doesn't build, as there's no sync_file.h available, even after running > make headers_install. How am I supposed to use the ioctls from there? It looks like it's missing from include/uapi/linux/Kbuild, you need to add it to the list of exported headers: diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index dd604395606b..40411b4ff012 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -397,6 +397,7 @@ header-y += stddef.h header-y += string.h header-y += suspend_ioctls.h header-y += swab.h +header-y += sync_file.h header-y += synclink.h header-y += sysctl.h header-y += sysinfo.h cheers ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] selftest: sync: basic tests for sw_sync framework
Emilio López writes: > Hi, > > El 27/09/16 a las 01:23, Michael Ellerman escribió: >> Emilio López writes: >>> El 22/09/16 a las 06:43, Michael Ellerman escribió: >>>> Emilio López writes: >>>> >>>> Please don't include the *kernel* headers, they're really not meant to >>>> be used in userspace programs :) >>>> >>>>> +CFLAGS += -I../../../../usr/include/ >>>> >>>> That is the correct place to get them from. They'll have been put there >>>> by 'make headers_install'. >>> >>> My inspiration here has been tools/testing/selftests/memfd/Makefile, >>> which does it this way. If I only include the ones on usr then it >>> doesn't build, as there's no sync_file.h available, even after running >>> make headers_install. How am I supposed to use the ioctls from there? >> >> It looks like it's missing from include/uapi/linux/Kbuild, you need to >> add it to the list of exported headers: > > I tried that over the weekend and it worked, but I wondered if it was > the way to go. Thanks for the confirmation :) I've sent a patch for > that[0] now. Great thanks. > With that resolved, CFLAGS can just be > > CFLAGS += -O2 -g -std=gnu89 -pthread -Wall -Wextra > CFLAGS += -I../../../../usr/include/ LGTM. cheers ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
