Received: by 2002:a05:7412:e794:b0:fa:551:50a7 with SMTP id o20csp81135rdd; Mon, 8 Jan 2024 18:53:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IHpZSxCjQUipFAgDaybuDBdKaWyU/YLiRNJLhxNT/R1YRDzTJuOafRQLsAGMQLPh4h+k4Ki X-Received: by 2002:a05:651c:a0a:b0:2cc:ce6d:5ade with SMTP id k10-20020a05651c0a0a00b002ccce6d5ademr2167970ljq.84.1704768792214; Mon, 08 Jan 2024 18:53:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704768792; cv=none; d=google.com; s=arc-20160816; b=cCUeysNjwUfJqsMGWAgRIobWFZp2xFHnamClXZSwCb9SO+MEe4BvwmfupWHk6vnQQH ZRLYiGD8VZG/ad56RJr9PJiH9PRfUDyn1hUtjOI+fOxPYO7aPwsDsgYH51G7TgF7Q8hD n8jTsEYhVngBQkQLLzUdrPBLJelhH3nund93Xv+c5NEb5sVttgxTHpdytgAVgglYKfbW imRvuy/f6GGVd3IefcXsEs5nNljaSzuIJddkrD/63PHuQMXqhI5472M0mKZP6pGgn6mk jxR3GCsDaiviCPf0xaWbzmsnhf8AcGUcN0fLYlS4m5NnU76MgmrIMNkiQEXsUg4QDirT 6n8A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:cc:user-agent:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:date:message-id:dkim-signature; bh=zYRpt2bBaj2r5gV3nxStVdMjLaemUAJltZhpsRYkhsM=; fh=uzznn7J+IYAjVbPHItJO5zuzOqyiZAy1PaNe6S3d3bk=; b=mWACAUddtichz7hOBVRC0aUAZPrrd6TbcOWSnbAa6fQhsqEn3MT/sqHWAnuZ4QgnoB qEV7bYbcegm8NLSBNC95qXkNh8FsXweCN0TqxhRknAVtb2HyABEMEH05MrTsz/+WZM3y WYQa9OwtO5eYtlB702LkqfYpp+VSun8/rPT3xnmpd1D3LBM+Lt3jNCLGc+uLzcw1o5hz iFwgHsYUDjtpt8DapoKO7Y03u5EgbVM00Xw/OTYOXgvEPBuUFgp6YGCHLpuuRVc9oCQB 6GZjC4dcjlgurtz7+v7hMmwfynbn2iSCNJV6FgIuwq1NnXbRSlVJIFtzZGL3GxzZdJNu ejJQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=jjdx1tjv; spf=pass (google.com: domain of linux-kernel+bounces-20299-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-20299-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id t26-20020a50ab5a000000b0055678085e1bsi400369edc.187.2024.01.08.18.53.12 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 18:53:12 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-20299-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=jjdx1tjv; spf=pass (google.com: domain of linux-kernel+bounces-20299-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-20299-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id EA7FA1F23BFF for ; Tue, 9 Jan 2024 02:53:11 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 357D533F6; Tue, 9 Jan 2024 02:53:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="jjdx1tjv" Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 56DED5390; Tue, 9 Jan 2024 02:52:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1704768778; x=1736304778; h=message-id:date:mime-version:cc:subject:to:references: from:in-reply-to:content-transfer-encoding; bh=UArfAUbovcI7fB2bfs6Ng4TVNlajHZ5AT7kwEtscyUg=; b=jjdx1tjvfHfTH+kkGtl6BsNAydiNm1thk1R3ntqMRuJvrI0xW0PDl/Yc lqrNnzcY/xvv3Dl3Ykccqw4xPSwOLliomTroBANablAUCjVid9Rzw4/Wf 4uoL1JdlN8WynpxZ3FL6HN+5qQrCrRdE155tSUwLgLc3cZIyxYCRAp58F 5SXrbxmq13/8xpkkQsyPtrU0JkRLlmcF2f+76gfeYD3h35I/b9VY8DrLt vjMYHBnXok3eTVSRdeLpzd09lhZUiDmWhGCtS72ejBtvxnNumUMmk2QDl fXNsvA1kitQJHyCYCmVOO7PLDTomWuFPTLkjiUxDn018a8Ci3D1BM6A8Q A==; X-IronPort-AV: E=McAfee;i="6600,9927,10947"; a="4844584" X-IronPort-AV: E=Sophos;i="6.04,181,1695711600"; d="scan'208";a="4844584" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmvoesa103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jan 2024 18:52:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10947"; a="905002419" X-IronPort-AV: E=Sophos;i="6.04,181,1695711600"; d="scan'208";a="905002419" Received: from allen-box.sh.intel.com (HELO [10.239.159.127]) ([10.239.159.127]) by orsmga004.jf.intel.com with ESMTP; 08 Jan 2024 18:52:53 -0800 Message-ID: <8071d34f-e247-41b4-bd50-f68e58d84902@linux.intel.com> Date: Tue, 9 Jan 2024 10:47:40 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: baolu.lu@linux.intel.com, Joerg Roedel , Will Deacon , Robin Murphy , Kevin Tian , Jean-Philippe Brucker , Nicolin Chen , Yi Liu , Jacob Pan , Longfang Liu , Yan Zhao , iommu@lists.linux.dev, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v9 12/14] iommu: Use refcount for fault data access To: Jason Gunthorpe References: <20231220012332.168188-1-baolu.lu@linux.intel.com> <20231220012332.168188-13-baolu.lu@linux.intel.com> <20240105160913.GG50608@ziepe.ca> Content-Language: en-US From: Baolu Lu In-Reply-To: <20240105160913.GG50608@ziepe.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/6/24 12:09 AM, Jason Gunthorpe wrote: > On Wed, Dec 20, 2023 at 09:23:30AM +0800, Lu Baolu wrote: >> The per-device fault data structure stores information about faults >> occurring on a device. Its lifetime spans from IOPF enablement to >> disablement. Multiple paths, including IOPF reporting, handling, and >> responding, may access it concurrently. >> >> Previously, a mutex protected the fault data from use after free. But >> this is not performance friendly due to the critical nature of IOPF >> handling paths. >> >> Refine this with a refcount-based approach. The fault data pointer is >> obtained within an RCU read region with a refcount. The fault data >> pointer is returned for usage only when the pointer is valid and a >> refcount is successfully obtained. The fault data is freed with >> kfree_rcu(), ensuring data is only freed after all RCU critical regions >> complete. >> >> An iopf handling work starts once an iopf group is created. The handling >> work continues until iommu_page_response() is called to respond to the >> iopf and the iopf group is freed. During this time, the device fault >> parameter should always be available. Add a pointer to the device fault >> parameter in the iopf_group structure and hold the reference until the >> iopf_group is freed. >> >> Make iommu_page_response() static as it is only used in io-pgfault.c. >> >> Co-developed-by: Jason Gunthorpe >> Signed-off-by: Jason Gunthorpe >> Signed-off-by: Lu Baolu >> Tested-by: Yan Zhao >> --- >> include/linux/iommu.h | 17 +++-- >> drivers/iommu/io-pgfault.c | 129 +++++++++++++++++++++++-------------- >> drivers/iommu/iommu-sva.c | 2 +- >> 3 files changed, 90 insertions(+), 58 deletions(-) > > This looks basically Ok > >> +/* Caller must hold a reference of the fault parameter. */ >> +static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param) >> +{ >> + if (refcount_dec_and_test(&fault_param->users)) >> + kfree_rcu(fault_param, rcu); >> +} > > [..] > >> @@ -402,10 +429,12 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev) >> INIT_LIST_HEAD(&fault_param->faults); >> INIT_LIST_HEAD(&fault_param->partial); >> fault_param->dev = dev; >> + refcount_set(&fault_param->users, 1); >> + init_rcu_head(&fault_param->rcu); > > No need to do init_rcu_head() when only using it for calling > kfree_rcu() Removed. > >> @@ -454,8 +485,10 @@ int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev) >> list_for_each_entry_safe(iopf, next, &fault_param->partial, list) >> kfree(iopf); >> >> - param->fault_param = NULL; >> - kfree(fault_param); >> + /* dec the ref owned by iopf_queue_add_device() */ >> + rcu_assign_pointer(param->fault_param, NULL); >> + if (refcount_dec_and_test(&fault_param->users)) >> + kfree_rcu(fault_param, rcu); > > Why open code iopf_put_dev_fault_param()? Just call it. Done. > > With those: > > Reviewed-by: Jason Gunthorpe > > Jason Thank you very much! Best regards, baolu