From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754631AbcCSNZh (ORCPT ); Sat, 19 Mar 2016 09:25:37 -0400 Received: from lists.s-osg.org ([54.187.51.154]:54857 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752825AbcCSNZc (ORCPT ); Sat, 19 Mar 2016 09:25:32 -0400 Subject: Re: [PATCH] sound/usb: fix to release stream resources from media_snd_device_delete() To: Mauro Carvalho Chehab References: <1458355831-9467-1-git-send-email-shuahkh@osg.samsung.com> <20160318235708.1eccf0e6@recife.lan> <20160319073944.60235d88@recife.lan> Cc: tiwai@suse.com, perex@perex.cz, linux-media@vger.kernel.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Shuah Khan From: Shuah Khan Organization: Samsung Open Source Group Message-ID: <56ED5343.1060401@osg.samsung.com> Date: Sat, 19 Mar 2016 07:25:23 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160319073944.60235d88@recife.lan> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/19/2016 04:39 AM, Mauro Carvalho Chehab wrote: > Em Fri, 18 Mar 2016 23:57:08 -0300 > Mauro Carvalho Chehab escreveu: > >> Em Fri, 18 Mar 2016 20:50:31 -0600 >> Shuah Khan escreveu: >> >>> Fix to release stream resources from media_snd_device_delete() before >>> media device is unregistered. Without this change, stream resource free >>> is attempted after the media device is unregistered which would result >>> in use-after-free errors. >>> >>> Signed-off-by: Shuah Khan >>> --- >>> >>> - Ran bind/unbind loop (1000 iteration) test on snd-usb-audio >>> while running mc_nextgen_test loop (1000 iterations) in parallel. >>> - Ran bind/unbind and rmmod/modprobe tests on both drivers. Also >>> generated graphs when after bind/unbind, rmmod/modprobe. Graphs >>> look good. >>> - Note: Please apply the following patch to fix memory leak: >>> sound/usb: Fix memory leak in media_snd_stream_delete() during unbind >>> https://lkml.org/lkml/2016/3/16/1050 >>> >>> sound/usb/media.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/sound/usb/media.c b/sound/usb/media.c >>> index de4a815..e35af88 100644 >>> --- a/sound/usb/media.c >>> +++ b/sound/usb/media.c >>> @@ -301,6 +301,13 @@ int media_snd_device_create(struct snd_usb_audio *chip, >>> void media_snd_device_delete(struct snd_usb_audio *chip) >>> { >>> struct media_device *mdev = chip->media_dev; >>> + struct snd_usb_stream *stream; >>> + >>> + /* release resources */ >>> + list_for_each_entry(stream, &chip->pcm_list, list) { >>> + media_snd_stream_delete(&stream->substream[0]); >>> + media_snd_stream_delete(&stream->substream[1]); >> >> I'll look on it better tomorrow, but it sounds weird to hardcode >> substream[0] and [1] here... are you sure that this is valid for >> *all* devices supported by snd-usb-audio? > > After looking at pcm.c and finding this: > > static void snd_usb_audio_stream_free(struct snd_usb_stream *stream) > { > free_substream(&stream->substream[0]); > free_substream(&stream->substream[1]); > list_del(&stream->list); > kfree(stream); > } > > It seems that assuming that substream is always an array with size 2 > is right. > > I'll do some tests with it today with your patch. > Right. snd-usb-audio uses this in several places like the one above you found. thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shuahkh@osg.samsung.com | (970) 217-8978 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shuah Khan Subject: Re: [PATCH] sound/usb: fix to release stream resources from media_snd_device_delete() Date: Sat, 19 Mar 2016 07:25:23 -0600 Message-ID: <56ED5343.1060401@osg.samsung.com> References: <1458355831-9467-1-git-send-email-shuahkh@osg.samsung.com> <20160318235708.1eccf0e6@recife.lan> <20160319073944.60235d88@recife.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from lists.s-osg.org (lists.s-osg.org [54.187.51.154]) by alsa0.perex.cz (Postfix) with ESMTP id 346702614BB for ; Sat, 19 Mar 2016 14:25:32 +0100 (CET) In-Reply-To: <20160319073944.60235d88@recife.lan> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Mauro Carvalho Chehab Cc: alsa-devel@alsa-project.org, tiwai@suse.com, Shuah Khan , linux-kernel@vger.kernel.org, linux-media@vger.kernel.org List-Id: alsa-devel@alsa-project.org On 03/19/2016 04:39 AM, Mauro Carvalho Chehab wrote: > Em Fri, 18 Mar 2016 23:57:08 -0300 > Mauro Carvalho Chehab escreveu: > >> Em Fri, 18 Mar 2016 20:50:31 -0600 >> Shuah Khan escreveu: >> >>> Fix to release stream resources from media_snd_device_delete() before >>> media device is unregistered. Without this change, stream resource free >>> is attempted after the media device is unregistered which would result >>> in use-after-free errors. >>> >>> Signed-off-by: Shuah Khan >>> --- >>> >>> - Ran bind/unbind loop (1000 iteration) test on snd-usb-audio >>> while running mc_nextgen_test loop (1000 iterations) in parallel. >>> - Ran bind/unbind and rmmod/modprobe tests on both drivers. Also >>> generated graphs when after bind/unbind, rmmod/modprobe. Graphs >>> look good. >>> - Note: Please apply the following patch to fix memory leak: >>> sound/usb: Fix memory leak in media_snd_stream_delete() during unbind >>> https://lkml.org/lkml/2016/3/16/1050 >>> >>> sound/usb/media.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/sound/usb/media.c b/sound/usb/media.c >>> index de4a815..e35af88 100644 >>> --- a/sound/usb/media.c >>> +++ b/sound/usb/media.c >>> @@ -301,6 +301,13 @@ int media_snd_device_create(struct snd_usb_audio *chip, >>> void media_snd_device_delete(struct snd_usb_audio *chip) >>> { >>> struct media_device *mdev = chip->media_dev; >>> + struct snd_usb_stream *stream; >>> + >>> + /* release resources */ >>> + list_for_each_entry(stream, &chip->pcm_list, list) { >>> + media_snd_stream_delete(&stream->substream[0]); >>> + media_snd_stream_delete(&stream->substream[1]); >> >> I'll look on it better tomorrow, but it sounds weird to hardcode >> substream[0] and [1] here... are you sure that this is valid for >> *all* devices supported by snd-usb-audio? > > After looking at pcm.c and finding this: > > static void snd_usb_audio_stream_free(struct snd_usb_stream *stream) > { > free_substream(&stream->substream[0]); > free_substream(&stream->substream[1]); > list_del(&stream->list); > kfree(stream); > } > > It seems that assuming that substream is always an array with size 2 > is right. > > I'll do some tests with it today with your patch. > Right. snd-usb-audio uses this in several places like the one above you found. thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shuahkh@osg.samsung.com | (970) 217-8978