linux-kernel - Re: [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe

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:	Sun, 15 Mar 2015 15:21:37 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Davidlohr Bueso <dave@...olabs.net>
Cc:	akpm@...ux-foundation.org, viro@...iv.linux.org.uk,
	gorcunov@...nvz.org, koct9i@...il.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file
	serialization

I didn't even read this version, but honestly I don't like it anyway.

I leave the review to Cyrill and Konstantin though, If they like these
changes I won't argue.

But I simply can't understand why are you doing this.

Yes, this code needs cleanups, I agree. Does this series makes it better?
To me it doesn't, and the diffstat below shows that it blows the code.

In fact, to me it complicates this code. For example. Personally I think
that MMF_EXE_FILE_CHANGED should die. And currently we can just remove it.
Not after your patch which adds another dependency.

Or do you think this is performance improvement? I don't think so. Yes,
prctl() abuses mmap_sem, but this not a hot path and the task can only
abuse its own ->mm.

OK, I agree, dup_mm_exe_file() is horrible. But as I already said it can
simply die. We can move this code into dup_mmap() and avoid another
down_read/up_read.

Hmm. And this series is simply wrong without more changes in audit paths.
Unfortunately this is fixable, but let me NACK at least this version ;)

Speaking of cleanups... IIRC Konstantin suggested to rcuify this pointer
and I agree, this looks better than the new lock. But in fact I think
that the cleanups should start with s/get_mm_exe_file//get_mm_exe_path/.
Note that nobody actually needs "file *", every caller needs "struct path".
Plus kill dup_mm_exe_file().

Oleg.

On 03/14, Davidlohr Bueso wrote:
>
> This is a set I created on top of patch 1/4 which also includes mm_struct cleanups
> and dealing with prctl exe_file functionality. Specific details are in each patch.
> Patch 4 is an extra trivial one I found while going through the code.
>
> Applies on top of next-20150313.
>
> Thanks!
>
> Davidlohr Bueso (4):
>   mm: replace mmap_sem for mm->exe_file serialization
>   mm: introduce struct exe_file
>   prctl: move MMF_EXE_FILE_CHANGED into exe_file struct
>   kernel/fork: use pr_alert() for rss counter bugs
>
>  fs/exec.c                |   6 +++
>  include/linux/mm.h       |   4 ++
>  include/linux/mm_types.h |   8 +++-
>  include/linux/sched.h    |   5 +--
>  kernel/fork.c            |  72 ++++++++++++++++++++++++++------
>  kernel/sys.c             | 106 +++++++++++++++++++++++++++--------------------
>  6 files changed, 141 insertions(+), 60 deletions(-)
>
> --
> 2.1.4
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/