Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751578AbdFFMYc (ORCPT ); Tue, 6 Jun 2017 08:24:32 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:28086 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751305AbdFFMYb (ORCPT ); Tue, 6 Jun 2017 08:24:31 -0400 Subject: Re: [PATCH 2/5] Protectable Memory Allocator To: Tetsuo Handa CC: , , , , , , , , , , References: <20170605192216.21596-1-igor.stoppa@huawei.com> <20170605192216.21596-3-igor.stoppa@huawei.com> <201706060444.v564iWds024768@www262.sakura.ne.jp> <201706062108.JDD17143.MOQFFVtHLJOFOS@I-love.SAKURA.ne.jp> From: Igor Stoppa Message-ID: Date: Tue, 6 Jun 2017 15:23:12 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <201706062108.JDD17143.MOQFFVtHLJOFOS@I-love.SAKURA.ne.jp> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.122.225.51] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.59369EF4.01F2,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: ed181ec4a6e04dbbf9ca4d0b7706b84a Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2294 Lines: 76 On 06/06/17 15:08, Tetsuo Handa wrote: > Igor Stoppa wrote: >>>> +struct pmalloc_node { >>>> + struct hlist_node nodes_list; >>>> + atomic_t used_words; >>>> + unsigned int total_words; >>>> + __PMALLOC_ALIGNED align_t data[]; >>>> +}; >>> >>> Is this __PMALLOC_ALIGNED needed? Why not use "long" and "BITS_PER_LONG" ? >> >> In an earlier version I actually asked the same question. >> It is currently there because I just don't know enough about various >> architectures. The idea of having "align_t" was that it could be tied >> into what is the most desirable alignment for each architecture. >> But I'm actually looking for advise on this. > > I think that let the compiler use natural alignment is OK. On a 64 bit machine the preferred alignment might be either 32 or 64, depending on the application. How can the compiler choose? >>> You need to check for node != NULL before dereference it. >> >> So, if I understood correctly, there shouldn't be a case where node is >> NULL, right? >> Unless it has been tampered/damaged. Is that what you mean? > > I meant to say > > + node = __pmalloc_create_node(req_words); > // this location. > + starting_word = atomic_fetch_add(req_words, &node->used_words); argh, yes >>>> +const char *__pmalloc_check_object(const void *ptr, unsigned long n) >>>> +{ >>>> + unsigned long p; >>>> + >>>> + p = (unsigned long)ptr; >>>> + n += (unsigned long)ptr; >>>> + for (; (PAGE_MASK & p) <= (PAGE_MASK & n); p += PAGE_SIZE) { >>>> + if (is_vmalloc_addr((void *)p)) { >>>> + struct page *page; >>>> + >>>> + page = vmalloc_to_page((void *)p); >>>> + if (!(page && PagePmalloc(page))) >>>> + return msg; >>>> + } >>>> + } >>>> + return NULL; >>>> +} >>> >>> I feel that n is off-by-one if (ptr + n) % PAGE_SIZE == 0 >>> according to check_page_span(). >> >> It seems to work. If I am missing your point, could you please >> use the same format of the example I made, to explain me? > > If ptr == NULL and n == PAGE_SIZE so that (ptr + n) % PAGE_SIZE == 0, > this loop will access two pages (one page containing p == 0 and another > page containing p == PAGE_SIZE) when this loop should access only one > page containing p == 0. When checking n bytes, it's range is 0 to n - 1. oh, so: p = (unsigned long) ptr; n = p + n - 1; -- igor