From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87183C433ED for ; Tue, 4 May 2021 08:27:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 55C7B613B3 for ; Tue, 4 May 2021 08:27:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230015AbhEDI2a (ORCPT ); Tue, 4 May 2021 04:28:30 -0400 Received: from mx2.suse.de ([195.135.220.15]:41786 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229875AbhEDI23 (ORCPT ); Tue, 4 May 2021 04:28:29 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 2F88AB04F; Tue, 4 May 2021 08:27:34 +0000 (UTC) Date: Tue, 04 May 2021 10:27:34 +0200 Message-ID: From: Takashi Iwai To: Jaroslav Kysela Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb() In-Reply-To: References: <20210503115736.2104747-1-gregkh@linuxfoundation.org> <20210503115736.2104747-38-gregkh@linuxfoundation.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 03 May 2021 22:33:52 +0200, Jaroslav Kysela wrote: > > Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a): > > While it is almost impossible to hit an error calling usb_alloc_urb(), > > to make systems like syzbot which does error injection, and some static > > analysis tools happy, properly handle errors on this path by unwinding > > the previously allocated urbs and freeing them. > > Perhaps, I miss something, but this revert and add the cleanup to the wrong > place makes things worse: > > The usb_stream_free() is called when init_urbs() fails (returns an error), so > all urbs are freed there and pointers are reset to NULL. Your code frees urbs > twice on an allocation error. > > The reverted code does the job better. Right, the suggested patch will cause the double-free, and the reverted code is fine in this regard. In anyway, the cleanup of this driver code is on my post-15.3 TODO list. So, Greg, could you drop the revert and the additional fix at this round for usx2y driver? Thanks! Takashi > > Jaroslav > > > Cc: Takashi Iwai > > Cc: Jaroslav Kysela > > Signed-off-by: Greg Kroah-Hartman > > --- > > sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c > > index 6bba17bf689a..1091ea96a29a 100644 > > --- a/sound/usb/usx2y/usb_stream.c > > +++ b/sound/usb/usx2y/usb_stream.c > > @@ -88,18 +88,35 @@ static int init_urbs(struct usb_stream_kernel *sk, unsigned use_packsize, > > sizeof(struct usb_stream_packet) * > > s->inpackets; > > int u; > > + int i; > > + int err = -ENOMEM; > > > > for (u = 0; u < USB_STREAM_NURBS; ++u) { > > + sk->outurb[u] = NULL; > > sk->inurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL); > > + if (!sk->inurb[u]) > > + goto error; > > sk->outurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL); > > + if (!sk->outurb[u]) > > + goto error; > > } > > + u--; > > > > if (init_pipe_urbs(sk, use_packsize, sk->inurb, indata, dev, in_pipe) || > > init_pipe_urbs(sk, use_packsize, sk->outurb, sk->write_page, dev, > > - out_pipe)) > > - return -EINVAL; > > + out_pipe)) { > > + err = -EINVAL; > > + goto error; > > + } > > > > return 0; > > + > > +error: > > + for (i = 0; i <= u; ++i) { > > + usb_free_urb(sk->inurb[i]); > > + usb_free_urb(sk->outurb[i]); > > + } > > + return err; > > > > -- > Jaroslav Kysela > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. >