linux-kernel - Re: [PATCH v9 6/8] mm: Introduce Reported pages

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Sep 2019 19:33:55 +0300
From:   "Kirill A. Shutemov" <kirill@...temov.name>
To:     Alexander Duyck <alexander.h.duyck@...ux.intel.com>
Cc:     Alexander Duyck <alexander.duyck@...il.com>,
        virtio-dev@...ts.oasis-open.org, kvm@...r.kernel.org,
        mst@...hat.com, catalin.marinas@....com, david@...hat.com,
        dave.hansen@...el.com, linux-kernel@...r.kernel.org,
        willy@...radead.org, mhocko@...nel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, will@...nel.org,
        linux-arm-kernel@...ts.infradead.org, osalvador@...e.de,
        yang.zhang.wz@...il.com, pagupta@...hat.com,
        konrad.wilk@...cle.com, nitesh@...hat.com, riel@...riel.com,
        lcapitulino@...hat.com, wei.w.wang@...el.com, aarcange@...hat.com,
        ying.huang@...el.com, pbonzini@...hat.com,
        dan.j.williams@...el.com, fengguang.wu@...el.com,
        kirill.shutemov@...ux.intel.com
Subject: Re: [PATCH v9 6/8] mm: Introduce Reported pages

On Mon, Sep 09, 2019 at 09:25:04AM -0700, Alexander Duyck wrote:
> > Proper description for the config option?
> 
> I can add one. However the feature doesn't do anything without a caller
> that makes use of it. I guess it would make sense to enable this for
> something such as an out-of-tree module to later use.

Description under 'help' section will not make the option user selectable
if you leave 'bool' without description.

> > > +	mutex_lock(&page_reporting_mutex);
> > > +
> > > +	/* nothing to do if already in use */
> > > +	if (rcu_access_pointer(ph_dev_info)) {
> > > +		err = -EBUSY;
> > > +		goto err_out;
> > > +	}
> > 
> > Again, it's from "something went horribly wrong" category.
> > Maybe WARN_ON()?
> 
> That one I am not so sure about. Right now we only have one user for the
> page reporting interface. My concern is if we ever have more than one we
> may experience collisions. The device driver requesting this should
> display an error message if it is not able tor register the interface.

Fair enough.

> > > +	boundary = kcalloc(MAX_ORDER - PAGE_REPORTING_MIN_ORDER,
> > > +			   sizeof(struct list_head *) * MIGRATE_TYPES,
> > > +			   GFP_KERNEL);
> > 
> > Could you comment here on why this size of array is allocated?
> > The calculation is not obvious to a reader.
> 
> Would something like the following work for you?
>         /*
>          * Allocate space to store the boundaries for the zone we are
>          * actively reporting on. We will need to store one boundary
>          * pointer per migratetype, and then we need to have one of these
>          * arrays per order for orders greater than or equal to
>          * PAGE_REPORTING_MIN_ORDER.
>          */

Ack.

-- 
 Kirill A. Shutemov