linux-kernel - Re: [PATCH] perf map: fix overlapped map handling

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:   Fri, 27 Sep 2019 12:35:40 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Steve MacLean <Steve.MacLean@...rosoft.com>
Cc:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Changbin Du <changbin.du@...el.com>,
        Eric Saint-Etienne <eric.saint.etienne@...cle.com>,
        John Keeping <john@...anate.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Song Liu <songliubraving@...com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Leo Yan <leo.yan@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Robbins <brianrob@...rosoft.com>,
        Tom McDonald <Thomas.McDonald@...rosoft.com>,
        John Salem <josalem@...rosoft.com>
Subject: Re: [PATCH] perf map: fix overlapped map handling

Em Fri, Sep 20, 2019 at 09:46:15PM +0000, Steve MacLean escreveu:
> >>  			after->start = map->end;
> >> +			after->pgoff = pos->map_ip(pos, map->end);
> >
> > So is this equivalent to what __split_vma() does in the kernel, i.e.:
> >
> >        if (new_below)
> >                new->vm_end = addr;
> >        else {
> >                new->vm_start = addr;
> >                new->vm_pgoff += ((addr - vma->vm_start) >> PAGE_SHIFT);
> >        }
> >
> > where new->vm_pgoff starts equal to the vm_pgoff of the mmap being split?
> 
> It is roughly equivalent.  The pgoff in struct map is stored in bytes not in pages, so it doesn't include the shift.
> 
> An earlier version of this patch used:
>   			after->start = map->end;
> +			after->pgoff += map->end - pos->start;
> 
> Instead of the newer Functionally equivalent:
>   			after->start = map->end;
> +			after->pgoff = pos->map_ip(pos, map->end);
> 
> I preferred the latter form as it made more sense with the assertion that the mapping of map->end should match in pos and after.

Sorry for the delay in continuing with this discussion, I was at
Plumbers in Lisbon and then some vacations, etc. Also I was hoping
someone else would jump here and provide some Reviewed-by tag, etc :-)

So, if they are equivalent then I think its better to use code that
ressembles the kernel as much as possible, so that when in doubt we can
compare the tools/perf calcs with how the kernel does it, filtering out
things like the PAGE_SHIFT, can we go that way?

Also do you have some reproducer, if you have one then we can try and
have this as a 'perf test' entry, bolting some more checks into
tools/perf/tests/perf-record.c or using it as a start for a test that
stresses this code.

This is not a prerequisite for having your fix on, but would help
checking that perf doesn't regresses in this area.
 
- Arnaldo