netdev - Re: [PATCH net-next 3/6] net: Add control functions for irq suspension

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+Ryar9QPL+PCw8P4Q9Wy8U1S1+q1J+_V4E0qYu3cLnUQ@mail.gmail.com>
Date: Fri, 23 Aug 2024 20:14:18 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Joe Damato <jdamato@...tly.com>
Cc: netdev@...r.kernel.org, amritha.nambiar@...el.com, 
	sridhar.samudrala@...el.com, sdf@...ichev.me, peter@...eblog.net, 
	m2shafiei@...terloo.ca, bjorn@...osinc.com, hch@...radead.org, 
	willy@...radead.org, willemdebruijn.kernel@...il.com, skhawaja@...gle.com, 
	kuba@...nel.org, Martin Karsten <mkarsten@...terloo.ca>, 
	"David S. Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, Jiri Pirko <jiri@...nulli.us>, 
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Lorenzo Bianconi <lorenzo@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 3/6] net: Add control functions for irq suspension

On Fri, Aug 23, 2024 at 7:31 PM Joe Damato <jdamato@...tly.com> wrote:
>
> From: Martin Karsten <mkarsten@...terloo.ca>
>
> The napi_suspend_irqs routine bootstraps irq suspension by elongating
> the defer timeout to irq_suspend_timeout.
>
> The napi_resume_irqs routine effectly cancels irq suspension by forcing
> the napi to be scheduled immediately.
>
> Signed-off-by: Martin Karsten <mkarsten@...terloo.ca>
> Co-developed-by: Joe Damato <jdamato@...tly.com>
> Signed-off-by: Joe Damato <jdamato@...tly.com>
> Tested-by: Joe Damato <jdamato@...tly.com>
> Tested-by: Martin Karsten <mkarsten@...terloo.ca>
> ---
>  include/net/busy_poll.h |  3 +++
>  net/core/dev.c          | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
> index 9b09acac538e..f095b2bdeee1 100644
> --- a/include/net/busy_poll.h
> +++ b/include/net/busy_poll.h
> @@ -52,6 +52,9 @@ void napi_busy_loop_rcu(unsigned int napi_id,
>                         bool (*loop_end)(void *, unsigned long),
>                         void *loop_end_arg, bool prefer_busy_poll, u16 budget);
>
> +void napi_suspend_irqs(unsigned int napi_id);
> +void napi_resume_irqs(unsigned int napi_id);
> +
>  #else /* CONFIG_NET_RX_BUSY_POLL */
>  static inline unsigned long net_busy_loop_on(void)
>  {
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 74060ba866d4..4de0dfc86e21 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6507,6 +6507,39 @@ void napi_busy_loop(unsigned int napi_id,
>  }
>  EXPORT_SYMBOL(napi_busy_loop);
>
> +void napi_suspend_irqs(unsigned int napi_id)
> +{
> +       struct napi_struct *napi;
> +
> +       rcu_read_lock();
> +       napi = napi_by_id(napi_id);
> +       if (napi) {
> +               unsigned long timeout = READ_ONCE(napi->dev->irq_suspend_timeout);
> +
> +               if (timeout)
> +                       hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
> +       }
> +       rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(napi_suspend_irqs);
> +
> +void napi_resume_irqs(unsigned int napi_id)
> +{
> +       struct napi_struct *napi;
> +
> +       rcu_read_lock();
> +       napi = napi_by_id(napi_id);
> +       if (napi) {
> +               if (READ_ONCE(napi->dev->irq_suspend_timeout)) {


Since we'll read irq_suspend_timeout twice, we could have a situation
where the napi_schedule() will not be done
if another thread changes irq_suspend_timeout ?

If this is fine, a comment would be nice :)

The thing is that the kernel can not trust the user (think of syzbot)

> +                       local_bh_disable();
> +                       napi_schedule(napi);
> +                       local_bh_enable();
> +               }
> +       }
> +       rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(napi_resume_irqs);
> +
>  #endif /* CONFIG_NET_RX_BUSY_POLL */
>
>  static void napi_hash_add(struct napi_struct *napi)
> --
> 2.25.1
>