From patchwork Mon Mar 1 15:07:05 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Johannes Berg X-Patchwork-Id: 1445637 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.infradead.org (client-ip=2001:8b0:10b:1231::1; helo=merlin.infradead.org; envelope-from=linux-um-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; secure) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=merlin.20170209 header.b=hFN15e+5; dkim-atps=neutral Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:8b0:10b:1231::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Dq3XQ43h7z9sR4 for ; Tue, 2 Mar 2021 02:07:34 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-Id:Date: Subject:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=+nbDkNJmJoH0X7U2/atVMd4tJbnUs69h/T1fp7UGvuc=; b=hFN15e+5R4IvDnfNbYznICnoM lSvEdzACkDxNRDplCPjQk9uJz6OPyRzRQgLfkp/Xk8BE6zIYcdvQAHXqgoib8pdqF2S0UeP8ytKU3 km1vmdyMJiIaeIx9QoaVhzIjvhtEAPyM3ueZqElTalLO8kDIfLTaYhURnne1KHElVhkQWzbsueZ4x 0cyTdIXtuz8MCpBhitdHDG4whra/mzHkMXgPQlJsMyKTQoyRZwhvcBgJA/nO50n0HQgPKgaxtS1Id g9KjruA0VTX4eYeH3DFAbXty7eg1FsIu+UW4URavrwFEkLVRqyA6Qctcgcesnd+9ZEoycusrf0vh5 /S4VNhNgg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGk8r-0008DW-Fo; Mon, 01 Mar 2021 15:07:25 +0000 Received: from s3.sipsolutions.net ([2a01:4f8:191:4433::2] helo=sipsolutions.net) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lGk8n-0008Ai-Ah for linux-um@lists.infradead.org; Mon, 01 Mar 2021 15:07:23 +0000 Received: by sipsolutions.net with esmtpsa (TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.94) (envelope-from ) id 1lGk8k-00AJOK-Kt; Mon, 01 Mar 2021 16:07:18 +0100 From: Johannes Berg To: linux-um@lists.infradead.org Subject: [PATCH v2 5/8] um: time-travel/signals: fix ndelay() in interrupt Date: Mon, 1 Mar 2021 16:07:05 +0100 Message-Id: <20210301160501.ccfd4018f832.I3507b456d425cb95a092eb50b6cb491fe8575c50@changeid> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20210301150708.244970-1-johannes@sipsolutions.net> References: <20210301150708.244970-1-johannes@sipsolutions.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210301_100721_502423_2C7DF0F4 X-CRM114-Status: GOOD ( 30.79 ) X-Spam-Score: 0.3 (/) X-Spam-Report: SpamAssassin version 3.4.4 on merlin.infradead.org summary: Content analysis details: (0.3 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 SPF_NONE SPF: sender does not publish an SPF Record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.3 KHOP_HELO_FCRDNS Relay HELO differs from its IP's reverse DNS X-BeenThere: linux-um@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Arnd Bergmann , linux-kernel@vger.kernel.org, Johannes Berg Sender: "linux-um" Errors-To: linux-um-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org From: Johannes Berg We should be able to ndelay() from any context, even from an interrupt context! However, this is broken (not functionally, but locking-wise) in time-travel because we'll get into the time-travel code and enable interrupts to handle messages on other time-travel aware subsystems (only virtio for now). Luckily, I've already reworked the time-travel aware signal (interrupt) delivery for suspend/resume to have a time travel handler, which runs directly in the context of the signal and not from the Linux interrupt. In order to fix this time-travel issue then, we need to do a few things: 1) rework the signal handling code to not block SIGIO (if time-travel is enabled, just to simplify the other case) but rather let it bubble through the system, all the way *past* the IRQ's timetravel_handler, stopping it after that and before real interrupt delivery if IRQs are not actually enabled; 2) rework time-travel to not enable interrupts while it's waiting for a message; 3) rework time-travel to not (just) disable interrupts but rather block signals at a lower level while it needs them disabled for communicating with the controller. Finally, since now we can actually spend even virtual time in interrupts-disabled sections, the delay warning when we deliver a time-travel delayed interrupt is no longer valid, things can (and should) now get delayed. Signed-off-by: Johannes Berg --- v2: - check for signals_enabled as well as irqs_suspended to see if only time-travel handlers should be called - fix race in unblock --- arch/um/include/shared/irq_user.h | 1 + arch/um/include/shared/os.h | 3 +++ arch/um/kernel/irq.c | 42 ++++++++++++++++++++++++----- arch/um/kernel/time.c | 35 +++++++++--------------- arch/um/os-Linux/signal.c | 45 ++++++++++++++++++++++++++++--- 5 files changed, 93 insertions(+), 33 deletions(-) diff --git a/arch/um/include/shared/irq_user.h b/arch/um/include/shared/irq_user.h index 07239e801a5b..065829f443ae 100644 --- a/arch/um/include/shared/irq_user.h +++ b/arch/um/include/shared/irq_user.h @@ -17,6 +17,7 @@ enum um_irq_type { struct siginfo; extern void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs); +void sigio_run_timetravel_handlers(void); extern void free_irq_by_fd(int fd); extern void deactivate_fd(int fd, int irqnum); extern int deactivate_all_fds(void); diff --git a/arch/um/include/shared/os.h b/arch/um/include/shared/os.h index f9fbbddc38bb..453b13369533 100644 --- a/arch/um/include/shared/os.h +++ b/arch/um/include/shared/os.h @@ -242,6 +242,9 @@ extern int set_signals_trace(int enable); extern int os_is_signal_stack(void); extern void deliver_alarm(void); extern void register_pm_wake_signal(void); +extern void block_signals_hard(void); +extern void unblock_signals_hard(void); +extern void mark_sigio_pending(void); /* util.c */ extern void stack_protections(unsigned long address); diff --git a/arch/um/kernel/irq.c b/arch/um/kernel/irq.c index 82af5191e73d..ccf5e4d27202 100644 --- a/arch/um/kernel/irq.c +++ b/arch/um/kernel/irq.c @@ -123,7 +123,8 @@ static bool irq_do_timetravel_handler(struct irq_entry *entry, #endif static void sigio_reg_handler(int idx, struct irq_entry *entry, enum um_irq_type t, - struct uml_pt_regs *regs) + struct uml_pt_regs *regs, + bool timetravel_handlers_only) { struct irq_reg *reg = &entry->reg[t]; @@ -136,18 +137,32 @@ static void sigio_reg_handler(int idx, struct irq_entry *entry, enum um_irq_type if (irq_do_timetravel_handler(entry, t)) return; - if (irqs_suspended) +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT + /* + * We can only get here with signals disabled if we have time-travel + * support, otherwise we cannot have the hard handlers that may need + * to run even in interrupts-disabled sections and therefore sigio is + * blocked as well when interrupts are disabled. + */ + if (!signals_enabled) { + mark_sigio_pending(); + return; + } +#endif + + if (timetravel_handlers_only) return; irq_io_loop(reg, regs); } -void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) +static void _sigio_handler(struct uml_pt_regs *regs, + bool timetravel_handlers_only) { struct irq_entry *irq_entry; int n, i; - if (irqs_suspended && !um_irq_timetravel_handler_used()) + if (timetravel_handlers_only && !um_irq_timetravel_handler_used()) return; while (1) { @@ -172,14 +187,24 @@ void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) irq_entry = os_epoll_get_data_pointer(i); for (t = 0; t < NUM_IRQ_TYPES; t++) - sigio_reg_handler(i, irq_entry, t, regs); + sigio_reg_handler(i, irq_entry, t, regs, + timetravel_handlers_only); } } - if (!irqs_suspended) + if (!timetravel_handlers_only) free_irqs(); } +void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) +{ + _sigio_handler(regs, irqs_suspended +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT + || !signals_enabled +#endif + ); +} + static struct irq_entry *get_irq_entry_by_fd(int fd) { struct irq_entry *walk; @@ -467,6 +492,11 @@ int um_request_irq_tt(int irq, int fd, enum um_irq_type type, devname, dev_id, timetravel_handler); } EXPORT_SYMBOL(um_request_irq_tt); + +void sigio_run_timetravel_handlers(void) +{ + _sigio_handler(NULL, true); +} #endif #ifdef CONFIG_PM_SLEEP diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c index e0cdb9694fb8..fddd1dec27e6 100644 --- a/arch/um/kernel/time.c +++ b/arch/um/kernel/time.c @@ -68,23 +68,15 @@ static void time_travel_handle_message(struct um_timetravel_msg *msg, int ret; /* - * Poll outside the locked section (if we're not called to only read - * the response) so we can get interrupts for e.g. virtio while we're - * here, but then we need to lock to not get interrupted between the - * read of the message and write of the ACK. + * We can't unlock here, but interrupt signals with a timetravel_handler + * (see um_request_irq_tt) get to the timetravel_handler anyway. */ if (mode != TTMH_READ) { - bool disabled = irqs_disabled(); + BUG_ON(mode == TTMH_IDLE && !irqs_disabled()); - BUG_ON(mode == TTMH_IDLE && !disabled); - - if (disabled) - local_irq_enable(); while (os_poll(1, &time_travel_ext_fd) != 0) { /* nothing */ } - if (disabled) - local_irq_disable(); } ret = os_read_file(time_travel_ext_fd, msg, sizeof(*msg)); @@ -123,15 +115,15 @@ static u64 time_travel_ext_req(u32 op, u64 time) .time = time, .seq = mseq, }; - unsigned long flags; /* - * We need to save interrupts here and only restore when we - * got the ACK - otherwise we can get interrupted and send - * another request while we're still waiting for an ACK, but - * the peer doesn't know we got interrupted and will send - * the ACKs in the same order as the message, but we'd need - * to see them in the opposite order ... + * We need to block even the timetravel handlers of SIGIO here and + * only restore their use when we got the ACK - otherwise we may + * (will) get interrupted by that, try to queue the IRQ for future + * processing and thus send another request while we're still waiting + * for an ACK, but the peer doesn't know we got interrupted and will + * send the ACKs in the same order as the message, but we'd need to + * see them in the opposite order ... * * This wouldn't matter *too* much, but some ACKs carry the * current time (for UM_TIMETRAVEL_GET) and getting another @@ -140,7 +132,7 @@ static u64 time_travel_ext_req(u32 op, u64 time) * The sequence number assignment that happens here lets us * debug such message handling issues more easily. */ - local_irq_save(flags); + block_signals_hard(); os_write_file(time_travel_ext_fd, &msg, sizeof(msg)); while (msg.op != UM_TIMETRAVEL_ACK) @@ -152,7 +144,7 @@ static u64 time_travel_ext_req(u32 op, u64 time) if (op == UM_TIMETRAVEL_GET) time_travel_set_time(msg.time); - local_irq_restore(flags); + unblock_signals_hard(); return msg.time; } @@ -352,9 +344,6 @@ void deliver_time_travel_irqs(void) while ((e = list_first_entry_or_null(&time_travel_irqs, struct time_travel_event, list))) { - WARN(e->time != time_travel_time, - "time moved from %lld to %lld before IRQ delivery\n", - time_travel_time, e->time); list_del(&e->list); e->pending = false; e->fn(e); diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c index 8c9d162e6c51..8bce743fbe64 100644 --- a/arch/um/os-Linux/signal.c +++ b/arch/um/os-Linux/signal.c @@ -63,15 +63,19 @@ static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc) #define SIGALRM_MASK (1 << SIGALRM_BIT) int signals_enabled; +#ifdef UML_CONFIG_UML_TIME_TRAVEL_SUPPORT +static int signals_blocked; +#else +#define signals_blocked (!signals_enabled) +#endif static unsigned int signals_pending; static unsigned int signals_active = 0; void sig_handler(int sig, struct siginfo *si, mcontext_t *mc) { - int enabled; + int enabled = signals_enabled; - enabled = signals_enabled; - if (!enabled && (sig == SIGIO)) { + if (signals_blocked && (sig == SIGIO)) { signals_pending |= SIGIO_MASK; return; } @@ -99,7 +103,7 @@ void timer_alarm_handler(int sig, struct siginfo *unused_si, mcontext_t *mc) int enabled; enabled = signals_enabled; - if (!signals_enabled) { + if (!signals_enabled || signals_blocked) { signals_pending |= SIGALRM_MASK; return; } @@ -363,6 +367,39 @@ int set_signals_trace(int enable) return ret; } +#ifdef UML_CONFIG_UML_TIME_TRAVEL_SUPPORT +void mark_sigio_pending(void) +{ + signals_pending |= SIGIO_MASK; +} + +void block_signals_hard(void) +{ + if (signals_blocked) + return; + signals_blocked = 1; + barrier(); +} + +void unblock_signals_hard(void) +{ + if (!signals_blocked) + return; + /* Must be set to 0 before we check the pending bits etc. */ + signals_blocked = 0; + barrier(); + + if (signals_pending && signals_enabled) { + /* this is a bit inefficient, but that's not really important */ + block_signals(); + unblock_signals(); + } else if (signals_pending & SIGIO_MASK) { + /* we need to run time-travel handlers even if not enabled */ + sigio_run_timetravel_handlers(); + } +} +#endif + int os_is_signal_stack(void) { stack_t ss;