Received: by 2002:a25:c593:0:0:0:0:0 with SMTP id v141csp1030611ybe; Wed, 4 Sep 2019 11:25:02 -0700 (PDT) X-Google-Smtp-Source: APXvYqy2ceIHJgj1+G6c4+4SoqsmAbVZcJ3AmIWIr9D/esm1pk0EBTxYSrhQcigcCmXvHPGBJiND X-Received: by 2002:a65:448a:: with SMTP id l10mr35583611pgq.327.1567621501857; Wed, 04 Sep 2019 11:25:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1567621501; cv=none; d=google.com; s=arc-20160816; b=uBMjsimjkzjYhXJthjgvUIcl99hYi3z0x3mi/hLpDOtQe69kAyrk0NPDdTW1Asgp8N b9fl+BZbGEVnPdcPYVA7ZaHv/Wh0chRYVK0cK2GigV+IfRv2HYHPkeq3FdCH9MjvRpdR eIGgOvTcL2B/v1qajwqiy4aB8OWc6NfJ5oEMJMSyvrUmxA3u4TvPIfgIxwN5XCib/lx0 c6l0yUB+m+W9Bl8AKKinQct58bQP5vcKpa5nAE/DbMWMi66AsnhvnveaFQ2PYg/ED0yF uQjXxGZOMV+gFhlqUTMTCcSl5zfjMqqN1eMHBRA+yyw8ZoNst3CmVhS8K0NoscaTXV+T /BBA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature; bh=cGgzhIYlhyUWRd6NlvYi3bxves/TEqWLYRstRgagnVc=; b=t7OaCwn+e2e7DccKGitqyBQAVn22irCzBFzCtzCPz9jQ+UovuIidofMZ2r5hQN/yCL p4Al/ISauODNgdhBtb7YqvXu1EebbaAre7lkU4nIz+IDZlRnULciAq/2MTkjR27htVVD QK5QgfLznt4wY3ZBKhbfAzQOiAqShwWKA3bIFHAARwqyLq8O0ox/hsTGhX3mXgU5eBhp f9vwql64CpsEFB9lzmG0eWNNFQH4XvLZO0cFh/KZclD7dCB9wxbAZ8wBrkx6pjPUTsQv eeypm2GezoA66GEtvpKkJttFGJ2jVfvpVy1JkxDNbTEQ/bVaWoXWCBP6QfiYAvABMSOI wYvA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=oUkp3gRm; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 1si20698001pfe.271.2019.09.04.11.24.46; Wed, 04 Sep 2019 11:25:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=oUkp3gRm; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388903AbfIDSF3 (ORCPT + 99 others); Wed, 4 Sep 2019 14:05:29 -0400 Received: from mail.kernel.org ([198.145.29.99]:46900 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388895AbfIDSF2 (ORCPT ); Wed, 4 Sep 2019 14:05:28 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 076A5206BA; Wed, 4 Sep 2019 18:05:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1567620326; bh=o8v/59dMpD3hbwbXaI/fTlLZKYDcekca9GCmq2L7z8Y=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=oUkp3gRmOdC9yOLhHu2k01J1M2hrGyLu4JdYPZvoPdPmOtxpMJKfBlYNdW8TiEbIE If6LEsCG+OF5u9o3pq0n38eBEKdhaDzUEx/VBwVvhFsIFcedHs2JSmSHGUHzukeL9F QL3zly5BLVr45tRfHHkQrXwOlba3Lu5O168k1T7Y= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Benjamin Herrenschmidt , Alan Stern , Felipe Balbi , Sasha Levin Subject: [PATCH 4.19 17/93] usb: gadget: mass_storage: Fix races between fsg_disable and fsg_set_alt Date: Wed, 4 Sep 2019 19:53:19 +0200 Message-Id: <20190904175304.746167206@linuxfoundation.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190904175302.845828956@linuxfoundation.org> References: <20190904175302.845828956@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [ Upstream commit 4a56a478a525d6427be90753451c40e1327caa1a ] If fsg_disable() and fsg_set_alt() are called too closely to each other (for example due to a quick reset/reconnect), what can happen is that fsg_set_alt sets common->new_fsg from an interrupt while handle_exception is trying to process the config change caused by fsg_disable(): fsg_disable() ... handle_exception() sets state back to FSG_STATE_NORMAL hasn't yet called do_set_interface() or is inside it. ---> interrupt fsg_set_alt sets common->new_fsg queues a new FSG_STATE_CONFIG_CHANGE <--- Now, the first handle_exception can "see" the updated new_fsg, treats it as if it was a fsg_set_alt() response, call usb_composite_setup_continue() etc... But then, the thread sees the second FSG_STATE_CONFIG_CHANGE, and goes back down the same path, wipes and reattaches a now active fsg, and .. calls usb_composite_setup_continue() which at this point is wrong. Not only we get a backtrace, but I suspect the second set_interface wrecks some state causing the host to get upset in my case. This fixes it by replacing "new_fsg" by a "state argument" (same principle) which is set in the same lock section as the state update, and retrieved similarly. That way, there is never any discrepancy between the dequeued state and the observed value of it. We keep the ability to have the latest reconfig operation take precedence, but we guarantee that once "dequeued" the argument (new_fsg) will not be clobbered by any new event. Signed-off-by: Benjamin Herrenschmidt Acked-by: Alan Stern Signed-off-by: Felipe Balbi Signed-off-by: Sasha Levin --- drivers/usb/gadget/function/f_mass_storage.c | 28 +++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 1074cb82ec172..0b7b4d09785b6 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -261,7 +261,7 @@ struct fsg_common; struct fsg_common { struct usb_gadget *gadget; struct usb_composite_dev *cdev; - struct fsg_dev *fsg, *new_fsg; + struct fsg_dev *fsg; wait_queue_head_t io_wait; wait_queue_head_t fsg_wait; @@ -290,6 +290,7 @@ struct fsg_common { unsigned int bulk_out_maxpacket; enum fsg_state state; /* For exception handling */ unsigned int exception_req_tag; + void *exception_arg; enum data_direction data_dir; u32 data_size; @@ -391,7 +392,8 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep) /* These routines may be called in process context or in_irq */ -static void raise_exception(struct fsg_common *common, enum fsg_state new_state) +static void __raise_exception(struct fsg_common *common, enum fsg_state new_state, + void *arg) { unsigned long flags; @@ -404,6 +406,7 @@ static void raise_exception(struct fsg_common *common, enum fsg_state new_state) if (common->state <= new_state) { common->exception_req_tag = common->ep0_req_tag; common->state = new_state; + common->exception_arg = arg; if (common->thread_task) send_sig_info(SIGUSR1, SEND_SIG_FORCED, common->thread_task); @@ -411,6 +414,10 @@ static void raise_exception(struct fsg_common *common, enum fsg_state new_state) spin_unlock_irqrestore(&common->lock, flags); } +static void raise_exception(struct fsg_common *common, enum fsg_state new_state) +{ + __raise_exception(common, new_state, NULL); +} /*-------------------------------------------------------------------------*/ @@ -2285,16 +2292,16 @@ reset: static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt) { struct fsg_dev *fsg = fsg_from_func(f); - fsg->common->new_fsg = fsg; - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); + + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, fsg); return USB_GADGET_DELAYED_STATUS; } static void fsg_disable(struct usb_function *f) { struct fsg_dev *fsg = fsg_from_func(f); - fsg->common->new_fsg = NULL; - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); + + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); } @@ -2307,6 +2314,7 @@ static void handle_exception(struct fsg_common *common) enum fsg_state old_state; struct fsg_lun *curlun; unsigned int exception_req_tag; + struct fsg_dev *new_fsg; /* * Clear the existing signals. Anything but SIGUSR1 is converted @@ -2360,6 +2368,7 @@ static void handle_exception(struct fsg_common *common) common->next_buffhd_to_fill = &common->buffhds[0]; common->next_buffhd_to_drain = &common->buffhds[0]; exception_req_tag = common->exception_req_tag; + new_fsg = common->exception_arg; old_state = common->state; common->state = FSG_STATE_NORMAL; @@ -2413,8 +2422,8 @@ static void handle_exception(struct fsg_common *common) break; case FSG_STATE_CONFIG_CHANGE: - do_set_interface(common, common->new_fsg); - if (common->new_fsg) + do_set_interface(common, new_fsg); + if (new_fsg) usb_composite_setup_continue(common->cdev); break; @@ -2989,8 +2998,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f) DBG(fsg, "unbind\n"); if (fsg->common->fsg == fsg) { - fsg->common->new_fsg = NULL; - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); + __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL); /* FIXME: make interruptible or killable somehow? */ wait_event(common->fsg_wait, common->fsg != fsg); } -- 2.20.1