From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935447AbbCPQQH (ORCPT ); Mon, 16 Mar 2015 12:16:07 -0400 Received: from forward-corp1g.mail.yandex.net ([95.108.253.251]:56513 "EHLO forward-corp1g.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933138AbbCPQQD (ORCPT ); Mon, 16 Mar 2015 12:16:03 -0400 Authentication-Results: smtpcorp1m.mail.yandex.net; dkim=pass header.i=@yandex-team.ru Message-ID: <550701BE.40100@yandex-team.ru> Date: Mon, 16 Mar 2015 19:15:58 +0300 From: Konstantin Khlebnikov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Oleg Nesterov CC: linux-mm@kvack.org, Andrew Morton , linux-kernel@vger.kernel.org, Davidlohr Bueso , Al Viro Subject: Re: [PATCH] mm: rcu-protected get_mm_exe_file() References: <20150316131257.32340.36600.stgit@buzz> <20150316140720.GA1859@redhat.com> In-Reply-To: <20150316140720.GA1859@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16.03.2015 17:07, Oleg Nesterov wrote: > On 03/16, Konstantin Khlebnikov wrote: >> >> +/** >> + * set_mm_exe_file - change a reference to the mm's executable file >> + * >> + * This changes mm's executale file (shown as symlink /proc/[pid]/exe). >> + * >> + * Main users are mmput(), sys_execve() and sys_prctl(PR_SET_MM_MAP/EXE_FILE). >> + * Callers prevent concurrent invocations: in mmput() nobody alive left, >> + * in execve task is single-threaded, prctl holds mmap_sem exclusively. >> + */ >> void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) >> { >> + struct file *old_exe_file = rcu_dereference_protected(mm->exe_file, >> + !atomic_read(&mm->mm_users) || current->in_execve || >> + lock_is_held(&mm->mmap_sem)); >> + >> if (new_exe_file) >> get_file(new_exe_file); >> - if (mm->exe_file) >> - fput(mm->exe_file); >> - mm->exe_file = new_exe_file; >> + rcu_assign_pointer(mm->exe_file, new_exe_file); >> + if (old_exe_file) >> + fput(old_exe_file); >> } > > Yes, I think this is correct, __fput() does call_rcu(file_free_rcu). And > much better than the new lock ;) > > Acked-by: Oleg Nesterov > > > > So I think the patch is fine, but personally I dislike the "prctl holds > mmap_sem exclusively" and rcu_dereference_protected(). > > I mean, I think we can do another cleanup on top of this change. > > 1. set_mm_exe_file() should be called by exit/exec only, so > it should use > > rcu_dereference_protected(mm->exe_file, > atomic_read(&mm->mm_users) <= 1); > > 2. prctl() should not use it, it can do > > get_file(new_exe); > old_exe = xchg(&mm->exe_file); > if (old_exe) > fput(old_exe); I think smp_mb() is required before xchg() or probably this stuff should be hidden inside yet another magic RCU macro ( with two screens of comments =) > > 3. and we can remove down_write(mmap_sem) from prctl paths. > > Actually we can do this even without xchg() above, but we might > want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check. > > What do you think? > > Oleg. > -- Konstantin