linux-kernel - Re: [PATCH] tools: adding support for idle page tracking to tool

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] [day] [month] [year] [list]

Message-ID: <5F15F2EE-61F9-4E6F-99B2-B417B02C0F19@cisco.com>
Date:   Thu, 5 Jul 2018 18:14:17 +0000
From:   "Christian Hansen (chansen3)" <chansen3@...co.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
CC:     "xe-linux-external(mailer list)" <xe-linux-external@...co.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tools: adding support for idle page tracking to tool

Yes, I wasn't sure which one went in first.  I can make this one dependent on the other.

On 2018-06-19, 6:39 PM, "Andrew Morton" <akpm@...ux-foundation.org> wrote:

    On Tue, 12 Jun 2018 11:32:23 -0400 Christian Hansen <chansen3@...co.com> wrote:
    
    > Adding a flag which will use the kernels's idle
    > page tracking to mark pages idle.  As the tool already
    > prints the idle flag if set, subsequent runs will show
    > which pages have been accessed since last run.
    
    That sounds useful.
    
    This patch seems to have been prepared against the mainline kernel, so
    it conflicts with your "tools: modifying page-types to include shared
    map counts" patch.  Awkward, but I seem to have got it fixed up.
    
    > ...
    >
    > @@ -566,6 +570,30 @@ static int unpoison_page(unsigned long offset)
    >  	return 0;
    >  }
    >  
    > +static int mark_page_idle(unsigned long offset)
    > +{
    > +	static unsigned long off;
    > +	static uint64_t buf;
    > +	int len;
    > +
    > +	if ((offset / 64 == off / 64) || buf == 0) {
    > +		buf |= 1UL << (offset % 64);
    > +		off = offset;
    > +		return 0;
    > +	}
    > +
    > +	len = pwrite(page_idle_fd, &buf, 8, 8 * (off / 64));
    > +	if (len < 0) {
    > +		perror("mark page idle");
    > +		return len;
    > +	}
    > +
    > +	buf = 1UL << (offset % 64);
    > +	off = offset;
    > +
    > +	return 0;
    > +}
    
    This is a bit cumbersome.  Why not this way:
    
    static int mark_page_idle(unsigned long offset)
    {
    	static unsigned long off;
    	static uint64_t buf;
    	int len;
    
    	if ((offset / 64 != off / 64) && buf != 0) {
    		len = pwrite(page_idle_fd, &buf, 8, 8 * (off / 64));
    		if (len < 0) {
    			perror("mark page idle");
    			return len;
    		}
    	}
    	buf = 1UL << (offset % 64);
    	off = offset;
    	return 0;
    }
    
    Also, it's not very clear what's going on here - the handling of
    offset, off and buf.  Some well-crafted comments would help.
    
    >
    > ...
    >