From patchwork Fri Mar 5 12:19:56 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Johannes Berg X-Patchwork-Id: 1447807 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:1:d65d:64ff:fe57:4e05; helo=desiato.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=desiato.20200630 header.b=QbUahxlT; dkim-atps=neutral Received: from desiato.infradead.org (desiato.infradead.org [IPv6:2001:8b0:10b:1:d65d:64ff:fe57:4e05]) (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 4DsRfB15m5z9sWP for ; Fri, 5 Mar 2021 23:20:49 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-Id:Date: Subject:Cc:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=1VgCvU21EmlqIJaVFS4pI5Zq32/ihOFb7MXEdRPvA7Q=; b=QbUahxlT07du3zvl7/tyjP6YK xnc22fXfdtkpGE9oG+Ce7sZ4lMriG1cqbofPv9bbc7A4uetutmTD3di6krOpP5bxhbYaP0PQr76b7 2ZIpMbKdOpisIN8YZn9MU3R8847DETvPoLFhNda6cVoLh1WdzvBIgjTYdxrHPSeu7XJ1wmkBCQEUs ycLNWCJMPl4dGAjqYTmsi5NY7hkr5DSiR0l46Tthgjkw9hhqkwKHJeeOa/wiBGXhbQrB8hMhh4fvr OpfroXZafSHi46nVSvjBUMQAP/+iVytAyHEQDoabshVRrkPmZ4MfQxnmGJ8lU60crSn960NnJxcee LQG8wNcVA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lI9Re-00Ewrn-9Q; Fri, 05 Mar 2021 12:20:38 +0000 Received: from s3.sipsolutions.net ([2a01:4f8:191:4433::2] helo=sipsolutions.net) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lI9RW-00Ewnb-3v for linux-um@lists.infradead.org; Fri, 05 Mar 2021 12:20:32 +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 1lI9RV-00C3YV-Fc; Fri, 05 Mar 2021 13:20:29 +0100 From: Johannes Berg To: linux-um@lists.infradead.org Cc: Johannes Berg Subject: [PATCH v4 6/9] um: time-travel/signals: fix ndelay() in interrupt Date: Fri, 5 Mar 2021 13:19:56 +0100 Message-Id: <20210305131934.863e58ef45ea.I3507b456d425cb95a092eb50b6cb491fe8575c50@changeid> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20210305122010.442577-1-johannes@sipsolutions.net> References: <20210305122010.442577-1-johannes@sipsolutions.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210305_122030_230305_AA7F0471 X-CRM114-Status: GOOD ( 34.04 ) X-Spam-Score: 0.4 (/) X-Spam-Report: Spam detection software, running on the system "desiato.infradead.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: 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 [...] Content analysis details: (0.4 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.0 SPF_NONE SPF: sender does not publish an SPF Record 0.4 KHOP_HELO_FCRDNS Relay HELO differs from its IP's reverse DNS X-BeenThere: linux-um@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 call time-travel handlers (only) if interrupts are disabled but signals aren't blocked, instead of marking it only pending there. This is needed to not deadlock other communication. 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 v3: - rework the delivery-while-irqs-disabled completely to fix time-travel!=ext modes and to simplify the code --- arch/um/include/shared/irq_user.h | 1 + arch/um/include/shared/os.h | 3 ++ arch/um/kernel/irq.c | 35 ++++++++++++++++---- arch/um/kernel/time.c | 35 +++++++------------- arch/um/os-Linux/signal.c | 55 ++++++++++++++++++++++++++++--- 5 files changed, 96 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..1c448ea4729e 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,29 @@ 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) + /* + * If we're called to only run time-travel handlers then don't + * actually proceed but mark sigio as pending (if applicable). + * For suspend/resume, timetravel_handlers_only may be true + * despite time-travel not being configured and used. + */ + if (timetravel_handlers_only) { +#ifdef CONFIG_UML_TIME_TRAVEL_SUPPORT + mark_sigio_pending(); +#endif 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 +184,20 @@ 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); +} + static struct irq_entry *get_irq_entry_by_fd(int fd) { struct irq_entry *walk; @@ -467,6 +485,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..1d501acb22ee 100644 --- a/arch/um/os-Linux/signal.c +++ b/arch/um/os-Linux/signal.c @@ -18,6 +18,7 @@ #include #include #include +#include void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *) = { [SIGTRAP] = relay_signal, @@ -63,16 +64,29 @@ 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 false +#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)) { - signals_pending |= SIGIO_MASK; + if ((signals_blocked || !enabled) && (sig == SIGIO)) { + /* + * In TT_MODE_EXTERNAL, need to still call time-travel + * handlers unless signals are also blocked for the + * external time message processing. This will mark + * signals_pending by itself (only if necessary.) + */ + if (!signals_blocked && time_travel_mode == TT_MODE_EXTERNAL) + sigio_run_timetravel_handlers(); + else + signals_pending |= SIGIO_MASK; return; } @@ -363,6 +377,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;