| 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
| ||
|
Message-ID: <20201119161108.GA1798712@swahl-home.5wahls.com>
Date: Thu, 19 Nov 2020 10:11:37 -0600
From: Steve Wahl <steve.wahl@....com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
Arnd Bergmann <arnd@...db.de>,
Robin Holt <robinmholt@...il.com>,
Steve Wahl <steve.wahl@....com>,
Dimitri Sivanich <dimitri.sivanich@....com>,
Russ Anderson <russ.anderson@....com>
Subject: Re: [PATCH v2] misc/sgi-xp: Replace in_interrupt() usage
Reviewed-by: Steve Wahl <steve.wahl@....com>
On Thu, Nov 19, 2020 at 11:31:51AM +0100, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
>
> The usage of in_interrupt() in xpc_partition_disengaged() is clearly
> intended to avoid canceling the timeout timer when the function is invoked
> from the timer callback.
>
> While in_interrupt() is deprecated and ill defined as it does not provide
> what the name suggests it catches the intended case.
>
> Add an argument to xpc_partition_disengaged() which is true if called
> from timer and otherwise false.
> Use del_timer_sync() instead of del_singleshot_timer_sync() which is the
> same thing.
>
> Note: This does not prevent reentrancy into the function as the function
> has no concurrency control and timer callback and regular task context
> callers can happen concurrently on different CPUs or the timer can
> interrupt the task context before it is able to cancel it.
>
> While the only driver which is providing the arch_xpc_ops callbacks
> (xpc_uv) seems not to have a reentrancy problem and the only negative
> effect would be a double dev_info() entry in dmesg, the whole mechanism is
> conceptually broken.
>
> But that's not subject of this cleanup endeavour and left as an exercise to
> the folks who might have interest to make that code fully correct.
>
> [bigeasy: Add the argument, use del_timer_sync().]
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Cliff Whickman <cpw@....com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Robin Holt <robinmholt@...il.com>
> Cc: Steve Wahl <steve.wahl@....com>
> Cc: Dimitri Sivanich <dimitri.sivanich@....com>
> Cc: Russ Anderson <russ.anderson@....com>
> ---
> v1…v2: Use xpc_partition_disengaged_from_timer() and don't export the
> argument. Suggested by Greg.
>
> drivers/misc/sgi-xp/xpc.h | 1 +
> drivers/misc/sgi-xp/xpc_main.c | 2 +-
> drivers/misc/sgi-xp/xpc_partition.c | 20 +++++++++++++++-----
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/sgi-xp/xpc.h b/drivers/misc/sgi-xp/xpc.h
> index 71db60edff655..225f2bb84e39b 100644
> --- a/drivers/misc/sgi-xp/xpc.h
> +++ b/drivers/misc/sgi-xp/xpc.h
> @@ -634,6 +634,7 @@ extern int xpc_setup_rsvd_page(void);
> extern void xpc_teardown_rsvd_page(void);
> extern int xpc_identify_activate_IRQ_sender(void);
> extern int xpc_partition_disengaged(struct xpc_partition *);
> +extern int xpc_partition_disengaged_from_timer(struct xpc_partition *part);
> extern enum xp_retval xpc_mark_partition_active(struct xpc_partition *);
> extern void xpc_mark_partition_inactive(struct xpc_partition *);
> extern void xpc_discovery(void);
> diff --git a/drivers/misc/sgi-xp/xpc_main.c b/drivers/misc/sgi-xp/xpc_main.c
> index e5244fc1dab30..84610bbcc1314 100644
> --- a/drivers/misc/sgi-xp/xpc_main.c
> +++ b/drivers/misc/sgi-xp/xpc_main.c
> @@ -179,7 +179,7 @@ xpc_timeout_partition_disengage(struct timer_list *t)
>
> DBUG_ON(time_is_after_jiffies(part->disengage_timeout));
>
> - (void)xpc_partition_disengaged(part);
> + xpc_partition_disengaged_from_timer(part);
>
> DBUG_ON(part->disengage_timeout != 0);
> DBUG_ON(xpc_arch_ops.partition_engaged(XPC_PARTID(part)));
> diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c
> index 57df06820bae2..1999d02923dee 100644
> --- a/drivers/misc/sgi-xp/xpc_partition.c
> +++ b/drivers/misc/sgi-xp/xpc_partition.c
> @@ -262,8 +262,8 @@ xpc_get_remote_rp(int nasid, unsigned long *discovered_nasids,
> * from us. Though we requested the remote partition to deactivate with regard
> * to us, we really only need to wait for the other side to disengage from us.
> */
> -int
> -xpc_partition_disengaged(struct xpc_partition *part)
> +static int __xpc_partition_disengaged(struct xpc_partition *part,
> + bool from_timer)
> {
> short partid = XPC_PARTID(part);
> int disengaged;
> @@ -289,9 +289,9 @@ xpc_partition_disengaged(struct xpc_partition *part)
> }
> part->disengage_timeout = 0;
>
> - /* cancel the timer function, provided it's not us */
> - if (!in_interrupt())
> - del_singleshot_timer_sync(&part->disengage_timer);
> + /* Cancel the timer function if not called from it */
> + if (!from_timer)
> + del_timer_sync(&part->disengage_timer);
>
> DBUG_ON(part->act_state != XPC_P_AS_DEACTIVATING &&
> part->act_state != XPC_P_AS_INACTIVE);
> @@ -303,6 +303,16 @@ xpc_partition_disengaged(struct xpc_partition *part)
> return disengaged;
> }
>
> +int xpc_partition_disengaged(struct xpc_partition *part)
> +{
> + return __xpc_partition_disengaged(part, false);
> +}
> +
> +int xpc_partition_disengaged_from_timer(struct xpc_partition *part)
> +{
> + return __xpc_partition_disengaged(part, true);
> +}
> +
> /*
> * Mark specified partition as active.
> */
> --
> 2.29.2
>
--
Steve Wahl, Hewlett Packard Enterprise
Powered by blists - more mailing lists