| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190905034546.i6eklk6flbqk2hox@linux-r8p5>
Date: Wed, 4 Sep 2019 20:45:46 -0700
From: Davidlohr Bueso <dave@...olabs.net>
To: Michel Lespinasse <walken@...gle.com>
Cc: Ingo Molnar <mingo@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Andrew Morton <akpm@...ux-foundation.org>, x86@...nel.org,
LKML <linux-kernel@...r.kernel.org>,
Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH 1/3] x86,mm/pat: Use generic interval trees
On Wed, 04 Sep 2019, Michel Lespinasse wrote:
>I do not have time for a full review right now, but I did have a quick
>pass at it and it does seem to match the direction I'd like this to
>take.
Thanks, and no worries, I consider all this v5.5 material anyway.
>
>Please let me know if you'd like me to do a full review of this, or if
>it'll be easier to do it on the individual steps once this gets broken
>up.
If nothing else, I would appreciate you making sure I didn't do anything
stupid in the interval_tree_generic.h lookup changes.
>
>BTW are you going to plumbers ? I am and I would love to talk to you
>there, about this and about MM range locking, too.
No, not this year; perhaps lsfmm (although that's not for a while).
>
>Things I noticed in my quick pass:
>
>> diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c
>> index e0ad547786e8..dc9fad8ea84a 100644
>> --- a/drivers/gpu/drm/radeon/radeon_vm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_vm.c
>> @@ -450,13 +450,14 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>> {
>> uint64_t size = radeon_bo_size(bo_va->bo);
>> struct radeon_vm *vm = bo_va->vm;
>> - unsigned last_pfn, pt_idx;
>> + unsigned pt_idx;
>> uint64_t eoffset;
>> int r;
>>
>> if (soffset) {
>> + unsigned last_pfn;
>> /* make sure object fit at this offset */
>> - eoffset = soffset + size - 1;
>> + eoffset = soffset + size;
>> if (soffset >= eoffset) {
>Should probably be if (soffset > eoffset) now (just checking for
>arithmetic overflow).
>> r = -EINVAL;
>> goto error_unreserve;
>> @@ -471,7 +472,7 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>> }
>>
>> } else {
>> - eoffset = last_pfn = 0;
>> + eoffset = 1; /* interval trees are [a,b[ */
>Looks highly suspicious to me, and doesn't jive with the eoffset /=
>RADEON_GPU_PAGE_SIZE below.
>I did not look deep enough into this to figure out what would be correct though.
I think you're right, I will double check this. I think we only wanna do
the RADEON_GPU_PAGE_SIZE division when soffset is non-zero.
>> }
>>
>> mutex_lock(&vm->mutex);
>> diff --git a/drivers/infiniband/hw/hfi1/mmu_rb.c b/drivers/infiniband/hw/hfi1/mmu_rb.c
>> index 14d2a90964c3..d708c45bfabf 100644
>> --- a/drivers/infiniband/hw/hfi1/mmu_rb.c
>> +++ b/drivers/infiniband/hw/hfi1/mmu_rb.c
>> @@ -195,13 +195,13 @@ static struct mmu_rb_node *__mmu_rb_search(struct mmu_rb_handler *handler,
>> trace_hfi1_mmu_rb_search(addr, len);
>> if (!handler->ops->filter) {
>> node = __mmu_int_rb_iter_first(&handler->root, addr,
>> - (addr + len) - 1);
>> + (addr + len));
>> } else {
>> for (node = __mmu_int_rb_iter_first(&handler->root, addr,
>> - (addr + len) - 1);
>> + (addr + len));
>> node;
>> node = __mmu_int_rb_iter_next(node, addr,
>> - (addr + len) - 1)) {
>> + (addr + len))) {
>> if (handler->ops->filter(node, addr, len))
>> return node;
>> }
>
>Weird unnecessary parentheses through this block.
Yes, but I wanted to leave it with the least amount of changes. There are plenty
of places that could use interval_tree_foreach helpers, like the vma tree has.
>
>> @@ -787,7 +787,7 @@ static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
>> struct viommu_domain *vdomain = to_viommu_domain(domain);
>>
>> spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> - node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
>> + node = interval_tree_iter_first(&vdomain->mappings, iova, iova + 1);
>
>I was gonna suggest a stab iterator for the generic interval tree, but
>maybe not if it's only used here.
I considered it as well.
>
>> diff --git a/include/linux/interval_tree_generic.h b/include/linux/interval_tree_generic.h
>> index aaa8a0767aa3..e714e67ebdb5 100644
>> --- a/include/linux/interval_tree_generic.h
>> +++ b/include/linux/interval_tree_generic.h
>> @@ -69,12 +69,12 @@ ITSTATIC void ITPREFIX ## _remove(ITSTRUCT *node, \
>> } \
>> \
>> /* \
>> - * Iterate over intervals intersecting [start;last] \
>> + * Iterate over intervals intersecting [start;last[ \
>
>Maybe I'm extra nitpicky, but I would suggest to use 'end' instead of
>'last' when the intervals are half open: [start;end[
>To me 'last' implies that it's in the interval, while 'end' doesn't
>imply anything (and thus the half open convention used through the
>kernel applies).
That's fair enough.
>similarly ITLAST should be changed to ITEND, etc and similarly in the
>various call sites (drm and others).
>Again, maybe it's nitpicky but I feel changing the name also helps
>verify that we don't leave behind any off-by-one use.
>
>That said, it's really only my preference - if you think it's too
>painful to change, that's OK.
I'll see what I can do, but yeah it might be too much of a pain for
the benefits.
Thanks,
Davidlohr
Powered by blists - more mailing lists