linux-kernel - Re: [PATCH] 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] [day] [month] [year] [list]

Message-ID: <CALYGNiP_GA2Qk+0R9uTST3Oe9YgS9k6K1ddbCbmviTo5vRRJNQ@mail.gmail.com>
Date:	Wed, 11 Mar 2015 16:26:50 +0300
From:	Konstantin Khlebnikov <koct9i@...il.com>
To:	Davidlohr Bueso <dave@...olabs.net>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Cyrill Gorcunov <gorcunov@...il.com>,
	Davidlohr Bueso <dave.bueso@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: replace mmap_sem for mm->exe_file serialization

On Wed, Mar 11, 2015 at 3:40 PM, Davidlohr Bueso <dave@...olabs.net> wrote:
> On Wed, 2015-03-11 at 15:21 +0300, Konstantin Khlebnikov wrote:
>> On Fri, Feb 27, 2015 at 9:34 PM, Davidlohr Bueso <dave@...olabs.net> wrote:
>> > On Fri, 2015-02-27 at 18:36 +0100, Oleg Nesterov wrote:
>> >> On 02/26, Cyrill Gorcunov wrote:
>> >> >
>> >> > On Thu, Feb 26, 2015 at 11:36:57AM -0800, Davidlohr Bueso wrote:
>> >> > > We currently use the mmap_sem to serialize the mm exe_file.
>> >> > > This is atrocious and a clear example of the misuses this
>> >> > > lock has all over the place, making any significant changes
>> >> > > to the address space locking that much more complex and tedious.
>> >> > > This also has to do of how we used to check for the vma's vm_file
>> >> > > being VM_EXECUTABLE (much of which was replaced by 2dd8ad81e31).
>> >> > >
>> >> > > This patch, therefore, removes the mmap_sem dependency and
>> >> > > introduces a specific lock for the exe_file (rwlock_t, as it is
>> >> > > read mostly and protects a trivial critical region). As mentioned,
>> >> > > the motivation is to cleanup mmap_sem (as opposed to exe_file
>> >> > > performance).
>> >>
>> >> Well, I didn't see the patch, can't really comment.
>> >>
>> >> But I have to admit that this looks as atrocious and a clear example of
>> >> "lets add yet another random lock which we will regret about later" ;)
>> >>
>> >> rwlock_t in mm_struct just to serialize access to exe_file?
>> >
>> > I don't see why this is a random lock nor how would we regret this
>> > later. I regret having to do these kind of patches because people were
>> > lazy and just relied on mmap_sem without thinking beyond their use case.
>>
>> That's history: exe_file had direct relation to mm->mmap_sem,
>> that was file from first executable vma. After my patch it's less
>> related to vmas.
>
> Indeed. Yet I'm not changing the exe_file address space semantics at
> all.
>
>>
>> > As mentioned I'm also planning on creating an own sort of
>> > exe_file_struct, which would be an isolated entity (still in the mm
>> > though), with its own locking and prctl bits, that would tidy mm_struct
>> > a bit. RCU was something else I considered, but it doesn't suite well in
>> > all paths and we would still need a spinlock when updating the file
>> > anyway.
>>
>> Please don't. What's wrong with mmap_sem?
>>
>> Do you want optimize reading mm->exe_file?
>
> No, I want to get rid of certain things being done under mmap_sem,
> that's all. This is not performance motivated, it's to allow future work
> on lock breaking. I've just yesterday explained this at lsfmm (and not
> only related to exe_file). In any case I've clean up this patch and
> added more on top to create a friendlier interface, I'll send that out a
> bit later.
>
>> Then you should use rcu for that: struct file is rcu-protected thing.
>> See fget(), you could do something like that.
>
> As mentioned, not all exe paths are RCU friendly ;) We'd at least need
> srcu, but that's neither here nor there. A rwlock is suficient to get
> the job done and we really need not care much about optimizing this
> particular file further.

I mean you could make mm->exe_file rcu protected pointer and use
everywhere get_mm_exe_file() which grabs file refcount under rcu and
returns pointer.

>
> Thanks,
> Davidlohr
>
--
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/