From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fout-a3-smtp.messagingengine.com (fout-a3-smtp.messagingengine.com [103.168.172.146]) (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 4969D16F8E9 for ; Mon, 27 Jan 2025 08:29:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.146 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737966571; cv=none; b=m+ultrtO6vpWmwFGdreG15/Z1bPgIawriAlqHjzHfUSEj+gd0bczqS2Ptaq46PTlCr/spNLa1bPe1IqCqEeoVQbJxkRhae3Lw7olOCrJuY7XiV3MTXM//DqHi3PJ6Wq1Y9vAusHnKP2CP7bsx53piDJGwg36nsVuX30mGBASJ/M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737966571; c=relaxed/simple; bh=0tJhZNZpgQK/WMIlfFeFHlFtI0PEdDy6zUe+IvHnIJA=; h=MIME-Version:Date:From:To:Cc:Message-Id:In-Reply-To:References: Subject:Content-Type; b=fAxENNJoSfXVK9K2Sxi4CaldMo2KfFIh9X5KkAdx0OOftbPLOxYHnTuiQQ0SmzRoYqYb249/1eu50haca2Ad/ubaTRhH44p3Nz1GGTuu7pGDLCRlLEdVDITYjIpexFO8FssEyoaqeScP3g9AliFxxSiHA3aVm60t6UCgqujoNH8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arndb.de; spf=pass smtp.mailfrom=arndb.de; dkim=pass (2048-bit key) header.d=arndb.de header.i=@arndb.de header.b=UqjmEIg5; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=SQw5la5h; arc=none smtp.client-ip=103.168.172.146 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arndb.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arndb.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=arndb.de header.i=@arndb.de header.b="UqjmEIg5"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="SQw5la5h" Received: from phl-compute-10.internal (phl-compute-10.phl.internal [10.202.2.50]) by mailfout.phl.internal (Postfix) with ESMTP id 2E0911380B20; Mon, 27 Jan 2025 03:29:26 -0500 (EST) Received: from phl-imap-11 ([10.202.2.101]) by phl-compute-10.internal (MEProxy); Mon, 27 Jan 2025 03:29:26 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=arndb.de; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm2; t=1737966566; x=1738052966; bh=NCyg47o8YHGsdwMp2YInZbB0I93pqLNBXc+s7fm4okE=; b= UqjmEIg5/ViYVjSgLpc2OQGUZ9XiXlN5YsWgVXnpasDuzHxR2dZAyHc5uuU/BWRy OWTuBNnsIFUL+kFHPrDYCz7Ywgf5qr0Rt9j+z2fjChSn1dmxoPlGVFkJk3enwA4L pOuOAD7LpQs2mPvl37XLCt/lWkdA3P+EXxF95vOd94PBkuxWdB+Gok6psZzaC4Km rXZxHLODyqyXBEJ1S/s4UDu53RZnLQ8UHyJfndUn/EJD4mvVauZhBMnXySgooikn mQES9NLuFnz14UdUhjgQmGn6fTZFHG5YJF7D7Nr5XYqiFddSelx1+DBRh/Yc0Osn hdVIWHppG7Fde1BjmZtCZA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1737966566; x= 1738052966; bh=NCyg47o8YHGsdwMp2YInZbB0I93pqLNBXc+s7fm4okE=; b=S Qw5la5hbDxch3qhuIILJcHKHFAeC86Yq+gbEfyNWAatLkg2g/pMXPvgiOj7Z8h0E y9fZ1V5qGSXrsYziXYg6J2fuCmrJ6v+RTTgoawdonADDltJ5ue65MbuFrm6lrwIF niS9PzqXpFxBzGqCN3F9GsrV7c9pGaKvVaMWoTtflf8LQ/6DY1nRqYkCz1yoe+Kz 35goYg9vUkjxDeN2OTgVus52rLYsFkXP6qUhxeM5Y7XDmuWw5oZ2uuGQanHmpXQm 1FnCJ3/tq+LVSAI5fODSzZALPxS2RfHZ0ULIIAwbrI+on3HeqGxtxhZg+18yiTkV 2G2q/vxYnogXGP0VtyeEA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrudejgedguddvieelucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggv pdfurfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpih gvnhhtshculddquddttddmnecujfgurhepofggfffhvfevkfgjfhfutgfgsehtqhertder tdejnecuhfhrohhmpedftehrnhguuceuvghrghhmrghnnhdfuceorghrnhgusegrrhhnug gsrdguvgeqnecuggftrfgrthhtvghrnhepvdfhvdekueduveffffetgfdvveefvdelhedv vdegjedvfeehtdeggeevheefleejnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrg hmpehmrghilhhfrhhomheprghrnhgusegrrhhnuggsrdguvgdpnhgspghrtghpthhtohep hedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohepjhgvnhhsrdifihhklhgrnhguvg hrsehlihhnrghrohdrohhrghdprhgtphhtthhopehjvghrohhmvgdrfhhorhhishhsihgv rheslhhinhgrrhhordhorhhgpdhrtghpthhtohepshhumhhithdrghgrrhhgsehlihhnrg hrohdrohhrghdprhgtphhtthhopehophdqthgvvgeslhhishhtshdrthhruhhsthgvughf ihhrmhifrghrvgdrohhrghdprhgtphhtthhopehlihhnuhigqdhkvghrnhgvlhesvhhgvg hrrdhkvghrnhgvlhdrohhrgh X-ME-Proxy: Feedback-ID: i56a14606:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id CC0BB2220073; Mon, 27 Jan 2025 03:29:24 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Mon, 27 Jan 2025 09:29:04 +0100 From: "Arnd Bergmann" To: "Jens Wiklander" , "Sumit Garg" Cc: op-tee@lists.trustedfirmware.org, "Jerome Forissier" , linux-kernel@vger.kernel.org Message-Id: In-Reply-To: References: <20241213111453.367031-1-sumit.garg@linaro.org> Subject: Re: [PATCH] tee: optee: Add support for supplicant timeout Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Mon, Jan 27, 2025, at 08:33, Jens Wiklander wrote: > [+Arnd for a question below] > > On Wed, Jan 22, 2025 at 1:25=E2=80=AFPM Sumit Garg wrote: >> >> On Wed, 22 Jan 2025 at 15:36, Jens Wiklander wrote: >> > >> > On Wed, Jan 22, 2025 at 10:15=E2=80=AFAM Sumit Garg wrote: >> > > >> > > On Mon, 20 Jan 2025 at 19:01, Jens Wiklander wrote: >> > > > >> > > > On Mon, Jan 20, 2025 at 10:24=E2=80=AFAM Sumit Garg wrote: >> > > > > >> > > > > Hi Jens, >> > > > > >> > > > > On Fri, 13 Dec 2024 at 16:45, Sumit Garg wrote: >> > > > > > >> > > > > > OP-TEE supplicant is a user-space daemon and it's possible = for it >> > > > > > being crashed or killed in the middle of processing an OP-T= EE RPC call. >> > > > > > It becomes more complicated when there is incorrect shutdow= n ordering >> > > > > > of the supplicant process vs the OP-TEE client application = which can >> > > > > > eventually lead to system hang-up waiting for the closure o= f the client >> > > > > > application. >> > > > > > >> > > > > > In order to gracefully handle this scenario, let's add a lo= ng enough >> > > > > > timeout to wait for supplicant to process requests. In case= there is a >> > > > > > timeout then we return a proper error code for the RPC requ= est. >> > > > > > >> > > > > > Signed-off-by: Sumit Garg >> > > > > > --- >> > > > > > drivers/tee/optee/supp.c | 58 +++++++++++++++++++++++++---= ------------ >> > > > > > 1 file changed, 36 insertions(+), 22 deletions(-) >> > > > > > >> > > > > >> > > > > Do you have any further comments here? Or is it fine for you = to pick it up? >> > > > >> > > > I don't like the timeout, it's a bit too hacky. >> > > > >> > > >> > > Can you please elaborate here as to why? >> > >> > Tee-supplicant is supposed to respond in a timely manner. What is >> > timely manner depends on the use case, but you now want to say that >> > it's 10 seconds regardless of what's going on. This makes it >> > impossible to debug tee-supplicant with a debugger unless you're ve= ry >> > quick. It might also introduce random timouts in a system under a >> > heavy IO load. >> >> Although, initially I thought 10 seconds should be enough for any >> user-space process to be considered hung but then I saw >> DEFAULT_HUNG_TASK_TIMEOUT which is 120 seconds for a task to be >> considered hung. How about rather a Kconfig option like >> OPTEE_SUPPLICANT_HUNG_TIMEOUT which defaults to 120 seconds? It can be >> configured as 0 to disable timeout entirely for debugging purposes. > > Adding a timeout when a timeout isn't needed seems wrong, even if the > timeout is very long. Arnd, what do you think? It's hard to put an upper bound on user space latency. As far as I can tell, even that DEFAULT_HUNG_TASK_TIMEOUT limit is only for tasks that are in an unkillable state for more than two minutes, but the supplicant not providing results to the kernel could also happen when it waits in a killable or interruptible state, or when it does multiple I/Os in a row that each block for a time under 120 seconds. A single sector write to an eMMC can easily take multiple seconds by itself when nothing is going on and the device is in need of garbage collection. If the system is already in low memory or there are other tasks writing to the file system, you can have many such I/O operations queued up in the device when it adds another I/O to the back of the queue. Looking at the function that Sumit suggested changing, I see another problem, both before and after the patch: while (wait_for_completion_interruptible(&req->c)) { mutex_lock(&supp->mutex); interruptable =3D !supp->ctx; if (interruptable) { ... } mutex_unlock(&supp->mutex); if (interruptable) { req->ret =3D TEEC_ERROR_COMMUNICATION; break; } } The "_interruptible()" wait makes no sense here if the "interruptable" variable is unset: The task remains in interrupted state, so the while() loop that was waiting for the wake_up_interruptible() turns into a busy loop if the task actually gets a signal. If the task at this point is running at a realtime priority, it would prevent the thing it is waiting for from getting scheduled on the same CPU. >> > > So do you have a better suggestion to fix this in the mainline as= well >> > > as backported to stable releases? >> > >> > Let's start by finding out what problem you're trying to fix. >> >> Let me know if the Kconfig option as proposed above sounds reasonable= to you. > > No one is going to bother with that config option, recompiling the > kernel to be able to debug tee-supplicant is a bit much. If a timeout is needed, having it runtime configurable seems more useful than a build time option. Arnd