Message ID | 20240227113426.253232-1-herve.codina@bootlin.com |
---|---|
Headers |
Return-Path: <linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp2635112dyb; Tue, 27 Feb 2024 03:35:25 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCVUpRTtfOpR92U40E9gAK0573JGFuUfemCFbixdABc9ZjiIADdis0pClzvcMJjpu3A7OH4Sso61u8IrKulGLbLXjel5XQ== X-Google-Smtp-Source: AGHT+IH8sRzJt4+ONbc0hGdrdcUFSuyFPJUszpxBe8RaLwiYReuGPkbtlFNc35l5nvGH7vURspMk X-Received: by 2002:a0c:da88:0:b0:68f:2a5e:4fa5 with SMTP id z8-20020a0cda88000000b0068f2a5e4fa5mr1545997qvj.11.1709033725257; Tue, 27 Feb 2024 03:35:25 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709033725; cv=pass; d=google.com; s=arc-20160816; b=SIPyD+ERqduOCV9B84Nd5NBbSzDx+TNOWvlnd7GJLbkiCjEiHgCbyyCToyIOIgzT/8 U5rLQg5QUdBLQd2kSL8/hCjy0x5KXUQ1REo5uHqForL3V3X4LN3iVpzcUHPsmEeR34ZN Y2hBK+71BdD2igAlupPIzMoK9X69MWPIP2hi8yMyBpEOjXJ7wAnNAo8i/vT4ZpPFVoLV wFub+uLeE6U1erdJSorL8V2grmdXMh1CYZsC44jWwbP93CynTN5M4upFLRLU3qChGSQs RcjH1Ko4olk/ewGd5djMxG7m2+B+lI89NbcyrBBTUDtVkFAeCOpN4zW81e1gwKwd5ba5 p32A== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=QhfBBpBWQgyBYkE4IR+OyuoUGnqKUWopm8WIsqzfqQc=; fh=ONUZ3m01L0DVQm+Cjotv2dXDuBgIcWiaHC6/g+ZSH6Y=; b=F0H0lKVKFlL/bCqHGX8oP3uc5Qv7heAVydk4Ibq9rWUhGCD4HI+wlDFAJN3HQAdY+E Fc7ETolXORgqtJr1En8EqLKl9D3oml8ibpHMUMKLr496P/8QFxrEz4DL8qyn34ZJ3r3b aR9sKBaB9am8jhfYkNC10I2y7hreust2ev96jRhVzpn2QZ9PvtSefNDAR4HDLP0L5Cha FGtpU8SIE/hB1PCuimLBza4Cxy5I4rTZNZXYCKVZM+MO+5tBoS9nZbCOPJrrIAEtNdOE mZm9d054CLZ4YEYXnQoyqm0g/lGM2Yd2cRx4beVW6mq3A4LqXxfZmV5b2WVH98kvE2Ny UK7A==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=Rvney9wi; arc=pass (i=1 spf=pass spfdomain=bootlin.com dkim=pass dkdomain=bootlin.com dmarc=pass fromdomain=bootlin.com); spf=pass (google.com: domain of linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id r12-20020a056214212c00b0068f3ecd0f6csi7207154qvc.104.2024.02.27.03.35.25 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Feb 2024 03:35:25 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=Rvney9wi; arc=pass (i=1 spf=pass spfdomain=bootlin.com dkim=pass dkdomain=bootlin.com dmarc=pass fromdomain=bootlin.com); spf=pass (google.com: domain of linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-83130-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.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 ny.mirrors.kernel.org (Postfix) with ESMTPS id 9AAA61C22991 for <ouuuleilei@gmail.com>; Tue, 27 Feb 2024 11:35:13 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id A41C613A884; Tue, 27 Feb 2024 11:34:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="Rvney9wi" Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) (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 A007555E63; Tue, 27 Feb 2024 11:34:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.70.183.197 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709033682; cv=none; b=HccO9634FUWqHGePI75+Dl5fKIFUDsG1fHdiBwdVGFZ4qqvnQWVqAiLwnCuKv+dIIG35b5YXW+SnScWXY2fjqjV/dXrNQkOJyGTDPvUsDZm/zL3fhc0iv1mvHlqHNrN+gdOWxHAsbI6LyANgYTgRqGNgQKaHU0hgqqoLYht4NwA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709033682; c=relaxed/simple; bh=Pl6RzP7eVBM1SX/LSO8FN5EW7VXMT3LIJ50I1lG39SI=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-Type; b=sYH2ttn9WJoxTCkThHOSTzfBa8F0OtbDTSy+AwLGGjkxYnuZPV42UH5BABPblL2UFNF0zmwjy31iM3rrSJ/qpanp7hqR5JJpKFQU4C86SpXJLVGQGdhGR4PJfdN9s+mYquniZabuvffJ9CNVkdFRnbXxCg0HSQRBK2CQF8e0BQY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=Rvney9wi; arc=none smtp.client-ip=217.70.183.197 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Received: by mail.gandi.net (Postfix) with ESMTPA id 3641F1C0004; Tue, 27 Feb 2024 11:34:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1709033672; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=QhfBBpBWQgyBYkE4IR+OyuoUGnqKUWopm8WIsqzfqQc=; b=Rvney9wiUF3QWebEBIP7qQyiOEdbTsvbj6jXvyFjQ8bOFtWX5pwGMFIuLdr+0MDFdq+5H5 HAbm9mBPeLjAH/7YozPDy+6IehJINT0b9dF02C7FqjIhM4asE8h+RUHfN06OoWSg9pymY7 NZQP+GfI/5PrSA2GWEq6Yy6teb24vJTOOyXl8XbgS5I4B2gKKLlNHr/Vl8rHnGBt1897An O0Jxiu0TcJTYcCtuzQ33fT8IctA3ux8fELGixbtdtzdmKuy/0WR83tipvBHvwMjEBYfFOH gPc4katisPQdZmH06sJ6Tnj9+vJqpyWjoiL+wSfvpAOTYUPB0BgIiNj5TjA9lA== From: Herve Codina <herve.codina@bootlin.com> To: Bartosz Golaszewski <brgl@bgdev.pl>, Kent Gibson <warthog618@gmail.com>, Linus Walleij <linus.walleij@linaro.org> Cc: Saravana Kannan <saravanak@google.com>, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Luca Ceresoli <luca.ceresoli@bootlin.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Herve Codina <herve.codina@bootlin.com> Subject: [PATCH v2 0/2] gpio-cdev: Release IRQ used by gpio-cdev on gpio chip removal Date: Tue, 27 Feb 2024 12:34:23 +0100 Message-ID: <20240227113426.253232-1-herve.codina@bootlin.com> X-Mailer: git-send-email 2.43.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Sasl: herve.codina@bootlin.com X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792051747279758556 X-GMAIL-MSGID: 1792051747279758556 |
Series |
gpio-cdev: Release IRQ used by gpio-cdev on gpio chip removal
|
|
Message
Herve Codina
Feb. 27, 2024, 11:34 a.m. UTC
Hi, When a gpio chip device is removed while some related gpio are used by the user-space (gpiomon for instance), the following warning can appear: remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c ... Call trace: remove_proc_entry+0x190/0x19c unregister_irq_proc+0xd0/0x104 free_desc+0x4c/0xc4 irq_free_descs+0x6c/0x90 irq_dispose_mapping+0x104/0x14c gpiochip_irqchip_remove+0xcc/0x1a4 gpiochip_remove+0x48/0x100 ... Indeed, even if the gpio removal is notified to the gpio-cdev, the IRQ used is not released when it should be. This series calls the gpio removal notifier sooner in the removal process in order to give a chance to a notifier function to release the IRQ before releasing the IRQ mapping and adds the needed operations to release the IRQ in the gpio cdev notifier function. Compared to the previous iteration: https://lore.kernel.org/linux-kernel/20240220111019.133697-1-herve.codina@bootlin.com/ this v2 series set gdev->chip to NULL before calling gcdev_unregister(). Also, this v2 series was rebased on top of for-next branch of the GPIO tree. Best regards, Hervé Codina Changes v1 -> v2: - Patch 1 Set gdev->chip to NULL before calling gcdev_unregister() - Patch 2 No changes Herve Codina (2): gpiolib: call gcdev_unregister() sooner in the removal operations gpiolib: cdev: release IRQs when the gpio chip device is removed drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- drivers/gpio/gpiolib.c | 6 +++++- 2 files changed, 27 insertions(+), 12 deletions(-)
Comments
On Tue, Feb 27, 2024 at 12:34 PM Herve Codina <herve.codina@bootlincom> wrote: > > Hi, > > When a gpio chip device is removed while some related gpio are used by > the user-space (gpiomon for instance), the following warning can appear: > remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' > WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c > ... > Call trace: > remove_proc_entry+0x190/0x19c > unregister_irq_proc+0xd0/0x104 > free_desc+0x4c/0xc4 > irq_free_descs+0x6c/0x90 > irq_dispose_mapping+0x104/0x14c > gpiochip_irqchip_remove+0xcc/0x1a4 > gpiochip_remove+0x48/0x100 > ... > > Indeed, even if the gpio removal is notified to the gpio-cdev, the > IRQ used is not released when it should be. > > This series calls the gpio removal notifier sooner in the removal > process in order to give a chance to a notifier function to release > the IRQ before releasing the IRQ mapping and adds the needed > operations to release the IRQ in the gpio cdev notifier function. > > Compared to the previous iteration: > https://lore.kernel.org/linux-kernel/20240220111019.133697-1-herve.codina@bootlin.com/ > this v2 series set gdev->chip to NULL before calling gcdev_unregister(). > > Also, this v2 series was rebased on top of for-next branch of the GPIO > tree. > > Best regards, > Hervé Codina > > Changes v1 -> v2: > - Patch 1 > Set gdev->chip to NULL before calling gcdev_unregister() > > - Patch 2 > No changes > > Herve Codina (2): > gpiolib: call gcdev_unregister() sooner in the removal operations > gpiolib: cdev: release IRQs when the gpio chip device is removed > > drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- > drivers/gpio/gpiolib.c | 6 +++++- > 2 files changed, 27 insertions(+), 12 deletions(-) > > -- > 2.43.0 > Sorry but this is just papering over the real issue. I'd say NAK for now as I'd really prefer to get to the root of the problem and fix it for all GPIO interrupt users. Kent, Linus: what do you think? Bart
On Tue, Feb 27, 2024 at 08:31:33PM +0100, Bartosz Golaszewski wrote: > On Tue, Feb 27, 2024 at 12:34 PM Herve Codina <herve.codina@bootlin.com> wrote: > > > > Hi, > > > > When a gpio chip device is removed while some related gpio are used by > > the user-space (gpiomon for instance), the following warning can appear: > > remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon' > > WARNING: CPU: 2 PID: 72 at fs/proc/generic.c:717 remove_proc_entry+0x190/0x19c > > ... > > Call trace: > > remove_proc_entry+0x190/0x19c > > unregister_irq_proc+0xd0/0x104 > > free_desc+0x4c/0xc4 > > irq_free_descs+0x6c/0x90 > > irq_dispose_mapping+0x104/0x14c > > gpiochip_irqchip_remove+0xcc/0x1a4 > > gpiochip_remove+0x48/0x100 > > ... > > > > Indeed, even if the gpio removal is notified to the gpio-cdev, the > > IRQ used is not released when it should be. > > > > This series calls the gpio removal notifier sooner in the removal > > process in order to give a chance to a notifier function to release > > the IRQ before releasing the IRQ mapping and adds the needed > > operations to release the IRQ in the gpio cdev notifier function. > > > > Compared to the previous iteration: > > https://lore.kernel.org/linux-kernel/20240220111019.133697-1-herve.codina@bootlin.com/ > > this v2 series set gdev->chip to NULL before calling gcdev_unregister(). > > > > Also, this v2 series was rebased on top of for-next branch of the GPIO > > tree. > > > > Best regards, > > Hervé Codina > > > > Changes v1 -> v2: > > - Patch 1 > > Set gdev->chip to NULL before calling gcdev_unregister() > > > > - Patch 2 > > No changes > > > > Herve Codina (2): > > gpiolib: call gcdev_unregister() sooner in the removal operations > > gpiolib: cdev: release IRQs when the gpio chip device is removed > > > > drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++----------- > > drivers/gpio/gpiolib.c | 6 +++++- > > 2 files changed, 27 insertions(+), 12 deletions(-) > > > > -- > > 2.43.0 > > > > Sorry but this is just papering over the real issue. I'd say NAK for > now as I'd really prefer to get to the root of the problem and fix it > for all GPIO interrupt users. > > Kent, Linus: what do you think? > Agreed - a broader solution makes sense to me. Cheers, Kent.
On Tue, Feb 27, 2024 at 8:31 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Feb 27, 2024 at 12:34 PM Herve Codina <herve.codina@bootlin.com> wrote: > > Herve Codina (2): > > gpiolib: call gcdev_unregister() sooner in the removal operations > > gpiolib: cdev: release IRQs when the gpio chip device is removed (...) > Sorry but this is just papering over the real issue. I'd say NAK for > now as I'd really prefer to get to the root of the problem and fix it > for all GPIO interrupt users. > > Kent, Linus: what do you think? I'm not sure. What does "all GPIO interrupt users" mean in this context? If you mean "also the kernel-internal" (such as some random driver having performed gpiod_to_irq() and requested it or, taken it from a phandle in the device tree) then I think these are slightly semantically different. The big difference is that users of the cdev are *expected* to *crash* sometimes, releasing the file handle and then this cleanup needs to happen. Also cdev is more likely to be used for hotplugged/unplugged GPIOs. The kernel-internal users are *not* expected to crash, but to clean up their usage the right way. Also they are predominantly if not exclusively used for fixed GPIOs such as those on an SoC that do not hot-unplug and go away randomly. Use case 1: you run gpio-mon on a random GPIO with IRQ on a board. It is using a SoC-native GPIO. Suddenly gpio-mon crashes because of OOM or whatever and releases the filehandle on the way down. What to do? Use case 2: you plug in a USB dongle with GPIOs on. Start gpio-mon on one of the pins. Unplug the dongle. Then it is fair that the cdev cleans up the irq, because I don't see any way that a kernel driver would request any of these GPIOs (but I'm more uncertain here). I just think it is necessary to think about the big picture here. Yours, Linus Walleij
On Thu, Feb 29, 2024 at 3:09 PM Linus Walleij <linus.walleij@linaroorg> wrote: > > On Tue, Feb 27, 2024 at 8:31 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Feb 27, 2024 at 12:34 PM Herve Codina <herve.codina@bootlin.com> wrote: > > > > Herve Codina (2): > > > gpiolib: call gcdev_unregister() sooner in the removal operations > > > gpiolib: cdev: release IRQs when the gpio chip device is removed > (...) > > Sorry but this is just papering over the real issue. I'd say NAK for > > now as I'd really prefer to get to the root of the problem and fix it > > for all GPIO interrupt users. > > > > Kent, Linus: what do you think? > > I'm not sure. What does "all GPIO interrupt users" mean in this context? > > If you mean "also the kernel-internal" (such as some random driver > having performed gpiod_to_irq() and requested it or, taken it from a > phandle in the device tree) then I think these are slightly semantically > different. > Yes I mean both in-kernel and user-space consumers. > The big difference is that users of the cdev are *expected* to *crash* > sometimes, releasing the file handle and then this cleanup needs to > happen. Also cdev is more likely to be used for hotplugged/unplugged > GPIOs. > > The kernel-internal users are *not* expected to crash, but to clean up > their usage the right way. Also they are predominantly if not exclusively > used for fixed GPIOs such as those on an SoC that do not hot-unplug > and go away randomly. > > Use case 1: you run gpio-mon on a random GPIO with IRQ on a board. > It is using a SoC-native GPIO. Suddenly gpio-mon crashes because > of OOM or whatever and releases the filehandle on the way down. > What to do? > > Use case 2: you plug in a USB dongle with GPIOs on. Start gpio-mon > on one of the pins. Unplug the dongle. Then it is fair that the cdev cleans > up the irq, because I don't see any way that a kernel driver would > request any of these GPIOs (but I'm more uncertain here). > > I just think it is necessary to think about the big picture here. > Agreed and the big picture - just like with the reason behind the SRCU rework - is the fact that even static GPIO chips defined in ACPI or DT can be unbound. Unless you want to make the decision that we arbitrarily suppress_bind_attrs for all GPIO chips which I don't think you do. I have shown in the discussion under the previous iteration that a static GPIO chip defined in DT that is also marked as an interrupt-controller may have interrupts requested directly from its irq domain bypassing the .to_irq() callback. As long as this GPIO chip may be unbound (and we do not restrict this) it means the splat mentioned here can be triggered from user-space with a simple rmmod because a requested irq does not increase the module reference count nor do device links seem to work for interrupts not associated with a struct device explicitly. I DO want to fix it, don't get me wrong. I don't want to just leave it like this, especially since we've made so much progress with hotpluggability recently. I just don't believe this is the right fix, I will try to come up with a solution that addresses the issue globally. Bart > Yours, > Linus Walleij [1] https://lore.kernel.org/lkml/CAMRc=Mf5fRWoOMsJ41vzvE=-vp3wi-Obw=j5fBk3DuQaZNQP2Q@mail.gmail.com/
On Fri, Mar 1, 2024 at 8:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > Agreed and the big picture - just like with the reason behind the SRCU > rework - is the fact that even static GPIO chips defined in ACPI or DT > can be unbound. Unless you want to make the decision that we > arbitrarily suppress_bind_attrs for all GPIO chips which I don't think > you do. > > I have shown in the discussion under the previous iteration that a > static GPIO chip defined in DT that is also marked as an > interrupt-controller may have interrupts requested directly from its > irq domain bypassing the .to_irq() callback. As long as this GPIO chip > may be unbound (and we do not restrict this) it means the splat > mentioned here can be triggered from user-space with a simple rmmod > because a requested irq does not increase the module reference count > nor do device links seem to work for interrupts not associated with a > struct device explicitly. > > I DO want to fix it, don't get me wrong. I don't want to just leave it > like this, especially since we've made so much progress with > hotpluggability recently. I just don't believe this is the right fix, > I will try to come up with a solution that addresses the issue > globally. OK I trust you to come up with something better for sure! With regards to the ability to unbind/rebind drivers from sysfs, true. I have heard about that as a counterargument to many things. The problem is that I have never heard about a user unbinding/binding a driver from sysfs for anything but debugging a drivers ability to bind/unbind. Partly I feel that thing should just be moved to debugfs given the usecase and because it just looks like a way for attackers to provoke crashes given how some drivers look. Yours, Linus Walleij
On Fri, Mar 1, 2024 at 9:15 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Mar 1, 2024 at 8:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > Agreed and the big picture - just like with the reason behind the SRCU > > rework - is the fact that even static GPIO chips defined in ACPI or DT > > can be unbound. Unless you want to make the decision that we > > arbitrarily suppress_bind_attrs for all GPIO chips which I don't think > > you do. > > > > I have shown in the discussion under the previous iteration that a > > static GPIO chip defined in DT that is also marked as an > > interrupt-controller may have interrupts requested directly from its > > irq domain bypassing the .to_irq() callback. As long as this GPIO chip > > may be unbound (and we do not restrict this) it means the splat > > mentioned here can be triggered from user-space with a simple rmmod > > because a requested irq does not increase the module reference count > > nor do device links seem to work for interrupts not associated with a > > struct device explicitly. > > > > I DO want to fix it, don't get me wrong. I don't want to just leave it > > like this, especially since we've made so much progress with > > hotpluggability recently. I just don't believe this is the right fix, > > I will try to come up with a solution that addresses the issue > > globally. > > OK I trust you to come up with something better for sure! > > With regards to the ability to unbind/rebind drivers from sysfs, true. > I have heard about that as a counterargument to many things. > > The problem is that I have never heard about a user unbinding/binding > a driver from sysfs for anything but debugging a drivers ability to > bind/unbind. Partly I feel that thing should just be moved > to debugfs given the usecase and because it just looks like a way for > attackers to provoke crashes given how some drivers look. > > Yours, > Linus Walleij That's not the only thing - device unbind can also be triggered by removing the module providing the driver which is a completely normal operation for user-space. Bart
On Sat, Mar 2, 2024 at 10:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Fri, Mar 1, 2024 at 9:15 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > The problem is that I have never heard about a user unbinding/binding > > a driver from sysfs for anything but debugging a drivers ability to > > bind/unbind. Partly I feel that thing should just be moved > > to debugfs given the usecase and because it just looks like a way for > > attackers to provoke crashes given how some drivers look. > > That's not the only thing - device unbind can also be triggered by > removing the module providing the driver which is a completely normal > operation for user-space. That is one thing, but every time I hear about "have you thought about users going in and using bind/unbind in sysfs" it's for builtin bool drivers. I almost feel like bool drivers should have bind/unbind disabled by default :/ The introduction of deferred probe has made the situation much better because it tends to exercise the bind/unbind path a bit. Yours, Linus Walleij