From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762210AbdJQNW6 (ORCPT ); Tue, 17 Oct 2017 09:22:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44506 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758499AbdJQNWx (ORCPT ); Tue, 17 Oct 2017 09:22:53 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8CBF513AA8 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=bhe@redhat.com Date: Tue, 17 Oct 2017 21:22:46 +0800 From: Baoquan He To: Michal Hocko Cc: LKML , Linus Torvalds , Kees Cook , Jiri Kosina , Al Viro , Oleg Nesterov , Ingo Molnar Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map Message-ID: <20171017132246.GK11198@x1> References: <20171004075059.bbx7madwgwflb7ky@dhcp22.suse.cz> <20171016134446.19910-1-mhocko@kernel.org> <20171016134446.19910-2-mhocko@kernel.org> <20171017122614.GJ11198@x1> <20171017125605.7quzqa3swvagyeiq@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171017125605.7quzqa3swvagyeiq@dhcp22.suse.cz> User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 17 Oct 2017 13:22:53 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/17/17 at 02:56pm, Michal Hocko wrote: > On Tue 17-10-17 20:26:14, Baoquan He wrote: > > Hi Michal, > > > > Earlier I posted a patchset to clean up these in a different way, but > > patches sent to your below mail address are all rejected. > > > > : host mail.kerne.org[104.131.33.237] said: 454 4.1.1 > > : Recipient address rejected: User unknown in virtual > > mailbox table (in reply to RCPT TO command) > > yes, there was a typo in the email address which I've fixed up in the > reply... Your cleanup was mostly about replacing vm_mmap by > get_unmaped_area which is an independent issue to what I am trying to > achieve here. Oops. Yes, I must have typed your address by hand, but not copying from the previous discussion thread. Sorry for that. > > > On 10/16/17 at 03:44pm, Michal Hocko wrote: > > > From: Michal Hocko > > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > > index 6466153f2bf0..09456e2add18 100644 > > > --- a/fs/binfmt_elf.c > > > +++ b/fs/binfmt_elf.c > > > @@ -341,6 +341,29 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, > > > > > > #ifndef elf_map > > > > > > +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr, > > > + unsigned long size, int prot, int type, unsigned long off) > > > +{ > > > + unsigned long map_addr; > > > + > > > + /* > > > + * If caller requests the mapping at a specific place, make sure we fail > > > + * rather than potentially clobber an existing mapping which can have > > > + * security consequences (e.g. smash over the stack area). > > > + */ > > > + map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off); > > > + if (BAD_ADDR(map_addr)) > > > + return map_addr; > > > + > > > + if ((type & MAP_FIXED) && map_addr != addr) { > > > + pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n", > > > + (void*)addr); > > > + return -EAGAIN; > > > + } > > > + > > > + return map_addr; > > > +} > > > + > > > static unsigned long elf_map(struct file *filep, unsigned long addr, > > > struct elf_phdr *eppnt, int prot, int type, > > > unsigned long total_size) > > > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr, > > > */ > > > if (total_size) { > > > total_size = ELF_PAGEALIGN(total_size); > > > - map_addr = vm_mmap(filep, addr, total_size, prot, type, off); > > > + map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off); > > > if (!BAD_ADDR(map_addr)) > > > vm_munmap(map_addr+size, total_size-size); > > > > Here we will still take the map total, then unmap the rest way. > > > > I am wondering why we don't fix those issues we figured out, but add > > another level of wrapper elf_vm_mmap() to hack it. > > Could you be more specific please? In the current code, obviously the total size for PIE loading is unnecessary since it's MAP_FIXED, and it's initial mapping from ELF_ET_DYN_BASE, I can't see any chance it will fail to map. Though you use a trick in this new elf_vm_mmap() to check if it failed on mapping, in essence it's a MAP_FIXED. And as I said before, then we will still have the ungraceful 'mapping total'|'unmapping the rest' method. If from a new code reader's point of view, the newly added elf_vm_mmap() plus the existing elf_map() may confuse people more. Just personnal opinion. Anyway, your patches are better, my patches have been resting there for several days but no comment. :-) I may still need strength the understanding about elf loading code. Thanks Baoquan > > > So we will have > > elf_map() -> elf_vm_mmap() -> vm_mmap(), not even counting > > vm_mmap_pgoff(), then finally enter into do_mmap_pgoff(), to do the > > maping for elf program. > > I've added another level of helper to keep the code in elf_map saner. It > is quite complex already. > -- > Michal Hocko > SUSE Labs