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 07:54:30 -0700
From:	Davidlohr Bueso <dave@...olabs.net>
To:	Oleg Nesterov <oleg@...hat.com>
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

On Sun, 2015-03-15 at 15:21 +0100, Oleg Nesterov wrote:
> 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.

Looking at some of the caller paths now, I have to disagree.

> 
> 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.

How could you remove this? I mean it's user functionality, so you need
some way of keeping track of a changed file. But you might be talking
about something else.

> Not after your patch which adds another dependency.

I don't add another dependency, I just replace the current one.

> 
> 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.

I've tried to make it as clear as possible this is a not performance
patch. I guess I've failed. Let me repeat it again: this is *not*
performance motivated ;) This kind of things under mmap_sem prevents
lock breakup.

> 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.

If this series goes to the dumpster then ok I'll send a patch for this,
I have no objection.

> 
> 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 ;)

Could you explain this? Are you referring to the audit.c user? If so
that caller has already been updated.

> 
> 
> Speaking of cleanups... IIRC Konstantin suggested to rcuify this pointer
> and I agree, this looks better than the new lock.

Yes, I can do that in patch 1, but as mentioned, rcu is not really the
question to me, it's the lock for when we change the exe file, so if
it's not mmap_sem we'd still need another lock. If mmap_sem is kept, yes
we can use the read lock in things like get_mm_exe_file() and still rely
on the file ref counting so we wouldn't need to do everything under rcu,
which was a though I originally had.

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/