Received-SPF: pass (google.com: domain of linux-kernel+bounces-73181-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 20 Feb 2024 22:29:59 +0800
From: Kent Gibson <warthog618@gmail.com>
To: Herve Codina <herve.codina@bootlin.com>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Luca Ceresoli <luca.ceresoli@bootlin.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 2/2] gpiolib: cdev: release IRQs when the gpio chip
 device is removed
Message-ID: <20240220142959.GA244726@rigel>
References: <20240220111019.133697-1-herve.codina@bootlin.com>
 <20240220111019.133697-3-herve.codina@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240220111019.133697-3-herve.codina@bootlin.com>

On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote:
> When gpio chip device is removed while some related gpio are used by the
> user-space, 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, the gpio cdev uses an IRQ but this IRQ is not released when the
> gpio chip device is removed.
>
> Release IRQs used in the device removal notifier functions.
> Also move one of these function definition in order to avoid a forward
> declaration (move after the edge_detector_stop() definition).
>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 2a88736629ef..aec4a4c8490a 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -688,17 +688,6 @@ static void line_set_debounce_period(struct line *line,
>  	 GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \
>  	 GPIO_V2_LINE_EDGE_FLAGS)
>
> -static int linereq_unregistered_notify(struct notifier_block *nb,
> -				       unsigned long action, void *data)
> -{
> -	struct linereq *lr = container_of(nb, struct linereq,
> -					  device_unregistered_nb);
> -
> -	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
> -
> -	return NOTIFY_OK;
> -}
> -
>  static void linereq_put_event(struct linereq *lr,
>  			      struct gpio_v2_line_event *le)
>  {
> @@ -1189,6 +1178,23 @@ static int edge_detector_update(struct line *line,
>  	return edge_detector_setup(line, lc, line_idx, edflags);
>  }
>
> +static int linereq_unregistered_notify(struct notifier_block *nb,
> +				       unsigned long action, void *data)
> +{
> +	struct linereq *lr = container_of(nb, struct linereq,
> +					  device_unregistered_nb);
> +	int i;
> +
> +	for (i = 0; i < lr->num_lines; i++) {
> +		if (lr->lines[i].desc)
> +			edge_detector_stop(&lr->lines[i]);
> +	}
> +

Firstly, the re-ordering in the previous patch creates a race,
as the NULLing of the gdev->chip serves to numb the cdev ioctls, so
there is now a window between the notifier being called and that numbing,
during which userspace may call linereq_set_config() and re-request
the irq.

There is also a race here with linereq_set_config().  That can be prevented
by holding the lr->config_mutex - assuming the notifier is not being called
from atomic context.

You also have a race with the line being freed that could pull the
lr out from under you, so a use after free problem.
I'd rather live with the warning :(.
Fixing that requires rethinking the lifecycle management for the
linereq/lineevent.

Cheers,
Kent.

> +	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
>  				     unsigned int line_idx)
>  {
> @@ -1898,6 +1904,11 @@ static int lineevent_unregistered_notify(struct notifier_block *nb,
>  	struct lineevent_state *le = container_of(nb, struct lineevent_state,
>  						  device_unregistered_nb);
>
> +	if (le->irq) {
> +		free_irq(le->irq, le);
> +		le->irq = 0;
> +	}
> +
>  	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
>
>  	return NOTIFY_OK;
> --
> 2.43.0
>