From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752593AbdI2N1b (ORCPT ); Fri, 29 Sep 2017 09:27:31 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:37680 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752484AbdI2N11 (ORCPT ); Fri, 29 Sep 2017 09:27:27 -0400 Date: Fri, 29 Sep 2017 06:27:23 -0700 From: Srikar Dronamraju To: Oleg Nesterov Cc: "Naveen N. Rao" , Ingo Molnar , Ananth N Mavinakayanahalli , Anton Blanchard , Michael Ellerman , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/3] kernel/uprobes: Fix check for active uprobe Reply-To: Srikar Dronamraju References: <6718c4ebec1d71524bb81c907180bad90fc8d3c3.1506061547.git.naveen.n.rao@linux.vnet.ibm.com> <20170925161619.GA15325@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20170925161619.GA15325@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-TM-AS-MML: disable x-cbid: 17092913-0008-0000-0000-00000158F6FF X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17092913-0009-0000-0000-0000098EA1B0 Message-Id: <20170929132723.GA2318@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-29_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709290191 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -1751,6 +1751,19 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) > > uprobe = find_uprobe(inode, offset); > > } > > > > + /* Ensure that the breakpoint was actually installed */ > > + if (uprobe) { > > + /* > > + * TODO: move copy_insn/etc into _register and remove > > + * this hack. After we hit the bp, _unregister + > > + * _register can install the new and not-yet-analyzed > > + * uprobe at the same address, restart. > > + */ > > + smp_rmb(); /* pairs with wmb() in prepare_uprobe() */ > > + if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) > > + uprobe = NULL; > > + } > > ACK ... > > but the comment is no longer valid, it only mentions the race unregister + > register. > > And note that "restart" is not true in that we are not going to simply restart, > we will check is_trap_at_addr() and then either send SIGTRAP or restart. > > This is correct because we do this check under mmap_sem so we can't race with > install_breakpoint(), so is_trap_at_addr() == T can't be falsely true if > UPROBE_COPY_INSN is not set. > Right, Given that we are doing this in the mmap_sem, we should also be removing the rmb/wmb pairs too. Right Oleg? > And btw, perhaps you should do this check right after find_uprobe() in the > if (valid_vma) block. > > Oleg. >