This message generated a parse failure. Raw output follows here. Please use 'back' to navigate. From devnull@lkml.org Thu Apr 25 21:59:39 2024 >From mailfetcher Fri Apr 16 17:28:05 2021 Envelope-to: lkml@grols.ch Delivery-date: Fri, 16 Apr 2021 17:27:45 +0200 Received: from stout.grols.ch [195.201.141.146] by 72459556e3a9 with IMAP (fetchmail-6.3.26) for (single-drop); Fri, 16 Apr 2021 17:28:05 +0200 (CEST) Received: from vger.kernel.org ([23.128.96.18]) by stout.grols.ch with esmtp (Exim 4.89) (envelope-from ) id 1lXQNk-000553-IL for lkml@grols.ch; Fri, 16 Apr 2021 17:27:45 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236307AbhDPP2A convert rfc822-to-8bit (ORCPT ); Fri, 16 Apr 2021 11:28:00 -0400 Received: from netrider.rowland.org ([192.131.102.5]:48407 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S236062AbhDPP2A (ORCPT ); Fri, 16 Apr 2021 11:28:00 -04 Received: (qmail 43914 invoked by uid 1000); 16 Apr 2021 11:27:34 -0400 Date: Fri, 16 Apr 2021 11:27:34 -0400 From: Alan Stern To: Anirudh Rayabharam Cc: Dmitry Vyukov , syzbot , Andrey Konovalov , Felipe Balbi , Dan Carpenter , Subject: Re: [syzbot] general protection fault in gadget_setup Message-Id: <20210416152734.GB42403@rowland.harvard.edu> References: <00000000000075c58405bfd6228c@google.com> <20210413161311.GC1454681@rowland.harvard.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-Id: X-Mailing-List: linux-kernel@vger.kernel.org Received-SPF: pass client-ip=23.128.96.18; envelope-from=linux-kernel-owner@vger.kernel.org; helo=vger.kernel.org X-Spam-Score: -1.3 X-Spam-Score-Bar: - X-Spam-Action: no action X-Spam-Report: Action: no action Symbol: FORGED_SENDER_MAILLIST(0.00) Symbol: RBL_SENDERSCORE_FAIL(0.00) Symbol: R_SPF_ALLOW(-0.20) Symbol: TO_DN_ALL(0.00) Symbol: RCPT_COUNT_SEVEN(0.00) Symbol: MAILLIST(-0.10) Symbol: RCVD_NO_TLS_LAST(0.10) Symbol: FORGED_RECIPIENTS_M On Fri, Apr 16, 2021 at 11:10:35AM +0530, Anirudh Rayabharam wrote: > On Tue, Apr 13, 2021 at 12:13:11PM -0400, Alan Stern wrote: > > Maybe we can test this reasoning by putting a delay just before the call > > to dum->driver->setup. That runs in the timer handler, so it's not a > > good place to delay, but it may be okay just for testing purposes. > > > > Hopefully this patch will make the race a lot more likely to occur. Is > > Hi Alan, > > Indeed, I was able to reproduce this bug easily on my machine with your > delay patch applied and using this syzkaller program: > > syz_usb_connect$cdc_ncm(0x1, 0x6e, &(0x7f0000000040)={{0x12, 0x1, 0x0, 0x2, 0x0, 0x0, 0x8, 0x525, 0xa4a1, 0x40, 0x1, 0x2, 0x3, 0x1, [{{0x9, 0x2, 0x5c, 0x2, 0x1, 0x0, 0x0, 0x0, {{0x9, 0x4, 0x0, 0x0, 0x1, 0x2, 0xd, 0x0, 0x0, {{0x5}, {0x5}, {0xd}, {0x6}}, {{0x9, 0x5, 0x81, 0x3, 0x200}}}}}}]}}, &(0x7f0000000480)={0x0, 0x0, 0x0, 0x0, 0x3, [{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}]}) > > I also tested doing the synchronize_irq emulation in dummy_pullup and it > fixed the issue. The patch is below. That's great! Thanks for testing. > Thanks! > > - Anirudh. > > diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c > index ce24d4f28f2a..931d4612d859 100644 > --- a/drivers/usb/gadget/udc/dummy_hcd.c > +++ b/drivers/usb/gadget/udc/dummy_hcd.c > @@ -903,6 +903,12 @@ static int dummy_pullup(struct usb_gadget *_gadget, int value) > spin_lock_irqsave(&dum->lock, flags); > dum->pullup = (value != 0); > set_link_state(dum_hcd); > + /* emulate synchronize_irq(): wait for callbacks to finish */ > + while (dum->callback_usage > 0) { > + spin_unlock_irqrestore(&dum->lock, flags); > + usleep_range(1000, 2000); > + spin_lock_irqsave(&dum->lock, flags); > + } We should do this only if value == 0. No synchronization is needed when the pullup is turned on. Also, there should be a comment explaining that this is necessary because there's no other place to emulate the call made to synchronize_irq() in core.c:usb_gadget_remove_driver(). > spin_unlock_irqrestore(&dum->lock, flags); > > usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd)); > @@ -1005,13 +1011,6 @@ static int dummy_udc_stop(struct usb_gadget *g) > dum->ints_enabled = 0; > stop_activity(dum); > > - /* emulate synchronize_irq(): wait for callbacks to finish */ > - while (dum->callback_usage > 0) { > - spin_unlock_irq(&dum->lock); > - usleep_range(1000, 2000); > - spin_lock_irq(&dum->lock); > - } > - > dum->driver = NULL; > spin_unlock_irq(&dum->lock); Actually, I wanted to move this emulation code into a new subroutine and then call that subroutine from _both_ places. Would you like to write and submit a patch that does this? Alan Stern