From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752383AbdK1MzZ (ORCPT ); Tue, 28 Nov 2017 07:55:25 -0500 Received: from mail.kernel.org ([198.145.29.99]:56390 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751254AbdK1MzY (ORCPT ); Tue, 28 Nov 2017 07:55:24 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CFB6221894 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Tue, 28 Nov 2017 21:55:20 +0900 From: Masami Hiramatsu To: JianKang Chen Cc: , , , , , Subject: Re: [PATCH] kernel/kprobes: add re-register safe check for register_kretprobe() Message-Id: <20171128215520.21846e0b2243f6654c9a63f0@kernel.org> In-Reply-To: <1511785525-2894-1-git-send-email-chenjiankang1@huawei.com> References: <1511785525-2894-1-git-send-email-chenjiankang1@huawei.com> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 27 Nov 2017 20:25:25 +0800 JianKang Chen wrote: > From: Chen Jiankang > > When there are two same struct kretprobe rp, the INIT_HLIST_HEAD() > will result in a empty list table rp->free_instances. The memory leak > will happen. So it needs to add re-register safe check by > __get_valid_kprobe(). > > However, current this is not safe for multi-threadings, because > there is still a chance to re-register kretprobe concurrently. > So I add a kretprobe_mutex lock to protect the INIT_LIST_HEAD; > > And we use rcu read lock to protect the rcu list for __get_valid_kprobe > > Signed-off-by: Chen Jiankang > --- > kernel/kprobes.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index a1606a4..6e4e657 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -67,6 +67,7 @@ > > /* This protects kprobe_table and optimizing_list */ > static DEFINE_MUTEX(kprobe_mutex); > +static DEFINE_MUTEX(kretprobe_mutex); > static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL; > static struct { > raw_spinlock_t lock ____cacheline_aligned_in_smp; > @@ -1947,6 +1948,13 @@ int register_kretprobe(struct kretprobe *rp) > rp->maxactive = num_possible_cpus(); > #endif > } > + > + mutex_lock(&kretprobe_mutex); > + rcu_read_lock(); > + if (__get_valid_kprobe(&rp->kp)) { > + ret = -EINVAL; > + goto out; > + } > raw_spin_lock_init(&rp->lock); You can not use rcu_read_lock() as this way, since below block including mutex_lock() (means it may sleep) and also it updates rcu list. So, instead, I meant mutex_lock(&kretprobe_mutex); rcu_read_lock(); kp = __get_valid_kprobe(&rp->kp); rcu_read_unlock(); if (kp) { ret = -EINVAL; goto out; } Which is enough, because we only take care the case that same "kretprobe" is reused, which is protected by kretprobe_mutex. rcu_read_lock() is only for accessing kprobe_table. Thank you, > INIT_HLIST_HEAD(&rp->free_instances); > for (i = 0; i < rp->maxactive; i++) { > @@ -1954,7 +1962,8 @@ int register_kretprobe(struct kretprobe *rp) > rp->data_size, GFP_KERNEL); > if (inst == NULL) { > free_rp_inst(rp); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out; > } > INIT_HLIST_NODE(&inst->hlist); > hlist_add_head(&inst->hlist, &rp->free_instances); > @@ -1965,6 +1974,9 @@ int register_kretprobe(struct kretprobe *rp) > ret = register_kprobe(&rp->kp); > if (ret != 0) > free_rp_inst(rp); > +out: > + rcu_read_unlock(); > + mutex_unlock(&kretprobe_mutex); > return ret; > } > EXPORT_SYMBOL_GPL(register_kretprobe); > -- > 1.7.12.4 > -- Masami Hiramatsu