Received-SPF: pass (google.com: domain of linux-kernel+bounces-108669-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Guoyong Wang <guoyong.wang@mediatek.com>
To: "Jason A . Donenfeld" <Jason@zx2c4.com>, Theodore Ts'o <tytso@mit.edu>,
	Tejun Heo <tj@kernel.org>, Lai Jiangshan <jiangshanlai@gmail.com>, "Matthias
 Brugger" <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>
CC: <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>, <wsd_upstream@mediatek.com>, "Guoyong
 Wang" <guoyong.wang@mediatek.com>
Subject: Re: [PATCH] random: Fix the issue of '_might_sleep' function running in an atomic contex
Date: Wed, 20 Mar 2024 17:02:42 +0800
Message-ID: <20240320090242.10318-1-guoyong.wang@mediatek.com>
In-Reply-To: <Zfo3QUFEy4wHkLEB@zx2c4.com>
References: <Zfo3QUFEy4wHkLEB@zx2c4.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Web, 20 Mar 2024 02:09:21 +0100, Jason A. Donenfeld wrote:
>> Hi Jason,
>> 
>> Thanks for your suggestions. 
>> 
>> I am inclined to accept your second suggestion. My reluctance to accept 
>> the first is due to the concern that "&& !in_atomic()" could potentially 
>> alter the original meaning of the 'execute_in_process_context' interface. 
>> Regarding the third suggestion, modifying the logic associated with 'input' 
>> is not recommended.
> 
> Doesn't something like the below seem simplest? Just move the call out
> of the spinlock and we're done.
> 
> diff --git a/drivers/input/input-core-private.h b/drivers/input/input-core-private.h
> index 116834cf8868..717f239e28d0 100644
> --- a/drivers/input/input-core-private.h
> +++ b/drivers/input/input-core-private.h
> @@ -10,7 +10,7 @@
>  struct input_dev;
> 
>  void input_mt_release_slots(struct input_dev *dev);
> -void input_handle_event(struct input_dev *dev,
> +bool input_handle_event(struct input_dev *dev,
>  unsigned int type, unsigned int code, int value);
> 
>  #endif /* _INPUT_CORE_PRIVATE_H */
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index f71ea4fb173f..2faf46218c66 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -391,20 +391,22 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
>  }
>  }
> 
> -void input_handle_event(struct input_dev *dev,
> +bool input_handle_event(struct input_dev *dev,
>  unsigned int type, unsigned int code, int value)
>  {
>  int disposition;
> +bool should_contribute_to_rng = false;
> 
>  lockdep_assert_held(&dev->event_lock);
> 
>  disposition = input_get_disposition(dev, type, code, &value);
>  if (disposition != INPUT_IGNORE_EVENT) {
>  if (type != EV_SYN)
> -add_input_randomness(type, code, value);
> +should_contribute_to_rng = true;
> 
>  input_event_dispose(dev, disposition, type, code, value);
>  }
> +return should_contribute_to_rng;
>  }
> 
>  /**
> @@ -428,12 +430,15 @@ void input_event(struct input_dev *dev,
>   unsigned int type, unsigned int code, int value)
>  {
>  unsigned long flags;
> +bool should_contribute_to_rng;
> 
>  if (is_event_supported(type, dev->evbit, EV_MAX)) {
> 
>  spin_lock_irqsave(&dev->event_lock, flags);
> -input_handle_event(dev, type, code, value);
> +should_contribute_to_rng = input_handle_event(dev, type, code, value);
>  spin_unlock_irqrestore(&dev->event_lock, flags);
> +if (should_contribute_to_rng)
> +add_input_randomness(type, code, value);
>  }
>  }
>  EXPORT_SYMBOL(input_event);

Hi Jason,

Your proposal is not suitable for scenarios where input_event is called within an atomic context.

For example:
spin_lock(&dev->spinlock);
input_event(dev, EV_ABS, ABS_X, x);
spin_unlock(&dev->spinlock);